[PATCHv2] opcodes/x86: fix minor missed styling case
Checks
Commit Message
Jan Beulich <jbeulich@suse.com> writes:
> 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.
Comments
On 24.07.2024 13:58, Andrew Burgess wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>
>> 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.
And that one's okay to put in.
Jan
Jan Beulich <jbeulich@suse.com> writes:
> On 24.07.2024 13:58, Andrew Burgess wrote:
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> 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.
>
> And that one's okay to put in.
I pushed this patch.
Thanks for your patience.
Andrew
> > 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.
Thanks,
Lili.
> -----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.
Thx,
Haochen
>
> Thanks,
> Lili.
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.
> Thx,
> Haochen
>
> >
> > Thanks,
> > Lili.
>
"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
> 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.
@@ -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);