diff mbox series

opcodes/x86: fix minor missed styling case

Message ID 6efd2f1eab5550461e715c007272a3120ebd3dc8.1721751185.git.aburgess@redhat.com
State New
Headers
Series opcodes/x86: fix minor missed styling case |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 fail Patch failed to apply

Commit Message

Andrew Burgess July 23, 2024, 4:13 p.m. UTC 
  I noticed that the x86 instruction:

  sar    $0x1,%rsi

would fail to style the '$0x1' as an immediate.  This commit fixes
that case.
---
 opcodes/i386-dis.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)


base-commit: 40578beee8a593e3668852238fd8e9f53790f2c9

Comments

Jiang, Haochen July 24, 2024, 2:03 a.m. UTC | #1 
> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, July 24, 2024 12:13 AM
> To: binutils@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>
> Subject: [PATCH] opcodes/x86: fix minor missed styling case
> 
> I noticed that the x86 instruction:
> 
>   sar    $0x1,%rsi
> 
> would fail to style the '$0x1' as an immediate.  This commit fixes that case.
> ---
>  opcodes/i386-dis.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c index
> bc141f31770..d44fee33eb5 100644
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -12414,11 +12414,8 @@ OP_I (instr_info *ins, int bytemode, int
> sizeflag)
>  	}
>        break;
>      case const_1_mode:
> -      if (ins->intel_syntax)
> -	oappend (ins, "1");
> -      else
> -	oappend (ins, "$1");

It seems fixed the issue when the immediate is not decimal format right?

Thx,
Haochen

> -      return true;
> +      op = 1;
> +      break;
>      default:
>        oappend (ins, INTERNAL_DISASSEMBLER_ERROR);
>        return true;
> 
> base-commit: 40578beee8a593e3668852238fd8e9f53790f2c9
> --
> 2.25.4
Cui, Lili July 24, 2024, 2:31 a.m. UTC | #2 
> > 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

Lili.
> >  opcodes/i386-dis.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c index
> > bc141f31770..d44fee33eb5 100644
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -12414,11 +12414,8 @@ OP_I (instr_info *ins, int bytemode, int
> > sizeflag)
> >  	}
> >        break;
> >      case const_1_mode:
> > -      if (ins->intel_syntax)
> > -	oappend (ins, "1");
> > -      else
> > -	oappend (ins, "$1");
> 
> It seems fixed the issue when the immediate is not decimal format right?
> 
> Thx,
> Haochen
> 
> > -      return true;
> > +      op = 1;
> > +      break;
> >      default:
> >        oappend (ins, INTERNAL_DISASSEMBLER_ERROR);
> >        return true;
> >
> > base-commit: 40578beee8a593e3668852238fd8e9f53790f2c9
> > --
> > 2.25.4
Jan Beulich July 24, 2024, 5:55 a.m. UTC | #3 
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.)

Jan
Cui, Lili July 24, 2024, 8:42 a.m. UTC | #4 
> 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.)
> 
Jan, do you have any suggestions on how to distinguish between IMM1 and IMM8? It seems that the current distinction can easily cause confusion.

Currently,
Intel format: disassembler prints 1 for Imm1 and 0x1 for Imm8.
ATT  format: disassembler prints $1 for Imm1 and $0x1 for Imm8.


Thanks,
Lili.
Jan Beulich July 24, 2024, 8:46 a.m. UTC | #5 
On 24.07.2024 10:42, Cui, Lili wrote:
>> 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.)
>>
> Jan, do you have any suggestions on how to distinguish between IMM1 and IMM8? It seems that the current distinction can easily cause confusion.
> 
> Currently,
> Intel format: disassembler prints 1 for Imm1 and 0x1 for Imm8.
> ATT  format: disassembler prints $1 for Imm1 and $0x1 for Imm8.

And that's fine. It's just that the Imm1 case also wants styling as
immediate (without the prepending any 0x). If and when we move to not
always emitting 0x for immediates in general (which is a plan I have
been having for quite some time), your question would actually become
relevant. Provided we actually continue to think there is a need to
distinguish the two forms in disassembly. I don't think we make
recognizable which encoding is in use in a number of other cases
where multiple encodings exist.

Jan
diff mbox series

Patch

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index bc141f31770..d44fee33eb5 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -12414,11 +12414,8 @@  OP_I (instr_info *ins, int bytemode, int sizeflag)
 	}
       break;
     case const_1_mode:
-      if (ins->intel_syntax)
-	oappend (ins, "1");
-      else
-	oappend (ins, "$1");
-      return true;
+      op = 1;
+      break;
     default:
       oappend (ins, INTERNAL_DISASSEMBLER_ERROR);
       return true;