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

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

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

> >>>>> @@ -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?
> 

For rex2.R4X4B4. I can't imagine why they set RXB but didn't consume it at the end. Since rex2 has no content listed, so I think it is not useful to have rex2_used.
For rex2.WR3X3B3, I think it can be ignored in rex2, which doesn't list what's there.

We currently use the following rules to disambiguate.

When the instruction has egpr, it means it has rex2 prefix and we don't print {rex2} for it. When it also has an evex version, we add {evex} to the evex instruction.
When the instruction has no egpr, we need to print {rex2} for it. In case of evex instruction, we need to print {evex} for it.

            adc     $0x7b,%r8b
{rex2} adc     $0x7b,%r8b
            adc     $0x7b,%r16b
{evex} adc     $0x7b,%r16b
{evex} adc     $0x7b,%r8b

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

Here are some cases:
0:   41 0f a8                  rex.B push %gs         ---> rex.B was not consumed, rex will print it.
0:   d5 01 0f a8             {rex2} push %gs      ----> Without egpr, we need to print {rex2} for it. But we can't see anything about REX2.B3 from the prefix.
4:   d5 19 58                  pop    %r24             ----> There is egpr, we know it uses rex2 prefix. We cannot see the information of the lower 4 bits of rex2

It is not helpful to judge rex_used in rex2.

Thanks.
Lili.