[PATCH 1/8] Support APX GPR32 with rex2 prefix

[Jan Beulich]

On 09.11.2023 14:27, Cui, Lili wrote:
>>>> Also is this, ...
>>>>
>>>>>      {
>>>>>        unsigned char threebyte;
>>>>>
>>>>> -      ins.codep++;
>>>>> -      if (!fetch_code (info, ins.codep + 1))
>>>>> -	goto fetch_error_out;
>>>>> +      if (!ins.rex2)
>>>>> +	{
>>>>> +	  ins.codep++;
>>>>> +	  if (!fetch_code (info, ins.codep + 1))
>>>>> +	    goto fetch_error_out;
>>>>> +	}
>>>>>        threebyte = *ins.codep;
>>>>>        dp = &dis386_twobyte[threebyte];
>>>>>        ins.need_modrm = twobyte_has_modrm[threebyte];
>>>>
>>>> ... all the way to here, really correct for d5 00 0f?
>>>>
>>>
>>> I think the 0f here must indicate that it is the first byte of the legacy map1
>> instruction, meaning legacy map0 does not have 0f opcode. If this instruction
>> has a rex2 prefix, rex2.w must be 1 and should be d5 80. If a bad binary does
>> appear, our original code also has the same issue.
>>>
>>> static const struct dis386 dis386[] = { ...
>>> / * 0f  */
>>> { Bad_Opcode },       /* 0x0f extended opcode escape */
>>
>> No, this entry simply will never be used, because of how decoding is done.
>> My comment was about what's going to happen if you encounter the d5 00 0f
>> byte sequence. That's _not_ an indication to use map1 for decoding, nor to
>> read another opcode byte. In this case the table entry you quote above will
>> need to come into play, not any entry from dis386_twobyte[]. (As long as both
>> are Bad_Opcode the difference may not even be noticeable, but it would be a
>> latent trap for someone to fall into down the road.)
>>
> 
> 
>   /* REX2.M in rex2 prefix represents map0 or map1.  */
>   if (*ins.codep == 0x0f || (ins.rex2 & REX2_M))
>     {
>       unsigned char threebyte;
> 
>       if (!ins.rex2)
>         {
>           ins.codep++;
>           if (!fetch_code (info, ins.codep + 1))
>             goto fetch_error_out;                                                      ---> When there are no bytes after 0f, it will jump to fetch error, but no error will be reported.
>         }
>       threebyte = *ins.codep;
>       dp = &dis386_twobyte[threebyte];
>       ins.need_modrm = twobyte_has_modrm[threebyte];
>       ins.codep++;
>     }
> 
> For d5 00 0f
> Decode to:
>    0:   d5                      rex2
>    1:   00 0f                   add    %cl,(%rdi)

But this would better have d5 00 0f all on the first line (it
definitely needs to have d5 00 on the same line, as the bytes belong
together), as opposed to ...

> For 40 0f
> Decode to:
>    0:   40                      rex
>    1:   0f                      .byte 0xf

... this where there truly is a known missing byte before we could
proceed further. (It's still a little questionable to print REX
separately in this case, but that's the way the binutils disassembler
has always worked.)

Yet to restate - to see what I mean, you'd need to populate at least
one of the two 0f slots in the mentioned arrays. What I'm suspecting
from the code as this patch version has it is that d5 00 0f would
wrongly descend into dis386_twobyte[]. Yet you can tell that from
it correctly using dis386[] only if the two 0f slots of these arrays
are meaningfully different (or by actually looking at things in e.g.
a debugger).

>>>>> @@ -9513,6 +9572,13 @@ print_insn (bfd_vma pc, disassemble_info
>>>>> *info,
>>>> int intel_syntax)
>>>>>        && !ins.need_vex && ins.last_rex_prefix >= 0)
>>>>>      ins.all_prefixes[ins.last_rex_prefix] = 0;
>>>>>
>>>>> +  /* Check if the REX2 prefix is used.  */
>>>>> +  if (ins.last_rex2_prefix >= 0
>>>>> +      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
>>>>> +	   && (ins.rex2 & 0x7))
>>>>
>>>> DYM ((ins.rex2 & 7) & ~(ins.rex2_used & 7)) != 0
>>>>
>>>
>>> Here's an example of a negative scenario, when ins.rex2 == 1 and
>> ins.rex2_used == 1, we want to clear last_rex2_prefix, because it has egpr and
>> we don't want to add {rex2} to it.
>>
>> Well, that would be dealt with as well by the simpler code I suggested,
>> wouldn't it?
>>
> 
> No, for d510 ,  ((ins.rex2 & 7) & ~(ins.rex2_used & 7)) == 0. Anyway, I want to delete them. I don't see any point in it at all.

Hmm, I guess I'm confused. How would you present unconsumed REX2.{R,X,B}{3,4}
then?

>>>>> @@ -11086,8 +11155,11 @@ print_register (instr_info *ins, unsigned
>>>>> int
>>>> reg, unsigned int rexmask,
>>>>>      ins->illegal_masking = true;
>>>>>
>>>>>    USED_REX (rexmask);
>>>>> +  USED_REX2 (rexmask);
>>>>
>>>> Do both really need tracking separately? Whatever consumes REX.B will
>>>> also consume REX2.B4, an so on.
>>>>
>>> I was confused here, I think we only need to print {rex2} for the upper 4 bits
>> == *000, which means egpr is not used and we need to use {rex2} to
>> distinguish it from legacy encoding.  maybe we don’t need ((ins.rex2 & 0x7) ^
>> (ins.rex2_used & 0x7)) == 0, and nor USED_REX2 (rexmask). I intend to delete
>> them.
>>>
>>> +  /* Check if the REX2 prefix is used.  */
>>> +  if (ins.last_rex2_prefix >= 0
>>> +      && ((((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0
>>> +	   && (ins.rex2 & 0x7))
>>
>> But that's the same you had before. I'm afraid I don't see what you're trying
>> to tell me.
>>
> After removing  " ((ins.rex2 & 0x7) ^ (ins.rex2_used & 0x7)) == 0 ", the code changes to 
> 
>   +  /* Check if the REX2 prefix is used.  */
>   +  if (ins.last_rex2_prefix >= 0 && (ins.rex2 & 0x7))  
> 
> When it is true, decode will not print the {rex2} for this insn.

Yet ins.rex2 having any of the low 3 bits set says nothing about whether
every one of these was consumed while processing operands / suffixes.
You need to consult .rex{,2}_used; my earlier point was merely that you
don't need a separate .rex2_used; the bits in .rex_used are all you
require to get this right (as a consumer of, say, REX.X / REX2.X3 is
also a consumer of REX2.X4; leaving aside EVEX encoded insns for the
moment).

Jan