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

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

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

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


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

> >> @@
> >>> print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
> >>>  	if (name == NULL)
> >>>  	  abort ();
> >>>  	prefix_length += strlen (name) + 1;
> >>> -	i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
> >>> +	if (ins.all_prefixes[i] == REX2_OPCODE)
> >>> +	  i386_dis_printf (info, dis_style_mnemonic, "{%s} ", name);
> >>
> >> Do braces really count as part of the mnemonic?
> >
> > Yes, rex2 prefix prefers to use mnemonic {rex2}, unlike rex prefix use rex,
> rex.B....
> 
> Maybe you didn't understand what I mean: My comment was regarding the
> use of dis_style_mnemonic for the entire {rex2} (rather than perhaps just for
> what's inside the figure braces).
> 
> >>> @@ -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.

Lili.