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

[Andrew Burgess]

"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...

Thanks,
Andrew