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

[Jan Beulich <jbeulich@suse.com>]

On 10.11.2023 13:05, Cui, Lili wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, November 10, 2023 5:57 PM
>> To: Cui, Lili <lili.cui@intel.com>
>> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
>> binutils@sourceware.org
>> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
>>
>> On 10.11.2023 10:47, Cui, Lili wrote:
>>>> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
>>>>
>>>> On 10.11.2023 08:11, Cui, Lili wrote:
>>>>>> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
>>>>>>
>>>>>> 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).
>>>>>>
>>>>>
>>>>> I'm confused here, for d5 00 0f when it fetches the next byte after
>>>>> 0f it will
>>>> find there is no byte there and then go to fetch_error_out and then
>>>> it will return from print_insn and I don't have a chance to do
>>>> anything for it. It cannot reach dis386_twobyte[].
>>>>
>>>> But why would it even try to fetch the next byte? 0f already is the
>>>> major opcode byte in this case. Fetching more can only mean either
>>>> there's an entry in dis386[] specifying operands, or there's an
>>>> attempt to index dis386_twobyte[]. Since dis386[] has Bad_Opcode at
>>>> that slot, I conclude that what you say confirms my suspicion that
>>>> dis386_twobyte[] is (attempted to
>>>> be) used here.
>>>>
>>>
>>> I don't know how to identify that 0f is the last byte of the binary,
>>
>> That's entirely irrelevant here. I gave the byte sequence d5 00 0f just as the
>> minimal one required to make my point. My original concern equally applies
>> to e.g. d5 00 0f 01 00, which may not use dis386_twobyte[0x01].
>>
> Aha, I  got you. Changed the code to
> 
>    /* REX2.M in rex2 prefix represents map0 or map1.  */
> -  if (*ins.codep == 0x0f || (ins.rex2 & REX2_M))
> +  if ((*ins.codep == 0x0f && ins.last_rex2_prefix < 0) || (ins.rex2 & REX2_M))

Would you mind considering

  if (ins.last_rex2_prefix < 0 ? *ins.codep == 0x0f : (ins.rex2 & REX2_M))

as an alternative?

Jan