[PATCH] x86: Add tls check in gas
Cui, Lili
lili.cui@intel.com
Wed Aug 28 06:56:25 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.
>
Yes, I confirmed it with H.J in Bugzilla (https://sourceware.org/bugzilla/show_bug.cgi?id=32022), and H.J removed it with "[PATCH] x86: Allow R_386_TLS_LE_32 with KMOVD", I will rebase my patch.
> > 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.
>
Got it, thanks.
> As to the remark itself: I think keeping those tests as they are, with just the
> option added, is the way to go.
>
Ok.
> > 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.
>
Agree, I also don't like this solution, H.J created a patch to report operator for it, looks more reasonable.
> > @@ -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).
>
I used the same style here as previous one. But I think it only has two values, true or false, bool should be ok.
> > @@ -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.
Good point, in the linker code, it only supports lea. In abi doc, it supports lea and call. -- need HJ help to confirm here.
ABI doc:
General Dynamic Model Code Sequence with TLSDESC
LP64 ILP32
0x00 leaq x@tlsdesc(%rip),%rax 0x00 rex leal x@tlsdesc(%rip),%eax
0x07 call *x@tlsdesc((%rax) 0x07 call *x@tlsdesc(%eax)
0x08 add %fs:0x0,%eax 0x09 add %fs:0x0,%eax
>
> > + /* 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?
I think base/index register don't matter here, I saw both w/ and w/o sib testcases in linker's comments, but just for this tls type.
For operand size, linker doesn't check it. All I can think of is disabling reg16 in 32-bit mode, it doesn't make much sense. I prefer to drop this check and keep it consistent with the linker.
>
> > + 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.
>
It is a bug, BFD_RELOC_X86_64_TLSGD should be placed together with BFD_RELOC_X86_64_TLSLD.
> > + 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?
>
I prefer to do the same as what linker does.
> > + 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.
Copied it from linker, I think it means both reg32 and reg64 are ok.
>
> > + 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).
>
Good suggestion.
> For legibility may I further ask that non-fall-through case blocks be separated by
> a blank line?
>
Sure.
> > @@ -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?
>
I think immediate is constant value in instruction, it doesn't involve relocation process.
> > --- 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?
>
Added x86 for it.
> > @@ -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?
>
My first impression is similar to yours,but to be consistent with the previous option style, we use "enable_x86_tls_check = no" here.
> > --- 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.
>
Done.
> > --- /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)?
Good suggestion, I will use "-drw" for tls tests.
>
> > --- /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, ...
>
Done.
> > + movl foo@gottpoff(%ebx), %ecx
> > + addl foo@gottpoff(%ebx), %ecx
> > +
> > + /* BFD_RELOC_386_TLS_LE_32. */
> > + leal foo@tpoff(%ebx), %ecx
>
> ... again here.
>
Done.
Thanks,
Lili.
> Jan
More information about the Binutils
mailing list