[PATCH v3] x86: Add tls check in gas

[Cui, Lili]

> > +    case BFD_RELOC_X86_64_GOTPC32_TLSDESC:
> > +      /* Check GOTPC32 TLSDESC access model:
> > +
> > +	 --- LP64 mode ---
> > +	 leaq x@tlsdesc(%rip), %reg64 --> Memory reg must be %rip.
> > +
> > +	 --- X32 mode ---
> > +	 rex/rex2 leal x@tlsdesc(%rip), %reg32 --> Memory reg must be %rip.
> > +
> > +	 In X32 mode, when dest is Egpr, gas will automatically add rex2 to it.
> > +	 When dest is Gpr, we need to check whether there is a rex/rex2 prefix.
> > +       */
> > +      if (i.tm.mnem_off != MN_lea)
> > +	return x86_tls_error_insn;
> > +      if (!i.base_reg)
> > +	return x86_tls_error_no_base_reg;
> > +      if (i.base_reg->reg_num != RegIP)
> > +	return x86_tls_error_rip;
> 
> Just to double check: %eip as "memory reg" is okay?

If it is %eip, the instruction length will change. Looking at the ld code, it should be %rip
Added size check for memory reg.

> 
> > +      if (i.op[1].regs->reg_type.bitfield.dword
> > +	       && !(i.op[1].regs->reg_flags & RegRex2)
> 
> Nit: Indentation looks to be wrong starting from here.

Done.

> 
> > +	       && (pp.rex2_encoding == false
> > +		   && pp.rex_encoding == false
> > +		   && i.prefix[REX_PREFIX] != REX_OPCODE))
> 
> Why is this last set separately parenthesized? And as mentionbed before:
> Please don't ever compare booleans against true or false. Use booleans as
> booleans; that's what they are for.
> 

Done.

> Where's the operand size check for both cases, yielding
> x86_tls_error_dest_reg_size when it fails?

Added.

> > +    case BFD_RELOC_386_TLS_IE_32:
> > +      /* Check IE_32 access model:
> > +
> > +	 subl foo@gottpoff(%reg1), %reg2
> > +	 movl foo@gottpoff(%reg1), %reg2
> > +	 addl foo@gottpoff(%reg1), %reg2
> > +
> > +	 Memory operand: SIB is not supported.
> > +       */
> > +      if (i.tm.mnem_off != MN_sub
> > +	  && i.tm.mnem_off != MN_add
> > +	  && i.tm.mnem_off != MN_mov)
> > +	return x86_tls_error_insn;
> 
> If I'm not mistaken my prior question here wasn't answered: All forms of these
> are okay? Without checking, how do you even know ...
> 
> > +      if (!i.base_reg)
> > +	return x86_tls_error_no_base_reg;
> > +      if (i.index_reg)
> > +	return x86_tls_error_sib;
> > +      if (!i.op[1].regs->reg_type.bitfield.dword)
> > +	return x86_tls_error_dest_reg_size;
> 
> ... that i.op[1].regs is valid (i.e. that operand is a register)? Just to take an
> example.

I remember you mentioned this issue before, I can't remember why I missed it later. It's solved now.

> 
> > +      break;
> > +
> > +    case BFD_RELOC_386_TLS_IE:
> > +      /* Check IE access model:
> > +
> > +	 movl foo@indntpoff, %reg --> Mod == 00 && r/m == 5
> > +	 addl foo@indntpoff, %reg --> Mod == 00 && r/m == 5
> > +       */
> > +      if (i.tm.mnem_off != MN_add && i.tm.mnem_off != MN_mov)
> > +	return x86_tls_error_insn;
> > +      if (i.base_reg || i.index_reg)
> > +	return x86_tls_error_require_no_base_index_reg;
> > +      if (!i.op[1].regs->reg_type.bitfield.dword)
> > +	return x86_tls_error_dest_reg_size;
> 
> Same issue here.

Done.

> 
> > +      break;
> > +
> > +    case BFD_RELOC_X86_64_GOTTPOFF:
> > +      /* Check GOTTPOFF access model:
> > +
> > +	 mov foo@gottpoff(%rip), %reg --> Memory Reg must be %rip.
> > +	 add foo@gottpoff(%rip), %reg --> Memory Reg must be %rip.
> > +       */
> > +      if (i.tm.mnem_off != MN_add && i.tm.mnem_off != MN_mov)
> > +	return x86_tls_error_insn;
> > +      if (!i.base_reg)
> > +	return x86_tls_error_no_base_reg;
> > +      if (i.base_reg->reg_num != RegIP)
> > +	return x86_tls_error_rip;
> 
> And similarly here, even if in this case you don't de-reference a pointer that may
> not point to where you want it.
> 

Done.

> > +      break;
> > +
> > +    case BFD_RELOC_386_TLS_DESC_CALL:
> > +      /* Check GDesc access model:
> > +
> > +	 call *x@tlscall(%eax) --> Memory reg must be %eax and
> > +				   SIB is not supported.
> > +       */
> > +    case BFD_RELOC_X86_64_TLSDESC_CALL:
> > +      /* Check GDesc access model:
> > +
> > +	 call *x@tlscall(%rax) <--- LP64 mode.
> > +	 call *x@tlscall(%eax) <--- X32 mode.
> > +
> > +	 Only these fixed formats are supported.
> > +       */
> > +      if (i.tm.mnem_off != MN_call)
> > +	return x86_tls_error_insn;
> > +      if (i.index_reg)
> > +	return x86_tls_error_sib;
> > +      if (!i.base_reg)
> > +	return x86_tls_error_no_base_reg;
> > +      if (i.base_reg->reg_type.bitfield.instance != Accum)
> > +	return x86_tls_error_RegA;
> 
> No address size check?

Reloc () checks that, I will add an invalid test case for it.

> 
> > +static void
> > +x86_report_tls_error (enum x86_tls_error_type tls_error, enum
> > +bfd_reloc_code_real r_type) {
> > +  unsigned int k;
> > +  for (k = 0; k < ARRAY_SIZE (gotrel); k++)
> > +    if (gotrel[k].rel[object_64bit] == r_type)
> > +      break;
> > +
> > +  switch (tls_error)
> > +    {
> > +    case x86_tls_error_insn:
> > +      as_bad (_("@%s operator cannot be used with `%s'"),
> > +	      gotrel[k].str, insn_name (&i.tm));
> > +      return;
> > +
> > +    case x86_tls_error_sib:
> > +      as_bad (_("@%s operator requires no SIB"), gotrel[k].str);
> > +      return;
> > +
> > +    case x86_tls_error_no_base_reg:
> > +      as_bad (_("@%s operator requires base register"), gotrel[k].str);
> > +      return;
> > +
> > +    case x86_tls_error_require_no_base_index_reg:
> > +      as_bad (_("@%s operator requires no base/index register"),
> > +	      gotrel[k].str);
> > +      return;
> > +
> > +    case x86_tls_error_base_reg_name:
> > +      as_bad (_("@%s operator requires no base register"), gotrel[k].str);
> > +      return;
> 
> What does "name" stand for in the enumerator's identifier?
> 

Removed.

> > +    case x86_tls_error_index_ebx:
> > +      as_bad (_("@%s operator requires `%sebx' as index register"),
> > +	      gotrel[k].str, register_prefix);
> > +      return;
> > +
> > +    case x86_tls_error_RegA:
> > +      as_bad (_("@%s operator requires `%seax/rax' as base register"),
> > +	      gotrel[k].str, register_prefix);
> > +      return;
> 
> This diag is okay only if indeed both registers are okay. I think that isn't (always)
> the case. And of course the % prefix needs to appear on every register name (as
> long as we're in prefix-enabled mode).
> 

Done.

Thanks,
Lili.

> Jan