Re: [PATCH v2 0/6] RISC-V: Fix disassembler types and styles

[Palmer Dabbelt <palmer@dabbelt.com>]

On Tue, 04 Oct 2022 01:46:01 PDT (-0700), aburgess@redhat.com wrote:
> Palmer Dabbelt <palmer@dabbelt.com> writes:
>> On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote:
>>> Tsukasa OI via Binutils <binutils@sourceware.org> writes:
>>>
>>>> Hello,
>>>>
>>>> This patchset fixes various typing and styling errors on the RISC-V
>>>> disassembler.
>>>
>>> I can't approve binutils patches.
>>>
>>> I've taken a look through this series, and other than the minor typo I
>>> spotted, this all looks good to me.
>>
>> My worry with these things is that it's sort of a grey area in terms of
>> what our stable user interface is: there's definitely folks who try and
>> parse the text output of tools like objdump and changing anything risks
>> breaking that.  IMO that's too strict of a stable interface to keep
>> around as it pretty much prevents us from fixing any disassembly
>> weirdness, but I'm not sure if that's what other ports do and I don't
>> want to make a mess by breaking users' expectations here.
>
> I guess we'd want to consider these things on a case-by-case basis.  I'm
> struggling to think of a situation where we wouldn't want to fix
> something in the disassembler output if it was wrong over leaving it
> incorrect because someone, somewhere, might have a script that depends
> on the broken output.
>
> But unless I've missed something, I don't think this series makes any
> changes to the output other than styling, and I don't think a script
> that's trying to parse the disassembler output should be running with
> styling switched on....
>
> ...or if they are, maybe because they want to figure out which bits of
> the text are which, then surely they'd welcome these fixes?
>
>> That said, as far as I can tell the only user-visible change here is to
>> print some bits with the correct syntax coloring in GDB.
>
> And in objdump, which also styles its output by default now when the
> output is going to a tty.  Any scripts that capture objdump output
> should see no changes with this series.

Ah, cool, I hadn't seen that when poking around.  I don't think it 
changes the reasoning, though.

> The styling is pretty new, and I knew when I added it there were going
> to be mistakes with things styled incorrectly.  I was basically relying
> on getting something in that was not terrible, and then relying on folk
> like Tsukasa to come along and fix my mistakes.
>
> IMHO, any script that relies on a fixed styling is a bad script.

Ya, that seems like an easy one -- doubly so if it's relying on styling 
that's wrong it a handful of cases.  Like you said, if anything this 
would fix scripts rather than break them.

> I'd probably go further, and say that any script that can't handle minor
> changes in the disassembly output at all is a bad script, but without
> seeing every case that might be a little harsh.

I've definitely written scripts like that, and it felt pretty bad at the 
time.  It'd still be a small headache if they broke: less actually 
fixing the script, but more that some test would break on a binutils 
upgrade and that tends to spook the HW folks into just never upgrading 
the toolchain again (even if it's just a test checking script that broken).

That kind of stuff is someone else's problem these days, though, so I'm 
fine just pretending none of it ever happened ;)

> Anyway, FWIW, I think we should for sure approve this series.

Works for me.  Sounds like Jeff was saying about the same thing in his 
reply, so approved from me.

Tsukasa: I haven't built/tested these and my box is kind of tied up with 
Linux right now, but feel free to commit them assuming someone's tested 
them.

Thanks!

>
> Thanks,
> Andrew
>
>
>>                                                           Seems pretty
>> straight-forward to say anything users can't rely on parsing syntax
>> highlighting colors being a stable interface, so maybe we can just punt
>> on the grey area for now ;)
>>
>>> Thanks for doing this.
>>
>> Yep.  These LGTM, but I'm still kind of buried thanks to the
>> a few weeks of conference->COVID.  They're probably fine, but hopefully
>> Nelson has the time to chime in -- if not I'll leave them in the queue.
>>
>> Thanks!
>>
>>>
>>> Andrew
>>>
>>>
>>>
>>>>
>>>> Previous Patchsets:
>>>> <https://sourceware.org/pipermail/binutils/2022-July/121790.html> (2022-07-13)
>>>> <https://sourceware.org/pipermail/binutils/2022-August/122172.html> (2022-08-03)
>>>> Tracker on GitHub:
>>>> <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_printf_styles_and_types_1>
>>>>
>>>>
>>>> [Changes: v1 -> v2]
>>>>
>>>> -   Added PATCH 5-6/6 after T-Head extensions are merged.
>>>> -   Slightly improved the commit messages?
>>>>
>>>>
>>>> [Styling]
>>>> -   Print real immediates with the `immediate' style         (PATCH 1/6)
>>>> -   Print comma and tabs with the `text' style               (PATCH 4/6)
>>>> -   Print T-Head literal operand with `immediate' style      (PATCH 6/6)
>>>> [Typing on printf]
>>>> -   Fix wrong type for "%x" format specifier                 (PATCH 2/6)
>>>> -   Fix minor but various typing issues on T-Head immediates (PATCH 5/6)
>>>> [Small Optimization]
>>>> -   Use smallest portable types possible to print            (PATCH 3/6)
>>>>
>>>> Thanks,
>>>> Tsukasa
>>>>
>>>>
>>>>
>>>>
>>>> Tsukasa OI (6):
>>>>   RISC-V: Fix immediates to have "immediate" style
>>>>   RISC-V: Fix printf argument types corresponding %x
>>>>   RISC-V: Optimize riscv_disassemble_data printf
>>>>   RISC-V: Print comma and tabs as the "text" style
>>>>   RISC-V: Fix T-Head immediate types on printing
>>>>   RISC-V: Print XTheadMemPair literal as "immediate"
>>>>
>>>>  opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++--------------------
>>>>  1 file changed, 39 insertions(+), 32 deletions(-)
>>>>
>>>>
>>>> base-commit: c21736aed1d4877e090df60362413669dbdc391d
>>>> --
>>>> 2.34.1