Re: [PATCH] [gdb/tdep] Add XOP support in amd64_get_insn_details

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

On 8/26/25 15:20, Gerlicher, Klaus wrote:
> Hi Tom,
> 
> Thanks for working on this, I was tempted to also add it also but since this is only supported on 10 year old AMD CPUs, so I didn't bother.
> 

Hi Klaus,

thanks for the review.

FWIW, I don't know how relevant this patch is, but new gdbs are used on 
both older and recent architectures, so it could be relevant for someone.

> One inline comment/question. Seems fine to me.
> 
> Reviewed-By: Klaus Gerlicher<klaus.gerlicher.@intel.com>
> 
> Thanks
> Klaus
> 
>> -----Original Message-----
>> From: Tom de Vries <tdevries@suse.de>
>> Sent: Tuesday, August 26, 2025 10:59 AM
>> To: gdb-patches@sourceware.org
>> Cc: Gerlicher, Klaus <klaus.gerlicher@intel.com>
>> Subject: [PATCH] [gdb/tdep] Add XOP support in amd64_get_insn_details
>>
>> Implement support for XOP instructions [1] in amd64_get_insn_details.
>>
>> The encoding scheme is documented here [2].  Essentially it's a variant of the
>> VEX3 encoding scheme, with:
>> - 0x8f as the first byte instead of 0xc4, and
>> - with an opcode map >= 8.
>>
>> The changes are roughly the same as the XOP part of an earlier submission [3],
>> hence the tag.
>>
>> The only real difference is that that patch proposed to implement xop_prefix_p
>> using:
>> ...
>>    return pfx[0] == 0x8f && (pfx[1] & 0x38);
>> ...
>> which is incorrect because the 0x38 mask selects bits bit 3-5, and bit 5 is
>> not part of the opcode map in bits 0-4.
>>

After reading this ( 
https://en.wikipedia.org/wiki/XOP_instruction_set#cite_note-5 ) I 
realized what this is trying to do: detect that it's not a POP.  I've 
updated the commit message accordingly.

>> Instead, use:
>> ...
>>    gdb_byte m = pfx[1] & 0x1f;
>>    return pfx[0] == 0x8f && m >= 8;
> 
> AMD SDM says
> 
> 0x8 8 bit immediate
> 0x9  no immediate
> 0xa 32-bit immediate.
> 
> Did you see any other maps somewhere? Or is this just to distinguish from POP?

I haven't looked for that, I've only looked at the specification of the 
prefix.  I'm fine with being "forward compatible" with any bitcode map 
with m >= 8.

The test m >=8 is part of the XOP specification:
...
Prefix Byte 0
Byte 0 of the XOP prefix is set to 8Fh. This signifies an XOP prefix 
only in conjunction with the mmmmm field of the following byte being 
greater than or equal to 8; if the mmmmm field is less than
8 then these two bytes are a form of the POP instruction rather than an 
XOP prefix.
...

I've pushed this.

Thanks,
- Tom

> 
>> ...
>>
>> Tested on x86_64-linux.
>>
>> Co-Authored-By: Jan Beulich <jbeulich@suse.com>
>>
>> [1] https://en.wikipedia.org/wiki/XOP_instruction_set
>> [2] https://www.amd.com/content/dam/amd/en/documents/archived-tech-
>> docs/programmer-references/43479.pdf
>> [3] https://sourceware.org/pipermail/gdb-patches/2019-
>> February/155347.html
>> ---
>>   gdb/amd64-tdep.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index d5ea4aff4cf..d20f490a0f4 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -1181,6 +1181,15 @@ vex3_prefix_p (gdb_byte pfx)
>>     return pfx == 0xc4;
>>   }
>>
>> +/* True if PFX is the start of an XOP prefix.  */
>> +
>> +static bool
>> +xop_prefix_p (const gdb_byte *pfx)
>> +{
>> +  gdb_byte m = pfx[1] & 0x1f;
>> +  return pfx[0] == 0x8f && m >= 8;
>> +}
>> +
>>   /* Return true if PFX is the start of the 4-byte EVEX prefix.  */
>>
>>   static bool
>> @@ -1351,7 +1360,7 @@ amd64_get_insn_details (gdb_byte *insn, struct
>> amd64_insn *details)
>>         details->enc_prefix_offset = insn - start;
>>         insn += 2;
>>       }
>> -  else if (vex3_prefix_p (*insn))
>> +  else if (vex3_prefix_p (*insn) || xop_prefix_p (insn))
>>       {
>>         details->enc_prefix_offset = insn - start;
>>         insn += 3;
>> @@ -1438,6 +1447,11 @@ amd64_get_insn_details (gdb_byte *insn, struct
>> amd64_insn *details)
>>   	  return;
>>   	}
>>       }
>> +  else if (prefix != nullptr && xop_prefix_p (prefix))
>> +    {
>> +      details->opcode_len = 1;
>> +      need_modrm = 1;
>> +    }
>>     else if (*insn == TWO_BYTE_OPCODE_ESCAPE)
>>       {
>>         /* Two or three-byte opcode.  */
>> @@ -1508,7 +1522,7 @@ fixup_riprel (const struct amd64_insn &details,
>> gdb_byte *insn,
>>   	{
>>   	  /* VEX.!B is set implicitly.  */
>>   	}
>> -      else if (vex3_prefix_p (pfx[0]))
>> +      else if (vex3_prefix_p (pfx[0]) || xop_prefix_p (pfx))
>>   	pfx[1] |= VEX3_NOT_B;
>>         else if (evex_prefix_p (pfx[0]))
>>   	{
>> @@ -3755,6 +3769,20 @@ 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: vpcomtrueuq 0x0(%rip),%xmm7,%xmm0, xop prefix.  */
>> +  insn = { 0x8f, 0xe8, 0x40, 0xef, 0x05, 0x00, 0x00, 0x00, 0x00, 0x07 };
>> +  amd64_get_insn_details (insn.data (), &details);
>> +  SELF_CHECK (details.opcode_len == 1);
>> +  SELF_CHECK (details.enc_prefix_offset == 0);
>> +  SELF_CHECK (details.opcode_offset == 3);
>> +  SELF_CHECK (details.modrm_offset == 4);
>> +
>> +  /* INSN: vpcomtrueuq 0x0(%ecx),%xmm7,%xmm0, xop prefix.  */
>> +  fixup_riprel (details, insn.data (), ECX_REG_NUM);
>> +  updated_insn
>> +    = { 0x8f, 0xe8, 0x40, 0xef, 0x81, 0x00, 0x00, 0x00, 0x00, 0x07 };
>> +  SELF_CHECK (insn == updated_insn);
>>   }
>>
>>   static void
>>
>> base-commit: 5319c8dec64aa5e37c56da2b0cfe77a1886231ca
>> --
>> 2.43.0
>>
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>