[PATCH] x86: Add tls check in gas
H.J. Lu
hjl.tools@gmail.com
Tue Aug 27 14:12:56 GMT 2024
On Tue, Aug 27, 2024 at 6:21 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Aug 26, 2024 at 11:59 PM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > 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.
>
> I submitted a patch:
>
> https://patchwork.sourceware.org/project/binutils/patch/20240827130654.4086230-1-hjl.tools@gmail.com/
>
> to drop the BFD_RELOC_386_TLS_LE_32 check.
>
> > 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
> ^^^^^^^
> guess
>
> > 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)
>
> Did you mean
>
> call *x@tlscall(%eax)
>
> > > + */
> > > + if (i.tm.mnem_off == MN_call)
> > > + return true;
> >
> > There are two forms of CALL - I doubt both are allowed.
>
> Please remove the CALL check and add a check for
> BFD_RELOC_386_TLS_DESC_CALL.
>
> >
> > > + /* 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)
>
> It should be "call *sg1@tlscall(%rax)". Please add a check
> for BFD_RELOC_X86_64_TLSDESC_CALL.
>
> > > + */
> > > + 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.
>
> Please use ELF relocation names.
Since these are assembler error messages, please report
the operator instead, something like
Error: @tpoff can't be used with kmovd
>
> > > --- /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
>
>
>
> --
> H.J.
--
H.J.
More information about the Binutils
mailing list