This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gas/x86-64: properly distinguish low and high register ranges


>>> On 24.07.12 at 16:16, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> Can you add some testcases?

I knew you would ask this, but sorry, this makes no sense - if test
cases would are desirable here, they shouldn't be testing just the
things that this patch fixes, but also any other invalid operand
combinations. As an example - why would testing that "xlat [r11]"
isn't accepted be needed, but not e.g. "xlat [ecx]"?

Furthermore, this fixes actually broken behavior, so accepting
the change shouldn't be dependent upon test case availability.

Jan

> On Jul 24, 2012 6:14 AM, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> There were several cases where the registers in the REX encoded range
>> got treated identically to the ones in the base range, due to not
>> paying attention to the fact that reg_entry's reg_num field doesn't
>> fully specify the register number (reg_flags also needs to be checked
>> for RegRex). This patch introduces and uses a new (inline) function to
>> obtain the full register number, and uses it to fix all those cases.
>>
>> 2012-07-24  Jan Beulich <jbeulich@suse.com>
>>
>>         * config/tc-i386.c (register_number): New function.
>>         (build_vex_prefix, process_immext, process_operands,
>>         build_modrm_byte, i386_index_check): Use it.
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -1758,6 +1758,17 @@ operand_type_register_match (i386_operan
>>  }
>>
>>  static INLINE unsigned int
>> +register_number (const reg_entry *r)
>> +{
>> +  unsigned int nr = r->reg_num;
>> +
>> +  if (r->reg_flags & RegRex)
>> +    nr += 8;
>> +
>> +  return nr;
>> +}
>> +
>> +static INLINE unsigned int
>>  mode_from_disp_size (i386_operand_type t)
>>  {
>>    if (t.bitfield.disp8)
>> @@ -2830,12 +2841,7 @@ build_vex_prefix (const insn_template *t
>>
>>    /* Check register specifier.  */
>>    if (i.vex.register_specifier)
>> -    {
>> -      register_specifier = i.vex.register_specifier->reg_num;
>> -      if ((i.vex.register_specifier->reg_flags & RegRex))
>> -       register_specifier += 8;
>> -      register_specifier = ~register_specifier & 0xf;
>> -    }
>> +    register_specifier = ~register_number (i.vex.register_specifier) &
>> 0xf;
>>    else
>>      register_specifier = 0xf;
>>
>> @@ -2976,14 +2982,13 @@ process_immext (void)
>>
>>    if (i.tm.cpu_flags.bitfield.cpusse3 && i.operands > 0)
>>      {
>> -      /* SSE3 Instructions have the fixed operands with an opcode
>> -        suffix which is coded in the same place as an 8-bit immediate
>> -        field would be.  Here we check those operands and remove them
>> -        afterwards.  */
>> +      /* MONITOR/MWAIT have fixed operands with an opcode suffix which
>> +        is coded in the same place as an 8-bit immediate field would be.
>> +        Here we check those operands and remove them afterwards.  */
>>        unsigned int x;
>>
>>        for (x = 0; x < i.operands; x++)
>> -       if (i.op[x].regs->reg_num != x)
>> +       if (register_number (i.op[x].regs) != x)
>>           as_bad (_("can't use register '%s%s' as operand %d in '%s'."),
>>                   register_prefix, i.op[x].regs->reg_name, x + 1,
>>                   i.tm.name);
>> @@ -5075,7 +5080,7 @@ process_operands (void)
>>         {
>>           /* The first operand is implicit and must be xmm0.  */
>>           gas_assert (operand_type_equal (&i.types[0], &regxmm));
>> -         if (i.op[0].regs->reg_num != 0)
>> +         if (register_number (i.op[0].regs) != 0)
>>             return bad_implicit_operand (1);
>>
>>           if (i.tm.opcode_modifier.vexsources == VEX3SOURCES)
>> @@ -5149,7 +5154,7 @@ duplicate:
>>        gas_assert (i.reg_operands
>>                   && (operand_type_equal (&i.types[0], &regxmm)
>>                       || operand_type_equal (&i.types[0], &regymm)));
>> -      if (i.op[0].regs->reg_num != 0)
>> +      if (register_number (i.op[0].regs) != 0)
>>         return bad_implicit_operand (i.types[0].bitfield.regxmm);
>>
>>        for (j = 1; j < i.operands; j++)
>> @@ -5352,10 +5357,7 @@ build_modrm_byte (void)
>>                        || operand_type_equal
>> (&i.tm.operand_types[reg_slot],
>>                                               &regymm));
>>            exp->X_op = O_constant;
>> -          exp->X_add_number
>> -              = ((i.op[reg_slot].regs->reg_num
>> -                  + ((i.op[reg_slot].regs->reg_flags & RegRex) ? 8 : 0))
>> -                << 4);
>> +          exp->X_add_number = register_number (i.op[reg_slot].regs) << 4;
>>          }
>>        else
>>          {
>> @@ -5399,9 +5401,7 @@ build_modrm_byte (void)
>>                       || operand_type_equal (&i.tm.operand_types[reg_slot],
>>                                              &regymm));
>>            i.op[imm_slot].imms->X_add_number
>> -              |= ((i.op[reg_slot].regs->reg_num
>> -                   + ((i.op[reg_slot].regs->reg_flags & RegRex) ? 8 : 0))
>> -                 << 4);
>> +              |= register_number (i.op[reg_slot].regs) << 4;
>>          }
>>
>>        gas_assert (operand_type_equal (&i.tm.operand_types[nds], &regxmm)
>> @@ -7314,7 +7314,7 @@ i386_index_check (const char *operand_st
>>                    ? i.base_reg->reg_type.bitfield.reg32
>>                    : i.base_reg->reg_type.bitfield.reg16))
>>         ok = 0;
>> -      else if (i.base_reg->reg_num != expected)
>> +      else if (register_number (i.base_reg) != expected)
>>         ok = -1;
>>
>>        if (ok < 0)
>> @@ -7329,7 +7329,7 @@ i386_index_check (const char *operand_st
>>                  : (flag_code == CODE_16BIT) ^ !i.prefix[ADDR_PREFIX]
>>                    ? i386_regtab[j].reg_type.bitfield.reg32
>>                    : i386_regtab[j].reg_type.bitfield.reg16)
>> -               && i386_regtab[j].reg_num == expected)
>> +               && register_number(i386_regtab + j) == expected)
>>               break;
>>           gas_assert (j < i386_regtab_size);
>>           as_warn (_("`%s' is not valid here (expected `%c%s%s%c')"),
>>
>>
>>



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]