[2/2] RISC-V: Better support for long instructions

  From: Tsukasa OI <research_trasio@irq.a4lg.com>

Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
instructions with .insn") tried to start supporting long instructions but
it was insufficient.

1.  It heavily depended on the bignum internals (radix of 2^16),
2.  It generates "value conflicts with instruction length" even if a big
    number instruction encoding does not exceed its expected length,
3.  Because long opcode was handled separately (from struct riscv_cl_insn),
    some information like DWARF line number correspondence was missing and
4.  On the disassembler, disassembler dump was limited up to 64-bit.
    For long (unknown) instructions, instruction bits are incorrectly
    zeroed out.

To solve these problems, this commit:

1.  Handles bignum (and its encodings) precisely,
2.  Incorporates long opcode handling into regular struct
    riscv_cl_insn-handling functions.
3.  Adds packet argument to support dumping instructions
    longer than 64-bits.

gas/ChangeLog:

	* config/tc-riscv.c (struct riscv_cl_insn): Add long opcode field.
	(create_insn) Clear long opcode marker.
	(install_insn) Install longer opcode as well.
	(s_riscv_insn) Likewise.
	(riscv_ip_hardcode): Make big number handling stricter. Length and
	the value conflicts only if the bignum size exceeds the expected
	length.
	* testsuite/gas/riscv/insn.s: Add testcases such that big number
	handling is required.
	* testsuite/gas/riscv/insn.d: Likewise.
	* testsuite/gas/riscv/insn-na.d: Likewise.
	* testsuite/gas/riscv/insn-dwarf.d: Likewise.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_disassemble_insn): Print unknown instruction
	using the new argument packet.
	(riscv_disassemble_data): Add unused argument packet.
	(print_insn_riscv): Pass packet to the disassemble function.
---
 gas/config/tc-riscv.c                | 50 +++++++++++++++++++++++-----
 gas/testsuite/gas/riscv/insn-dwarf.d | 10 +++++-
 gas/testsuite/gas/riscv/insn-na.d    |  8 +++++
 gas/testsuite/gas/riscv/insn.d       | 22 ++++++++++++
 gas/testsuite/gas/riscv/insn.s       |  9 +++++
 opcodes/riscv-dis.c                  | 13 +++++---
 6 files changed, 98 insertions(+), 14 deletions(-)
  

On 19.11.2022 08:10, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
> 
> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
> instructions with .insn") tried to start supporting long instructions but
> it was insufficient.
> 
> 1.  It heavily depended on the bignum internals (radix of 2^16),
> 2.  It generates "value conflicts with instruction length" even if a big
>     number instruction encoding does not exceed its expected length,
> 3.  Because long opcode was handled separately (from struct riscv_cl_insn),
>     some information like DWARF line number correspondence was missing and
> 4.  On the disassembler, disassembler dump was limited up to 64-bit.
>     For long (unknown) instructions, instruction bits are incorrectly
>     zeroed out.

Just FTR - of these 1 and 4 were deliberate (as in: deemed acceptable), the
former the keep the code reasonably simple and the latter because focus was
solely on the assembler.

I recall dealing with some instances of 2, but as you demonstrate I failed
to recognize and deal with further cases.

I will admit that I didn't even think of debug info generation.

> To solve these problems, this commit:
> 
> 1.  Handles bignum (and its encodings) precisely,
> 2.  Incorporates long opcode handling into regular struct
>     riscv_cl_insn-handling functions.
> 3.  Adds packet argument to support dumping instructions
>     longer than 64-bits.

Thanks for taking the time to make improvements there.

Jan
  

c.f. PATCH v2 2/2
<https://sourceware.org/pipermail/binutils/2022-November/124598.html>

On 2022/11/21 16:37, Jan Beulich wrote:
> On 19.11.2022 08:10, Tsukasa OI wrote:
>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>
>> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
>> instructions with .insn") tried to start supporting long instructions but
>> it was insufficient.
>>
>> 1.  It heavily depended on the bignum internals (radix of 2^16),
>> 2.  It generates "value conflicts with instruction length" even if a big
>>     number instruction encoding does not exceed its expected length,
>> 3.  Because long opcode was handled separately (from struct riscv_cl_insn),
>>     some information like DWARF line number correspondence was missing and
>> 4.  On the disassembler, disassembler dump was limited up to 64-bit.
>>     For long (unknown) instructions, instruction bits are incorrectly
>>     zeroed out.
> 
> Just FTR - of these 1 and 4 were deliberate (as in: deemed acceptable), the
> former the keep the code reasonably simple and the latter because focus was
> solely on the assembler.

I thought it's possible that 1 was a deliberate choice.  I don't want to
depend on some internal structure that could change easily (as long as
this is a reasonable choice).  To be honest, I didn't like my
"extracting prefix of an instruction" logic in PATCH v1 but I found a
good function: generic_bignum_to_int32 and decided use it on PATCH v2
(as a result, PATCH v2 2/2 is a bit simpler than PATCH v1).

For 4, resolving from the start would be better but since my current
focus is the RISC-V disassembler, I'm happy to resolve it (fortunately,
it didn't require large changes).

Nelson assigned you as the person who makes the final judgement for this
series and I want to hear your thoughts/decision about PATCH v2 2/2.

Thanks,
Tsukasa

> 
> I recall dealing with some instances of 2, but as you demonstrate I failed
> to recognize and deal with further cases.
> 
> I will admit that I didn't even think of debug info generation.
> 
>> To solve these problems, this commit:
>>
>> 1.  Handles bignum (and its encodings) precisely,
>> 2.  Incorporates long opcode handling into regular struct
>>     riscv_cl_insn-handling functions.
>> 3.  Adds packet argument to support dumping instructions
>>     longer than 64-bits.
> 
> Thanks for taking the time to make improvements there.
> 
> Jan
>
  

On 23.11.2022 09:40, Tsukasa OI wrote:
> c.f. PATCH v2 2/2
> <https://sourceware.org/pipermail/binutils/2022-November/124598.html>
> 
> On 2022/11/21 16:37, Jan Beulich wrote:
>> On 19.11.2022 08:10, Tsukasa OI wrote:
>>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>>
>>> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
>>> instructions with .insn") tried to start supporting long instructions but
>>> it was insufficient.
>>>
>>> 1.  It heavily depended on the bignum internals (radix of 2^16),
>>> 2.  It generates "value conflicts with instruction length" even if a big
>>>     number instruction encoding does not exceed its expected length,
>>> 3.  Because long opcode was handled separately (from struct riscv_cl_insn),
>>>     some information like DWARF line number correspondence was missing and
>>> 4.  On the disassembler, disassembler dump was limited up to 64-bit.
>>>     For long (unknown) instructions, instruction bits are incorrectly
>>>     zeroed out.
>>
>> Just FTR - of these 1 and 4 were deliberate (as in: deemed acceptable), the
>> former the keep the code reasonably simple and the latter because focus was
>> solely on the assembler.
> 
> I thought it's possible that 1 was a deliberate choice.  I don't want to
> depend on some internal structure that could change easily (as long as
> this is a reasonable choice).  To be honest, I didn't like my
> "extracting prefix of an instruction" logic in PATCH v1 but I found a
> good function: generic_bignum_to_int32 and decided use it on PATCH v2
> (as a result, PATCH v2 2/2 is a bit simpler than PATCH v1).

FAOD by saying "deliberate" I have by no means meant to say that I'm not
happy to see you improve the state of things. I was merely trying to give
some background.

Jan

> For 4, resolving from the start would be better but since my current
> focus is the RISC-V disassembler, I'm happy to resolve it (fortunately,
> it didn't require large changes).
> 
> Nelson assigned you as the person who makes the final judgement for this
> series and I want to hear your thoughts/decision about PATCH v2 2/2.
> 
> Thanks,
> Tsukasa
  

On 2022/11/23 17:44, Jan Beulich wrote:
> On 23.11.2022 09:40, Tsukasa OI wrote:
>> c.f. PATCH v2 2/2
>> <https://sourceware.org/pipermail/binutils/2022-November/124598.html>
>>
>> On 2022/11/21 16:37, Jan Beulich wrote:
>>> On 19.11.2022 08:10, Tsukasa OI wrote:
>>>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>>>
>>>> Commit bb996692bd96 ("RISC-V/gas: allow generating up to 176-bit
>>>> instructions with .insn") tried to start supporting long instructions but
>>>> it was insufficient.
>>>>
>>>> 1.  It heavily depended on the bignum internals (radix of 2^16),
>>>> 2.  It generates "value conflicts with instruction length" even if a big
>>>>     number instruction encoding does not exceed its expected length,
>>>> 3.  Because long opcode was handled separately (from struct riscv_cl_insn),
>>>>     some information like DWARF line number correspondence was missing and
>>>> 4.  On the disassembler, disassembler dump was limited up to 64-bit.
>>>>     For long (unknown) instructions, instruction bits are incorrectly
>>>>     zeroed out.
>>>
>>> Just FTR - of these 1 and 4 were deliberate (as in: deemed acceptable), the
>>> former the keep the code reasonably simple and the latter because focus was
>>> solely on the assembler.
>>
>> I thought it's possible that 1 was a deliberate choice.  I don't want to
>> depend on some internal structure that could change easily (as long as
>> this is a reasonable choice).  To be honest, I didn't like my
>> "extracting prefix of an instruction" logic in PATCH v1 but I found a
>> good function: generic_bignum_to_int32 and decided use it on PATCH v2
>> (as a result, PATCH v2 2/2 is a bit simpler than PATCH v1).
> 
> FAOD by saying "deliberate" I have by no means meant to say that I'm not
> happy to see you improve the state of things. I was merely trying to give
> some background.
> 
> Jan

Sorry, I never meant to blame you.

Tsukasa

> 
>> For 4, resolving from the start would be better but since my current
>> focus is the RISC-V disassembler, I'm happy to resolve it (fortunately,
>> it didn't require large changes).
>>
>> Nelson assigned you as the person who makes the final judgement for this
>> series and I want to hear your thoughts/decision about PATCH v2 2/2.
>>
>> Thanks,
>> Tsukasa
>
  

On Wed, Nov 23, 2022 at 4:40 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Nelson assigned you as the person who makes the final judgement for this
> series and I want to hear your thoughts/decision about PATCH v2 2/2.

I'm not a natural language speaker, so I thought this should be a
little bit of a misunderstanding.  "Assigned" isn't a suitable word
here, Jan is one of the global binutils maintainers, so he definitely
has the right to approve anything by himself, including the target
stuff.  Not only for these two patches, I will definitely respect all
of his suggestions, just like he always respects the decision from
target maintainers.  Therefore, I am glad he has time to help review
and do lots of risc-v stuff, and also thanks to everyone who helped
risc-v, of course including you :)

Thanks
Nelson
  

On 2022/11/25 10:38, Nelson Chu wrote:
> On Wed, Nov 23, 2022 at 4:40 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>>
>> Nelson assigned you as the person who makes the final judgement for this
>> series and I want to hear your thoughts/decision about PATCH v2 2/2.
> 
> I'm not a natural language speaker, so I thought this should be a
> little bit of a misunderstanding.  "Assigned" isn't a suitable word
> here, Jan is one of the global binutils maintainers, so he definitely

Yes.  After sending the message, I noticed that I used a "too strong"
word here.  Nelson's understandings are correct and thanks for your support.

I just submitted the PATCH v3 and I hope that Jan likes it.

Thanks,
Tsukasa

> has the right to approve anything by himself, including the target
> stuff.  Not only for these two patches, I will definitely respect all
> of his suggestions, just like he always respects the decision from
> target maintainers.  Therefore, I am glad he has time to help review
> and do lots of risc-v stuff, and also thanks to everyone who helped
> risc-v, of course including you :)
> 
> Thanks
> Nelson
>