[PATCH v3 2/2] RISC-V: Better support for long instructions (assembler)

Fri Nov 25 09:04:20 GMT 2022

On 25.11.2022 09:39, Tsukasa OI wrote:
> On 2022/11/25 17:15, Jan Beulich wrote:
>> On 25.11.2022 03:17, Tsukasa OI wrote:
>>> --- a/gas/testsuite/gas/riscv/insn-na.d
>>> +++ b/gas/testsuite/gas/riscv/insn-na.d
>>> @@ -73,3 +73,11 @@ Disassembly of section .text:
>>>  [^:]+:[ 	]+007f 0000 0000 0000 0000[ 	]+[._a-z].*
>>>  [^:]+:[ 	]+0000107f 00000000 00000000[ 	]+[._a-z].*
>>>  [^:]+:[ 	]+607f 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000[ 	]+[._a-z].*
>>> +[^:]+:[ 	]+007f 0000 0000 0000 8000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>> +[^:]+:[ 	]+007f 0000 0000 0000 8000[ 	]+\.byte[ 	]+0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80
>>> +[^:]+:[ 	]+607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000[ 	]+\.byte[ 	]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>
>> I have to admit that I still don't see what good the ".byte ..." part of
>> the expectations does for the purpose of the test. In the cover letter
>> you say "They are not 4-byte aligned (10 and 22-bytes) and unlikely to
>> change any time soon." But changing that is exactly my plan (unless
>> objected to by the arch maintainers): Showing insn components as bytes
>> is imo reasonable for RISC-V at most when things aren't even 2-byte
>> aligned. IOW I'd see these to be "disassembled" to ".2byte ...",
>> matching the "raw opcode" output left to .<N>byte. In fact when raw
>> opcodes are output I question the need for any .<N>byte - it's fully
>> redundant>
>> Bottom line: As before I'd prefer if these parts were dropped (to limit
>> the churn on the files when changing the .<N>byte granularity), but I'm
>> not going to insist. Apart from this the change looks good to me.
> 
> Okay, I have to admit that I misunderstood your intent.
> 
> Quoting my PATCH v1 cover letter:
> 
>> [Disassembler: Instruction is trimmed with 64-bits]
>>
>> In this section, we reuse the object file generated by the section above
>> (two 22-byte instructions).
>>
>>     0000000000000000 <.text>:
>>        0:   607f 0000 0000 0000     .byte   0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>        8:   0000 0000 0000 0000
>>       10:   0000 0000 0000
>>       16:   607f 33cc 55aa cdef     .byte   0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>       1e:   89ab 4567 0123 3210                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>       26:   7654 ba98 fedc                                                                  zeroed out after first 64-bits
>>
>> See ".byte" at the address 0x16.  It's trimmed at 64-bits.
>> The resolution is simple.  If we simply add a char* argument (containing all
>> instruction bits), we can easily resolve this.
>>
>>     0000000000000000 <.text>:
>>        0:   607f 0000 0000 0000     .byte   0x7f, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>        8:   0000 0000 0000 0000
>>       10:   0000 0000 0000
>>       16:   607f 33cc 55aa cdef     .byte   0x7f, 0x60, 0xcc, 0x33, 0xaa, 0x55, 0xef, 0xcd, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe
>>       1e:   89ab 4567 0123 3210                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>       26:   7654 ba98 fedc                                                                  all instruction bits are correct
> 
> At least, I want to test whether disassembly of this part (after first
> 64-bits of an instruction) is fixed.  That is exactly what's fixed in
> PATCH 1/2 and I want to make sure that this part is changed correctly.

Which you already achieve by the raw opcode output

607f 89ab 4567 0123 3210 7654 ba98 fedc 0000 0000 0000

The subsequent

.byte[ 	]+0x7f, 0x60, 0xab, 0x89, 0x67, 0x45, 0x23, 0x01, 0x10, 0x32, 0x54, 0x76, 0x98, 0xba, 0xdc, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00

does not check anything the first part didn't already check.

> If you change the output, you can freely remove or replace the testcase
> according to the new output format but until that happens, I want to
> keep those.  What am I missing?

I can only restate what I've said earlier: Testcases would imo better be
supplying as strict as necessary but as relaxed as possible expectations.
If it was truly the _intention_ for .byte (and not e.g. .2byte) to be
used here, _then_ it would make sense to have this be part of the
expectations. And then, by recording it that way, you also raise the
barrier of someone changing the behavior - after all the testcase then
says it is intended to be that way (rather than, as I assume here, it
being merely "the way it is").

And yes, if you look at other testcase expectations you will frequently
find way too strict expectations (often enough people simply take the
output of the dumping tool, massage it suitably to be regex-es, and be
done - sometimes with the effect of event recording outright wrong
expectations, simply because huge output is cumbersome to check for
correctness). In many cases this has resulted in unnecessarily big code
churn when extending such testcases, or unhelpful mishmash because of
people then always adding to the end instead of at a more sensible
place (e.g. next to related stuff). Hence in particular for a still
relatively new and tidy port like RISC-V is, it would seem desirable to
me to learn from mistakes made elsewhere before.

Yet to reiterate - I'm not going to insist, first and foremost because
binutils, unlike other projects, has overall relatively relaxed
acceptance criteria for patches.

Jan