[PATCH] x86: Add tls check in gas
Jan Beulich
jbeulich@suse.com
Tue Aug 27 06:59:32 GMT 2024
On 27.08.2024 08:07, Cui, Lili wrote:
> This patch is to add tls check in gas, and the following two changes are special:
> 1. Removed BFD_RELOC_386_TLS_LE_32 kmov test, as the linker does not change it.
Yet that, at some point, was specifically added (by or upon request from
H.J.). This needs clarifying. The more that you remove at least one check
in a testcase, which was deliberately put there iirc.
Also aiui this isn't meant to be part of the patch description (i.e. what
would eventually be committed), yet the reasoning here needs to be part
of the commit.
> 2. Added -mtls-check=no to gas reloc32/reloc64 test files as there are some illegal instructions. I'm not sure if we should remove them.
This, otoh, is legitimately a remark which doesn't need committing. For
reference (also for colleagues of yours), elsewhere the canonical way of
submitting patches is: Patch description, optionally a --- separator
followed by remarks (incl revision log; optionally intermediate ---
separators), optionally another --- separators followed by the diffstat,
actual patch hunks.
By following a fixed scheme it becomes clear to all involved parties
what parts of a submission are meant to be part of the eventual commit.
And it also is more predictable for readers what to look for where.
As to the remark itself: I think keeping those tests as they are, with
just the option added, is the way to go.
> Thanks,
> Lili.
>
> Assembler shouldn't accept invalid TLS instructions, TLS relocations
> can only be used with specific instructions as specified in TLS psABI
> and linker issues an error when TLS relocations are used with wrong
> instructions. It is inconvenient for gcc to rely on linker to report
> errors, adding tls check in the assembler stage so that gcc can know
> tls errors earlier.
>
> gas/ChangeLog:
>[...]
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -38,6 +38,9 @@
> #include "opcodes/i386-mnem.h"
> #include <limits.h>
>
> +#define _BFD_MAKE_TABLE_bfd_reloc_code_real
> +#include "bfd/libbfd.h"
This looks like a hack, without any explanation in the description. What
is this about? Having got to the bottom of the changes in the file, I
guesst this is so you can use bfd_reloc_code_real_names[]. Yet I don't
think that's how it's supposed to be done. But see a respective comment
further down - the BFD names anyway aren't what should be used, imo.
> @@ -717,6 +720,9 @@ lfence_before_ret;
> static int generate_relax_relocations
> = DEFAULT_GENERATE_X86_RELAX_RELOCATIONS;
>
> +/* 1 if the assembler should check tls relocation. */
> +static int tls_check = DEFAULT_TLS_CHECK;
bool please for booleans. Question though is whether we want this to be
a tristate (off, warning, error).
> @@ -6229,6 +6235,108 @@ static INLINE bool may_need_pass2 (const insn_template *t)
> && (t->base_opcode | 8) == 0x2c);
> }
>
> +static bool 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), %eax
> + call *x@tlsdesc(%eax)
> + */
> + if (i.tm.mnem_off == MN_call)
> + return true;
There are two forms of CALL - I doubt both are allowed.
> + /* fall through */
> + case BFD_RELOC_386_TLS_GD:
> + /* Check transition from GD access model:
> + leal foo@tlsgd(,%ebx,1), %eax
> + */
> + case BFD_RELOC_386_TLS_LDM:
> + /* Check transition from LDM access model:
> + leal foo@tlsldm(%ebx), %eax
> + */
> + if (i.tm.mnem_off == MN_lea)
> + return true;
And which base/index registers are used doesn't matter? Nor operand
size?
> + break;
> + case BFD_RELOC_X86_64_TLSGD:
> + /* Check transition from GD access model:
> + leaq foo@tlsgd(%rip), %rdi
> + */
Unlike above order is reversed here: Only LEA is mentioned, yet ...
> + case BFD_RELOC_X86_64_GOTPC32_TLSDESC:
> + /* Check transition from GOTPC32 TLSDESC access model:
> +
> + --- LP64 mode ---
> + leaq x@tlsdesc(%rip), %rax
> + call *x@tlsdesc(%rax)
> +
> + --- X32 mode ---
> + rex leal x@tlsdesc(%rip), %eax
> + call *x@tlsdesc(%eax)
> + */
> + if (i.tm.mnem_off == MN_call)
> + return true;
... CALL is being permitted. In fact ...
> + /* fall through */
> + case BFD_RELOC_X86_64_TLSLD:
> + /* Check transition from LD access model:
> + leaq foo@tlsld(%rip), %rdi
> + */
> + if (i.tm.mnem_off == MN_lea
> + && i.base_reg
> + && i.base_reg->reg_num == RegIP)
> + return true;
... this check looks to match what (as per the comment) X86_64_TLSGD
also needs.
> + 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
> + */
> + 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
> + */
> + if (i.tm.mnem_off == MN_sub)
> + return true;
> + /* fall through */
> + case BFD_RELOC_386_TLS_IE:
> + /* Check transition from IE access model:
> + movl foo@indntpoff, %eax
> + movl foo@indntpoff, %reg
> + addl foo@indntpoff, %reg
> + */
> + if (i.tm.mnem_off == MN_add || i.tm.mnem_off == MN_mov)
> + return true;
Same question again here (and below): Are all insn forms permitted?
> + break;
> + case BFD_RELOC_X86_64_GOTTPOFF:
> + /* Check transition from GOTTPOFF access model:
> + mov foo@gottpoff(%rip), %reg
> + add foo@gottpoff(%rip), %reg.
> + */
Here you even omit the insn suffixes, raising yet more questions.
> + if ((i.tm.mnem_off == MN_add || i.tm.mnem_off == MN_mov)
> + && i.base_reg
> + && i.base_reg->reg_num == RegIP)
> + return true;
> + break;
> + case BFD_RELOC_386_TLS_DESC_CALL:
> + /* Check transition from GDesc access model:
> + call *x@tlscall(%eax).
> + */
> + 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.
> + */
> + if (i.tm.mnem_off == MN_call)
> + return true;
> + break;
Throughout I think avoiding if() and instead using return directly
would make things quite a bit more clear. In particular one then won't
need to go from early in the switch() statement to the bottom of the
function to see what (really: no) further checks are done. I.e. for
the case here simply:
return i.tm.mnem_off == MN_call;
(assuming of course - see comments further up - that's really the
only thing which needs checking).
For legibility may I further ask that non-fall-through case blocks be
separated by a blank line?
> @@ -6567,6 +6675,18 @@ i386_assemble (char *line)
> i.prefix[LOCK_PREFIX] = 0;
> }
>
> + if (tls_check)
> + {
> + for (j = i.imm_operands; j < i.operands; ++j)
> + if (!x86_check_tls_relocation (i.reloc[j]))
> + {
> + as_bad (_("`%s' relocation cannot be used with `%s'"),
> + bfd_reloc_code_real_names[i.reloc[j]],
> + insn_name (&i.tm));
> + return;
> + }
> + }
Why do you skip immediate operands, which also may have relocations?
> --- a/gas/configure.ac
> +++ b/gas/configure.ac
> @@ -95,6 +95,17 @@ AC_ARG_ENABLE(x86_relax_relocations,
> no) ac_default_x86_relax_relocations=0 ;;
> esac])dnl
>
> +# PR gas/32022
> +# Decide if check tls relocation.
> +ac_default_tls_check=unset
> +# Provide a configure time option to override our default.
> +AC_ARG_ENABLE(tls_check,
> + AS_HELP_STRING([--enable-tls-check],
> + [check tls relocation by default]),
> +[case "${enableval}" in
> + no) ac_default_tls_check=0 ;;
> +esac])dnl
Like for the other one visible in context: Should there be "x86" in the
name? Or are there plans to extend this to other architectures?
> @@ -737,6 +748,13 @@ AC_DEFINE_UNQUOTED(DEFAULT_GENERATE_X86_RELAX_RELOCATIONS,
> $ac_default_x86_relax_relocations,
> [Define to 1 if you want to generate x86 relax relocations by default.])
>
> +if test ${ac_default_tls_check} = unset; then
> + ac_default_tls_check=1
> +fi
> +AC_DEFINE_UNQUOTED(DEFAULT_TLS_CHECK,
> + $ac_default_tls_check,
> + [Define to 1 if you want to check tls relocation by default.])
If the default is "enabled", shouldn't the option be --disable-tls-check?
> --- a/gas/testsuite/gas/i386/inval-tls.l
> +++ b/gas/testsuite/gas/i386/inval-tls.l
> @@ -1,3 +1,9 @@
> .*: Assembler messages:
> -.*:3: Error: .* `kmovd'
> -.*:4: Error: .* `kmovd'
> +.*:3: Error: `BFD_RELOC_386_TLS_GOTIE' relocation cannot be used with `kmovd'
> +.*:6: Error: `BFD_RELOC_386_TLS_GD' relocation cannot be used with `add'
> +.*:9: Error: `BFD_RELOC_386_TLS_LDM' relocation cannot be used with `add'
> +.*:12: Error: `BFD_RELOC_386_TLS_GOTDESC' relocation cannot be used with `add'
> +.*:15: Error: `BFD_RELOC_386_TLS_IE' relocation cannot be used with `sub'
> +.*:18: Error: `BFD_RELOC_386_TLS_GOTIE' relocation cannot be used with `lea'
> +.*:21: Error: `BFD_RELOC_386_TLS_IE_32' relocation cannot be used with `lea'
> +.*:24: Error: `BFD_RELOC_386_TLS_DESC_CALL' relocation cannot be used with `lea'
BFD-internal names in diagnostics aren't very helpful. The diagnostics
should mention either the reloc specifier that was used in source code
(preferred, for making it easiest to associate the diagnostic with the
respective location in source code), or - all of this being ELF-only
anyway if I'm not mistaken - the ELF relocation names.
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/tls.d
> @@ -0,0 +1,28 @@
> +#as:
> +#objdump: -dw
Maybe better -drw (also for the 64-bit counterpart)?
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/tls.s
> @@ -0,0 +1,36 @@
> +# Check tls relocation 32-bit mode
> +
> + .text
> +_start:
> + /* BFD_RELOC_386_TLS_GD. */
> + leal foo@tlsgd(,%ebx,1), %eax
> +
> + /* BFD_RELOC_386_TLS_LDM. */
> + leal foo@tlsldm(%ebx), %eax
> +
> + /* BFD_RELOC_386_TLS_GOTDESC. */
> + leal x@tlsdesc(%ebx), %eax
> + call *x@tlsdesc(%eax)
> +
> + /* BFD_RELOC_386_TLS_IE. */
> + movl foo@indntpoff, %eax
> + movl foo@indntpoff, %ebx
> + addl foo@indntpoff, %edx
> +
> + /* BFD_RELOC_386_TLS_GOTIE. */
> + subl foo@gotntpoff(%ebx), %ecx
> + movl foo@gotntpoff(%ebx), %ecx
> + addl foo@gotntpoff(%ebx), %ecx
> +
> + /* BFD_RELOC_386_TLS_IE_32. */
> + subl foo@gottpoff(%ebx), %ecx
Nit: Indentation, ...
> + movl foo@gottpoff(%ebx), %ecx
> + addl foo@gottpoff(%ebx), %ecx
> +
> + /* BFD_RELOC_386_TLS_LE_32. */
> + leal foo@tpoff(%ebx), %ecx
... again here.
Jan
More information about the Binutils
mailing list