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

[Jan Beulich <jbeulich@suse.com>]

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].

Jan

> if we can get this information in advance, we can use dis386[] to report bad, in the current case, only when ins.codep++ and fetch code return error, then we can know 0f is the last byte, we should use dis386[] for it, but it has returned. This is what I'm confused about.
> 
> Lili.