[PATCH v3 2/2] RISC-V: Better support for long instructions (assembler)
Tsukasa OI
research_trasio@irq.a4lg.com
Fri Nov 25 11:07:13 GMT 2022
On 2022/11/25 18:56, Jan Beulich wrote:
> On 25.11.2022 10:18, Tsukasa OI wrote:
>> 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).
>
> Oh, apologies: Disassembly then was wrong _only_ for the .byte part, but
> _not_ for the raw encoding? While indeed (going back) you said so in the
> original cover letter, the description of patch 1 doesn't make this
> clear. In that case, yes, I agree that the .byte part wants to be part
> of the expectations (but patch 1's description also wants adjusting).
>
> Jan
>
I'm so happy to resolve misunderstandings between us and I sincerely
apologize for misleading text. Hmm... splitting to three patches
(instead of two) might be better on PATCH v4.
1. Disassembler Fix
(clarify exactly what is going to be fixed: ".byte" output)
2. Assembler Fix
3. New testcases
(clarify that it tests both disassembler and assembler fixes)
PATCH v4 may not be submitted today but I will work on it.
Thanks,
Tsukasa
More information about the Binutils
mailing list