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

["Cui, Lili" <lili.cui@intel.com>]

> -----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))

For d5 00 0f c0
0000000000000000 <_start>:
   0:   d5 00 0f                {rex2} (bad)
   3:   c0                      .byte 0xc0

Thanks,
Lili.

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