[PATCH v2] x86: Add tls check in gas

[Jan Beulich]

On 05.09.2024 10:57, Cui, Lili wrote:
> @@ -6318,6 +6338,284 @@ static INLINE bool may_need_pass2 (const insn_template *t)
>  	       && (t->base_opcode | 8) == 0x2c);
>  }
>  
> +static enum x86_tls_error_type
> +x86_check_tls_relocation (unsigned int r_type)
> +{
> +  switch (r_type)
> +    {
> +    case BFD_RELOC_386_TLS_GOTDESC:
> +      /* Check transition from GDesc access model:
> +
> +	 leal x@tlsdesc(%ebx), %reg --> Memory reg must be %ebx and
> +					SIB is not supported.
> +       */

What does "check transition" here mean? Aiui it's the linker doing the
transitions (if/when possible).

> +      if (i.tm.mnem_off != MN_lea)
> +	return x86_tls_error_insn;
> +      else if (i.index_reg)
> +	return x86_tls_error_sib;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_type.bitfield.instance != RegB)
> +	return x86_tls_error_RegB;

As mentioned on other occasions before: While I understand this is to a
fair degree a matter of style and hence personal preference, I view use
of "else if" in such situations as misleading. Ommiting the "else" also
leads to shorter code, i.e. (slightly) improved readability. (Note how
further down you sometimes omit the "else". Such inconsistency is imo
even worse than having the "else" everywhere.)

As to in particular the last check: 16-bit addressing using %bx is then
also okay?

> +      break;
> +
> +    case BFD_RELOC_386_TLS_GD:
> +      /* Check transition from GD access model:
> +
> +	 leal foo@tlsgd(,%ebx,1), %eax   --> Only this fixed format is supported.
> +	 leal foo@tlsgd(%reg), %eax      --> Dest reg must be '%eax'
> +					     Memory reg can't be %eax.
> +       */
> +      if (i.tm.mnem_off != MN_lea)
> +	return x86_tls_error_insn;
> +      else if (i.index_reg)
> +	{
> +	  if (i.base_reg)
> +	    return x86_tls_error_base_reg_name;
> +	  if (i.index_reg->reg_type.bitfield.instance != RegB)
> +	    return x86_tls_error_index_RegB;
> +	  else if (i.types[1].bitfield.instance != Accum)
> +	    return x86_tls_error_dest_RegA;
> +	  else if (i.log2_scale_factor)
> +	    return x86_tls_error_scale_factor;
> +	}
> +      else
> +	{
> +	  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;
> +	  else if (i.types[1].bitfield.instance != Accum)
> +	    return x86_tls_error_dest_RegA;

Using i.types[] here looks fragile to me. Please consider using
i.op[1].regs instead.

> +	}
> +      break;
> +
> +    case BFD_RELOC_386_TLS_LDM:
> +      /*  Check transition from LDM access model:
> +
> +	  leal foo@tlsldm(%reg), %eax --> Dest reg must be '%eax'
> +					  Memory reg can't be %eax and SIB
> +					  is not supported.
> +       */
> +      if (i.tm.mnem_off != MN_lea)
> +	return x86_tls_error_insn;
> +      else if (i.index_reg)
> +	return x86_tls_error_sib;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_type.bitfield.instance == Accum)
> +	return x86_tls_error_RegA;
> +      else if (i.types[1].bitfield.instance != Accum)
> +	return x86_tls_error_dest_RegA;
> +      break;
> +
> +    case BFD_RELOC_X86_64_GOTPC32_TLSDESC:
> +      /* Check transition from GOTPC32 TLSDESC access model:
> +
> +	 --- LP64 mode ---
> +	 leaq x@tlsdesc(%rip), %rax --> Memory reg must be %rip.
> +
> +	 --- X32 mode ---
> +	 rex leal x@tlsdesc(%rip), %eax --> Memory reg must be %rip.
> +
> +       */
> +      if (i.tm.mnem_off != MN_lea)
> +	return x86_tls_error_insn;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_num != RegIP)
> +	return x86_tls_error_RegIP;
> +      break;

And no check for the required REX prefix in x32 mode? Or is it gas that
automatically adds it (could do with a few words in the comment then)?

> +    case BFD_RELOC_X86_64_TLSGD:
> +      /* Check transition from GD access model:
> +
> +	 leaq foo@tlsgd(%rip), %rdi --> Only this fixed format is supported.
> +       */
> +    case BFD_RELOC_X86_64_TLSLD:
> +      /* Check transition from LD access model:
> +
> +	 leaq foo@tlsld(%rip), %rdi --> Only this fixed format is supported.
> +       */
> +      if (i.tm.mnem_off != MN_lea)
> +	return x86_tls_error_insn;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_num != RegIP)
> +	return x86_tls_error_RegIP;
> +      else if (i.types[1].bitfield.instance != RegDI)
> +	return x86_tls_error_dest_RegDI;

For this you don't need to invent RegDI. You can check the register's
number and flags.

> +      break;
> +
> +    case BFD_RELOC_386_TLS_GOTIE:
> +      /* Check transition from GOTIE access model:
> +
> +	 subl foo@gotntpoff(%reg1), %reg2
> +	 movl foo@gotntpoff(%reg1), %reg2
> +	 addl foo@gotntpoff(%reg1), %reg2
> +
> +	 Memory operand: SIB is not supported.
> +       */
> +    case BFD_RELOC_386_TLS_IE_32:
> +      /* Check transition from 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;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.index_reg)
> +	return x86_tls_error_sib;
> +      break;

And all MOV/ADD/SUB forms are permitted here? Plus what about operand
size (perhaps also already further up)?

> +    case BFD_RELOC_386_TLS_IE:
> +      /* Check transition from 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;
> +      else if (i.base_reg || i.index_reg)
> +	return x86_tls_error_require_no_base_index_reg;
> +      break;

Wouldn't this allow RegEIZ to pass, when that shouldn't pass?

> +    case BFD_RELOC_X86_64_GOTTPOFF:
> +      /* Check transition from 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;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_num != RegIP)
> +	return x86_tls_error_RegIP;
> +      break;
> +
> +    case BFD_RELOC_386_TLS_DESC_CALL:
> +      /* Check transition from GDesc access model:
> +
> +	 call *x@tlscall(%eax) --> Memory reg must be %eax and
> +				   SIB is not supported.
> +       */
> +      if (i.tm.mnem_off != MN_call)
> +	return x86_tls_error_insn;
> +      else if (i.index_reg)
> +	return x86_tls_error_sib;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_type.bitfield.instance != Accum)
> +	return x86_tls_error_RegA;
> +      break;

While the commentary for this is different from that for ...

> +    case BFD_RELOC_X86_64_TLSDESC_CALL:
> +      /* Check transition from 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;
> +      else if (i.index_reg)
> +	return x86_tls_error_sib;
> +      else if (!i.base_reg)
> +	return x86_tls_error_no_base_reg;
> +      else if (i.base_reg->reg_type.bitfield.instance != Accum)
> +	return x86_tls_error_RegA;
> +      break;

... this, the code is all the same. Fold both?

> +    default:
> +      return x86_tls_error_none;
> +    }
> +
> +  return x86_tls_error_none;
> +}
> +
> +static void
> +x86_report_tls_error (enum x86_tls_error_type tls_error, unsigned int 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 do not support SIB"), gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_no_base_reg:
> +      as_bad (_("@%s operator must have base regster"), gotrel[k].str);

Nit: "register"

> +      return;
> +
> +    case x86_tls_error_require_no_base_index_reg:
> +      as_bad (_("@%s operator should not have base/index register"),

Nit: Odd wording. The operator never has any register. Perhaps better
"... requires no base/index register"?

> +	      gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_base_reg_name:
> +      as_bad (_("@%s operator should not have base register"), gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_index_RegB:
> +      as_bad (_("@%s operator only supports ebx/rbx as index register"),
> +	      gotrel[k].str);

Please properly print register names: With % prefix when such is required,
and only the one register size that's actually permitted.

> +      return;
> +
> +    case x86_tls_error_RegA:
> +      as_bad (_("@%s operator only supports eax/rax as base register"),
> +	      gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_RegB:
> +      as_bad (_("@%s operator only supports ebx/rbx as base register"),
> +	      gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_RegIP:
> +      as_bad (_("@%s operator only supports rip as base register"),
> +	      gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_dest_RegA:
> +      as_bad (_("@%s operator only supports eax/rax as dest register"),
> +	      gotrel[k].str);
> +      return;
> +
> +    case x86_tls_error_dest_RegDI:
> +      as_bad (_("@%s operator only supports eax/rax as dest register"),
> +	      gotrel[k].str);

Surely not again eax/rax?

> +      return;
> +
> +    case x86_tls_error_scale_factor:
> +      as_bad (_("@%s operator only supports scale factor of 00"),
> +	      gotrel[k].str);

The scale _factor_ can never be 0; you mean 1 here.

> @@ -16786,6 +17072,14 @@ md_parse_option (int c, const char *arg)
>  	  optimize_for_space = 0;
>  	}
>        break;
> +    case OPTION_MTLS_CHECK:
> +      if (strcasecmp (arg, "yes") == 0)
> +	tls_check = 1;
> +      else if (strcasecmp (arg, "no") == 0)
> +	tls_check = 0;

Nit: The variable has type bool - please use true/false in such cases.

> --- a/opcodes/i386-gen.c
> +++ b/opcodes/i386-gen.c
> @@ -526,6 +526,7 @@ static const struct {
>      INSTANCE (RegC),
>      INSTANCE (RegD),
>      INSTANCE (RegB),
> +    INSTANCE (RegDI),
>  };

In addition to what H.J. said here and below: If this was really needed (and
then actually used somewhere in the opcode table), it would surely need some
explanation in the description. Plus ...

> --- a/opcodes/i386-opc.h
> +++ b/opcodes/i386-opc.h
> @@ -843,6 +843,7 @@ enum operand_instance
>    RegC,  /* %cl / %cx / %ecx / %rcx, e.g. register to hold shift count */
>    RegD,  /* %dl / %dx / %edx / %rdx, e.g. register to hold I/O port addr */
>    RegB,  /* %bl / %bx / %ebx / %rbx */
> +  RegDI, /* %rdx */
>  };

... the comment would then look wrong here. Yet as already indicated by H.J.,
this likely all wants dropping again anyway.

Jan