[PATCH v3 2/2] RISC-V: Better support for long instructions (assembler)
Tsukasa OI
research_trasio@irq.a4lg.com
Fri Nov 25 09:18:13 GMT 2022
On 2022/11/25 18:04, Jan Beulich wrote:
> 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.
That's simply not true.
Again, quoting from PATCH v1 cover letter (BEFORE THE PATCH
[disassembler part]):
> 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
Compare hexdump and printed bytes (in ".byte"). You can see that "1e:
89ab..." are different from corresponding ".byte" (0x00, 0x00...).
They are completely separate and PATCH 1/2 only changes ".byte" output.
If ".byte[ ]+0x7f, 0x60..." does not check anything the first part
didn't already check, the dump would look like this:
> 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: 0000 0000 0000 0000 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 26: 0000 0000 0000 zeroed out after first 64-bits (but matches to hexdump)
If the hexdump and ".byte" output always matches EVEN BEFORE THE FIX,
that's what I can call "redundant". But in reality, they do not.
That's why I want to leave a test to make sure that the issue is fixed
(now hexdump and ".byte" output always matches).
Tsukasa
>
>> 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
>
More information about the Binutils
mailing list