[PATCHv2] opcodes/x86: fix minor missed styling case

[Cui, Lili]

> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Thursday, July 25, 2024 5:00 PM
> To: H.J. Lu <hjl.tools@gmail.com>; Jiang, Haochen <haochen.jiang@intel.com>
> Cc: Cui, Lili <lili.cui@intel.com>; Beulich, Jan <JBeulich@suse.com>; Binutils
> <binutils@sourceware.org>
> Subject: Re: [PATCHv2] opcodes/x86: fix minor missed styling case
> 
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> 
> > On Thu, Jul 25, 2024, 10:12 AM Jiang, Haochen
> > <haochen.jiang@intel.com>
> > wrote:
> >
> >> > -----Original Message-----
> >> > From: Cui, Lili <lili.cui@intel.com>
> >> > Sent: Thursday, July 25, 2024 9:53 AM
> >> > To: Andrew Burgess <aburgess@redhat.com>; Beulich, Jan <
> >> JBeulich@suse.com>
> >> > Cc: H.J. Lu <hjl.tools@gmail.com>; Jiang, Haochen <
> >> haochen.jiang@intel.com>;
> >> > binutils@sourceware.org
> >> > Subject: RE: [PATCHv2] opcodes/x86: fix minor missed styling case
> >> >
> >> >
> >> > > > On 24.07.2024 04:31, Cui, Lili wrote:
> >> > > >>>> I noticed that the x86 instruction:
> >> > > >>>>
> >> > > >>>>   sar    $0x1,%rsi
> >> > > >>>>
> >> > > >>>> would fail to style the '$0x1' as an immediate.  This commit
> >> > > >>>> fixes that
> >> > > case.
> >> > > >>>>
> >> > > >>
> >> > > >> I'm afraid it is not a bug, it is to distinguish between the
> >> > > >> two formats
> >> > > below.
> >> > > >>
> >> > > >> sar    r/m8, 1
> >> > > >> sar    r/m8, imm8
> >> > > >
> >> > > > It is a bug, but it also is a bug to change 1 to 0x1, as that
> >> > > > way said distinction goes away. (I also don't immediately see
> >> > > > how the code change alone would pass the testsuite; I'm pretty
> >> > > > sure we have expectations which would have required adjustment,
> >> > > > which would have made more obvious that the change wants doing
> >> > > > differently.)
> >> > >
> >> > > You are correct, I got sloppy, and I apologise.
> >> > >
> >> > > Thanks to everyone who pointed out the mistake.
> >> > >
> >> > > Here's an update, _fully_ tested patch.
> >> > >
> >> > > Thanks,
> >> > > Andrew
> >> > >
> >> > > ---
> >> > >
> >> > > commit ffe8ab67ab81ae24105433145ca6ee40c3fb92e2
> >> > > Author: Andrew Burgess <aburgess@redhat.com>
> >> > > Date:   Tue Jul 23 17:10:22 2024 +0100
> >> > >
> >> > >     opcodes/x86: fix minor missed styling case
> >> > >
> >> > >     I noticed that the x86 instruction:
> >> > >
> >> > >       sar    $1,%rsi
> >> > >
> >> > >     would fail to style the '$0x1' as an immediate.  This commit fixes
> >> > >     that case.
> >> > >
> >> > > diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c index
> >> > > bc141f31770..59ec771369a 100644
> >> > > --- a/opcodes/i386-dis.c
> >> > > +++ b/opcodes/i386-dis.c
> >> > > @@ -12415,9 +12415,9 @@ OP_I (instr_info *ins, int bytemode, int
> >> sizeflag)
> >> > >        break;
> >> > >      case const_1_mode:
> >> > >        if (ins->intel_syntax)
> >> > > -   oappend (ins, "1");
> >> > > +   oappend_with_style (ins, "1", dis_style_immediate);
> >> > >        else
> >> > > -   oappend (ins, "$1");
> >> > > +   oappend_with_style (ins, "$1", dis_style_immediate);
> >> > >        return true;
> >> > >      default:
> >> > >        oappend (ins, INTERNAL_DISASSEMBLER_ERROR);
> >> >
> >> > I am a little confused, how does this patch fix this case in the
> >> comment, the
> >> > output is not changed before and after the patch.
> >>
> >> Confused +1. It seems Changelog and the current output doesn't match
> >> or I misunderstood something.
> >>
> >
> > I think you will see the different output only when style is enabled.
> 
> Indeed.
> 
> I spotted this bug via GDB which styles disassembler output by default.
> Here's how you can see the change using just gas/objdump:
> 
>   $ cat sar.s
>           .text
>           sar $1, %rsi
> 
>   $ as -o sar.o sar.s
>   $ objdump -d --disassembler-color=on sar.o
> 
>   sar.o:     file format elf64-x86-64
> 
> 
>   Disassembly of section .text:
> 
>   0000000000000000 <.text>:
>      0:	48 d1 fe             	sar    $1,%rsi
> 
> Before the patch the '$1' will be styled as text (i.e. default terminal text
> colour).  After the patch the '$1' will be styled as an immediate.
> 
> Unfortunately there's (currently) no mechanism to adjust the disassembler
> colours used by objdump, so the colours don't work well on all terminals.
> GDB allows all the colours to be adjusted so you can set things up to work
> well based on your terminal colours.
> 
> I did write a patch to allow colour adjustment in objdump but it got rejected.  I
> need to come up with a different approach.  It is on my todo list...
> 

Oh, I see, thank you all for the detailed answers.

Lili.