From: Jan Beulich <jbeulich@suse.com>
To: Tsukasa OI <research_trasio@irq.a4lg.com>
Cc: binutils@sourceware.org
Subject: Re: [PATCH v3 2/2] RISC-V: Better support for long instructions (assembler)
Date: Fri, 25 Nov 2022 10:56:26 +0100 [thread overview]
Message-ID: <21f73985-0cfb-7e0e-522f-7ff0cbd5b5ab@suse.com> (raw)
In-Reply-To: <73fa81b0-afd7-e90e-57c1-8e32f8326002@irq.a4lg.com>
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
next prev parent reply other threads:[~2022-11-25 9:56 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-19 7:10 [PATCH 0/2] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Tsukasa OI
2022-11-19 7:10 ` [PATCH 1/2] RISC-V: Make .insn tests stricter Tsukasa OI
2022-11-21 7:32 ` Jan Beulich
2022-11-23 8:20 ` Tsukasa OI
2022-11-23 8:56 ` Jan Beulich
2022-11-19 7:10 ` [PATCH 2/2] RISC-V: Better support for long instructions Tsukasa OI
2022-11-21 7:37 ` Jan Beulich
2022-11-23 8:40 ` Tsukasa OI
2022-11-23 8:44 ` Jan Beulich
2022-11-23 8:51 ` Tsukasa OI
2022-11-25 1:38 ` Nelson Chu
2022-11-25 2:33 ` Tsukasa OI
2022-11-22 0:43 ` [PATCH 0/2] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Nelson Chu
2022-11-23 8:30 ` [PATCH v2 " Tsukasa OI
2022-11-23 8:30 ` [PATCH v2 1/2] RISC-V: Make .insn tests stricter Tsukasa OI
2022-11-23 8:30 ` [PATCH v2 2/2] RISC-V: Better support for long instructions Tsukasa OI
2022-11-23 9:04 ` Jan Beulich
2022-11-24 2:34 ` Tsukasa OI
2022-11-24 7:31 ` Jan Beulich
2022-11-24 7:35 ` Tsukasa OI
2022-11-25 2:17 ` [PATCH v3 0/2] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Tsukasa OI
2022-11-25 2:17 ` [PATCH v3 1/2] RISC-V: Better support for long instructions (disassembler) Tsukasa OI
2022-11-25 8:03 ` Jan Beulich
2022-11-25 2:17 ` [PATCH v3 2/2] RISC-V: Better support for long instructions (assembler) Tsukasa OI
2022-11-25 8:15 ` Jan Beulich
2022-11-25 8:39 ` Tsukasa OI
2022-11-25 9:04 ` Jan Beulich
2022-11-25 9:18 ` Tsukasa OI
2022-11-25 9:56 ` Jan Beulich [this message]
2022-11-25 11:07 ` Tsukasa OI
2022-11-25 11:41 ` [PATCH v3 0/3] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Tsukasa OI
2022-11-25 11:41 ` [PATCH v3 1/3] RISC-V: Better support for long instructions (disassembler) Tsukasa OI
2022-11-25 11:42 ` [PATCH v4 0/3] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Tsukasa OI
2022-11-25 11:42 ` [PATCH v4 1/3] RISC-V: Better support for long instructions (disassembler) Tsukasa OI
2022-11-25 11:42 ` [PATCH v4 2/3] RISC-V: Better support for long instructions (assembler) Tsukasa OI
2022-11-25 11:42 ` [PATCH v4 3/3] RISC-V: Better support for long instructions (tests) Tsukasa OI
2022-11-25 13:08 ` [PATCH v4 0/3] RISC-V: Better support for long instructions (64 < x <= 176 [bits]) Jan Beulich
2022-11-28 1:53 ` Nelson Chu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=21f73985-0cfb-7e0e-522f-7ff0cbd5b5ab@suse.com \
--to=jbeulich@suse.com \
--cc=binutils@sourceware.org \
--cc=research_trasio@irq.a4lg.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).