Re: Inconsistent usage on onebyte_modrm and twobyte_modrm table in x86 disassembler and gdb?

[Tom de Vries <tdevries@suse.de>]

On 8/25/25 11:26, Alexander Monakov wrote:
> Hi,
> 
> On Mon, 25 Aug 2025, Sam James wrote:
> 
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> On 25.08.2025 04:42, Jiang, Haochen wrote:
>>>> Hi all,
>>>>
>>>> Recently I happened to have a look at the gdb code. At gdb/amd64-tdep.c
>>>> L1102 comment, it mentioned that:
>>>>
>>>> /* WARNING: Keep onebyte_has_modrm, twobyte_has_modrm in sync with
>>>>     ../opcodes/i386-dis.c (until libopcodes exports them, or an alternative,
>>>>     at which point delete these in favor of libopcodes' versions).  */
>>>>
>>>> This means the table content and usage should be the same as gas.
>>>>
>>>> However, when we are using the table in disassembler at opcode/i386-dis.c
>>>> L9877, it is:
>>>>
>>>>    /* REX2.M in rex2 prefix represents map0 or map1.  */
>>>>    if (ins.last_rex2_prefix < 0 ? *ins.codep == 0x0f : (ins.rex2 & REX2_M))
>>>>      {
>>>>        if (!ins.rex2)
>>>>          {
>>>>            ins.codep++;
>>>>            if (!fetch_code (info, ins.codep + 1))
>>>>              goto fetch_error_out;
>>>>          }
>>>>
>>>>        dp = &dis386_twobyte[*ins.codep];
>>>>        ins.need_modrm = twobyte_has_modrm[*ins.codep];
>>>>      }
>>>>    else
>>>>      {
>>>>        dp = &dis386[*ins.codep];
>>>>        ins.need_modrm = onebyte_has_modrm[*ins.codep];
>>>>      }
>>>>
>>>> It will use the very first byte of the bytecode.
>>>>
>>>> On the other hand, in gdb, let's take VEX prefix as example at
>>>> gdb/amd64-tdep.c L1349, the logic is:
>>>>
>>>>    /* Skip REX/VEX instruction encoding prefixes.  */
>>>>    ...
>>>>    else if (vex2_prefix_p (*insn))
>>>>      {
>>>>        details->enc_prefix_offset = insn - start;
>>>>        insn += 2;
>>>>      }
>>>>    else if (vex3_prefix_p (*insn))
>>>>      {
>>>>        details->enc_prefix_offset = insn - start;
>>>>        insn += 3;
>>>>      }
>>>>    ...
>>>>    if (prefix != nullptr && rex2_prefix_p (*prefix))
>>>>      {
>>>>        ...
>>>>      }
>>>>    else if (prefix != nullptr && vex2_prefix_p (*prefix))
>>>>      {
>>>>        need_modrm = twobyte_has_modrm[*insn];
>>>>        details->opcode_len = 2;
>>>>      }
>>>>    else if (prefix != nullptr && vex3_prefix_p (*prefix))
>>>>      {
>>>>        need_modrm = twobyte_has_modrm[*insn];
>>>>        ...
>>>> }
>>>> ...
>>>>
>>>> It will skip the VEX prefix and use twobyte_has_modrm table instead of
>>>> onebyte_has_modrm[0xc4/c5] in disassembler. The table usage are totally
>>>> different although the table itself is the same. It will cause the need_modrm
>>>> value different eventually. For example, opcode for VPBLENDW under 128 bit
>>>> is "VEX.128.66.0F3A.WIG 0E /r ib". The need_modrm would be false in gdb
>>>> since twobyte_has_modrm[0x0e] is false.
>>>>
>>>> Does anyone know the reason on that? It is weird to me.
>>>
>>> Same here; see https://sourceware.org/pipermail/gdb-patches/2019-February/155347.html.
>>> That patch might require re-basing and some work to be up-to-date again, but
>>> fundamentally it still looks applicable. I don't really understand why stuff
>>> like this isn't allowed in. Pedro's desire for a testcase is understandable,
>>> but shouldn't block such a patch (there was a 2nd one s well) for this many
>>> years.
>>
>> I didn't realise a patch was rotting for this. There's Alexander's
>> PR28999 (and a few other either dupes or very-related bugs) too.
>>
>> While I can understand wanting a testcase, tdep is really in a sorry
>> state for x86 anyway, and this clearly makes it better. Perhaps with
>> Haochen's interest, we can finally get it in. But I don't see any
>> specific x86 maintainers for gdb.
> 
> I hope the bug is fixed by a more comprehensive patchset from Tom, which has
> already landed:
> https://inbox.sourceware.org/gdb-patches/e5282a4b-5d9f-4891-b9b8-45ded54ec6ee@suse.de/
> 
> Haochen's question still stands, I guess.

Hi Alexander,

thanks for cc-ing me on this thread.

Indeed the PR you filed has been fixed.

I grepped a bit in the gas testsuite for VPBLENDW and constructed a gdb 
unit test that passes with workaround but fails without:
...
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index d5ea4aff4cf..251e6932c64 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -3755,6 +3755,32 @@ test_amd64_get_insn_details (void)
      = { 0x62, 0xf1, 0x7c, 0x48, 0x28, 0x81, 0x00, 0xfc, 0xff, 0xff };
    fixup_riprel (details, insn.data (), ECX_REG_NUM);
    SELF_CHECK (insn == updated_insn);
+
+  /* INSN: vpblendw $0x7,%xmm4,%xmm6,%xmm2, vex3 prefix.  */
+  insn = { 0xc4, 0xe3, 0x49, 0x0e, 0xd4, 0x07 };
+  amd64_get_insn_details (insn.data (), &details);
+  SELF_CHECK (details.opcode_len == 3);
+  SELF_CHECK (details.enc_prefix_offset == 0);
+  SELF_CHECK (details.opcode_offset == 3);
+  SELF_CHECK (details.modrm_offset == -1);
+
+  /* INSN: vpblendw $0x7,0xff(%rip),%ymm6,%ymm2.  */
+  insn = { 0xc4, 0xe3, 0x4d, 0x0e, 0x15, 0xff, 0x00, 0x00, 0x00, 0x07 };
+  amd64_get_insn_details (insn.data (), &details);
+
+  if (1 && details.modrm_offset == -1)
+    {
+      /* Workaround.  */
+      details.modrm_offset = 4;
+    }
+
+  SELF_CHECK (details.modrm_offset != -1);
+
+  /* INSN: vpblendw $0x7,0xff(%ecx),%ymm6,%ymm2.  */
+  fixup_riprel (details, insn.data (), ECX_REG_NUM);
+  updated_insn
+    = { 0xc4, 0xe3, 0x4d, 0x0e, 0x91, 0xff, 0x00, 0x00, 0x00, 0x07 };
+  SELF_CHECK (insn == updated_insn);
  }

  static void
...
because of an incorrect modrm_offset value.

I'll try to fix this.

Thanks,
- Tom