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

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

> >    /* Bits of REX we've already used.  */
> >    uint8_t rex_used;
> >
> > +  /* REX2 prefix for the current instruction use gpr32(r16-r31). */
> > + unsigned char rex2;
> > +  /* Bits of REX2 we've already used.  */  unsigned char rex2_used;
> 
> Since these two fields don't hold the REX2 bits directly, the comment(s)
> want(s) saying what they contain.
> 
> > +  unsigned char rex2_payload;
> 
> I don't see this field used in more than one function. In which case it can be a
> local variable there (in the innermost scope covering all uses).
> 
Yes, it should be local variable. Changed it.

> > @@ -367,6 +381,7 @@ fetch_error (const instr_info *ins)
> >  #define PREFIX_IGNORED_DATA	(PREFIX_DATA <<
> PREFIX_IGNORED_SHIFT)
> >  #define PREFIX_IGNORED_ADDR	(PREFIX_ADDR <<
> PREFIX_IGNORED_SHIFT)
> >  #define PREFIX_IGNORED_LOCK	(PREFIX_LOCK <<
> PREFIX_IGNORED_SHIFT)
> > +#define PREFIX_IGNORED_REX2	(PREFIX_REX2 <<
> PREFIX_IGNORED_SHIFT)
> 
> I don't think "ignored" is what you mean, considering ...
> 
> >      /* MOD_0FAE_REG_5 */
> > -    { "xrstor",		{ FXSAVE }, PREFIX_OPCODE },
> > +    { "xrstor",		{ FXSAVE }, PREFIX_OPCODE |
> PREFIX_IGNORED_REX2 },
> >      { PREFIX_TABLE (PREFIX_0FAE_REG_5_MOD_3) },
> >    },
> 
> ... these uses here.
> 

Replaced " PREFIX_IGNORED_REX2" with "ILLEGAL_PREFIX_REX2 ".

> >      case USE_EVEX_TABLE:
> > +      if (ins->last_rex2_prefix >= 0)
> > +	return &bad_opcode;
> >        ins->two_source_ops = false;
> >        /* EVEX prefix.  */
> >        ins->vex.evex = true;
> 
> There aren't similar REX checks here, yet both should be handled as similarly
> as possible.

Done.

> 
> > @@ -9292,13 +9344,17 @@ print_insn (bfd_vma pc, disassemble_info
> *info, int intel_syntax)
> >        goto out;
> >      }
> >
> > -  if (*ins.codep == 0x0f)
> > +  /* M0 in rex2 prefix represents map0 or map1.  */  if (*ins.codep
> > + == 0x0f || (ins.rex2 & 0x8))
> 
> Please can literals like the 0x8 here gain meaningful names (REX2_M in this
> case)?
> 

Done.

> > @@ -9468,6 +9532,7 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        ins.used_prefixes |= PREFIX_DATA;
> >        /* Fall through.  */
> >      case PREFIX_OPCODE:
> > +    case PREFIX_OPCODE | PREFIX_IGNORED_REX2:
> 
> You may rather want to mask off the high part in the switch() expression itself.

Tried it , found that 'Case PREFIX_IGNORED' uses the high part of this switch. 

#define PREFIX_IGNORED          (PREFIX_IGNORED_REPZ \
                                 | PREFIX_IGNORED_REPNZ \
                                 | PREFIX_IGNORED_DATA)

#define PREFIX_IGNORED_REPZ     (PREFIX_REPZ << PREFIX_IGNORED_SHIFT)
#define PREFIX_IGNORED_REPNZ    (PREFIX_REPNZ << PREFIX_IGNORED_SHIFT)
#define PREFIX_IGNORED_DATA     (PREFIX_DATA << PREFIX_IGNORED_SHIFT)

> > @@ -9510,9 +9575,17 @@ print_insn (bfd_vma pc, disassemble_info *info,
> > int intel_syntax)
> >
> >    /* Check if the REX prefix is used.  */
> >    if ((ins.rex ^ ins.rex_used) == 0
> > -      && !ins.need_vex && ins.last_rex_prefix >= 0)
> > +      && !ins.need_vex && ins.last_rex_prefix >= 0
> > +      && ins.last_rex2_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))
> > +	  || dp == &bad_opcode))
> > +    ins.all_prefixes[ins.last_rex2_prefix] = 0;
> 
> I'm again puzzled by the dissimilarity with the REX handling. Furthermore,
> with the way you split the REX2 payload byte, I don't think the combination of
> REX and REX2 handling above will result in correct output in all cases.
> (There shouldn't be mention of an unused REX bit when REX2 was in use on
> an insn.)

Done.

> > @@ -11307,6 +11386,7 @@ static bool
> >  OP_E_memory (instr_info *ins, int bytemode, int sizeflag)  {
> >    int add = (ins->rex & REX_B) ? 8 : 0;
> > +  add += (ins->rex2 & REX_B) ? 16 : 0;
> >    int riprel = 0;
> >    int shift;
> 
> While generally mixing declaration and statements is okay nowadays, putting
> a statement in the middle of a block's initial declarations would imo still
> better be avoided.

Done.

Moved this comment from patch2/8 to here.
      if (modifiers[Vex].value
	  || (space > SPACE_0F
	      && space != SPACE_EVEXMAP4
	      && !modifiers[EVex].value
	      && !modifiers[Disp8MemShift].value
	      && !modifiers[Broadcast].value
	      && !modifiers[Masking].value
	      && !modifiers[SAE].value))
       }
	modifiers[No_egpr].value = 1;
      }

>And then - shouldn't at least part of this already be put in place in patch 1?
>Finally, to avoid the split between where this attribute gets set, wouldn't it be possible to also handle the XSAVE/XRSTOR variants here rather than directly in the opcode table?

I put part of this in patch 1, and add a new function to handle NoEgpr (the parameter's number of  process_i386_opcode_modifier is a bit too much, so I added a new function).
I merged partII patch2/6 into this patch. listed it with 3.

We handle the following in function if_entry_needs_special_handle():
1.  Prefixing XSAVE* and XRSTOR* instructions with REX2 triggers #UD.
2.  Disable Egpr for 3dnow and 3dnowA.
3.  All opcodes listed map0 0x4*, 0x7*, 0xa* and map0 0x3*, 0x8* are reserved under REX2 and triggers #UD when prefixed with REX2

Finally, thank you for checking these large patches so carefully. The next few days are China's national holidays, which last about 8 days, my feedback time will be longer. Thank you.

Lili.

> Jan