| Message ID | 20240206162801.882585-2-mjeanson@efficios.com |
|---|---|
| State | New |
| Headers | show |
| Series | Extend rseq support | expand |
On 2024-02-06 11:27, Michael Jeanson wrote: > While working on a patch to add support for the extensible rseq ABI, we > came across an issue where a new 'const' variable would be merged with > the existing '__rseq_size' variable. We tracked this to the use of > '-fmerge-all-constants' which allows the compiler to merge identical > constant variables. This means that all 'const' variables in a compile > unit that are of the same size and are initialized to the same value can > be merged. > > In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t' > are both 4 bytes and initialized to 0 which should trigger the merge. > However for reasons we haven't delved into when the attribute 'section > (".data.rel.ro")' is added to the mix, only variables of the same exact > types are merged. As far as we know this behavior is not specified > anywhere and could change with a new compiler version, hence this patch. > > Move the definitions of these variables into an assembler file and add > hidden writable aliases for internal use. This has the added bonus of > removing the asm workaround to set the values on rseq registration. > > Tested on Debian 12 with GCC 12.2. Hi Florian, Just to make sure you don't duplicate this effort: we ended up implementing this fix, even though we originally said we did not intend to. Feedback is welcome! Thanks, Mathieu > > Signed-off-by: Michael Jeanson <mjeanson@efficios.com> > Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > --- > csu/Makefile | 2 +- > csu/rseq-sizes.sym | 8 +++++ > elf/Makefile | 1 + > elf/dl-rseq-symbols.S | 55 +++++++++++++++++++++++++++++++++++ > sysdeps/nptl/dl-tls_init_tp.c | 14 ++++----- > 5 files changed, 71 insertions(+), 9 deletions(-) > create mode 100644 csu/rseq-sizes.sym > create mode 100644 elf/dl-rseq-symbols.S > > diff --git a/csu/Makefile b/csu/Makefile > index ac05ab24d5..0bf51a0e48 100644 > --- a/csu/Makefile > +++ b/csu/Makefile > @@ -99,7 +99,7 @@ before-compile += $(objpfx)abi-tag.h > generated += abi-tag.h > > # Put it here to generate it earlier. > -gen-as-const-headers += rtld-sizes.sym > +gen-as-const-headers += rtld-sizes.sym rseq-sizes.sym > > # These are the special initializer/finalizer files. They are always the > # first and last file in the link. crti.o ... crtn.o define the global > diff --git a/csu/rseq-sizes.sym b/csu/rseq-sizes.sym > new file mode 100644 > index 0000000000..c959758ff0 > --- /dev/null > +++ b/csu/rseq-sizes.sym > @@ -0,0 +1,8 @@ > +#include <stddef.h> > + > +-- > +RSEQ_SIZE_SIZE sizeof (unsigned int) > +RSEQ_SIZE_ALIGN __alignof (unsigned int) > + > +RSEQ_OFFSET_SIZE sizeof (ptrdiff_t) > +RSEQ_OFFSET_ALIGN __alignof (ptrdiff_t) > diff --git a/elf/Makefile b/elf/Makefile > index 5d78b659ce..7d711aedf0 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -73,6 +73,7 @@ dl-routines = \ > dl-origin \ > dl-printf \ > dl-reloc \ > + dl-rseq-symbols \ > dl-runtime \ > dl-scope \ > dl-setup_hash \ > diff --git a/elf/dl-rseq-symbols.S b/elf/dl-rseq-symbols.S > new file mode 100644 > index 0000000000..2d8e88367f > --- /dev/null > +++ b/elf/dl-rseq-symbols.S > @@ -0,0 +1,55 @@ > +/* Define symbols used by rseq. > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <rseq-sizes.h> > +#include <sysdep.h> > + > +/* Some targets define a macro to denote the zero register. */ > +#undef zero > + > +/* Define 2 symbols, __rseq_size is public const and _rseq_size, which is an > + alias of __rseq_size, but hidden and writable for internal use. */ > + > + .globl __rseq_size > + .type __rseq_size, %object > + .size __rseq_size, RSEQ_SIZE_SIZE > + .hidden _rseq_size > + .globl _rseq_size > + .type _rseq_size, %object > + .size _rseq_size, RSEQ_SIZE_SIZE > + .section .data.rel.ro > + .balign RSEQ_SIZE_ALIGN > +__rseq_size: > +_rseq_size: > + .zero RSEQ_SIZE_SIZE > + > +/* Define 2 symbols, __rseq_offset is public const and _rseq_offset, which is an > + alias of __rseq_offset, but hidden and writable for internal use. */ > + > + .globl __rseq_offset > + .type __rseq_offset, %object > + .size __rseq_offset, RSEQ_OFFSET_SIZE > + .hidden _rseq_offset > + .globl _rseq_offset > + .type _rseq_offset, %object > + .size _rseq_offset, RSEQ_OFFSET_SIZE > + .section .data.rel.ro > + .balign RSEQ_OFFSET_ALIGN > +__rseq_offset: > +_rseq_offset: > + .zero RSEQ_OFFSET_SIZE > diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c > index 092c274f36..80eb0107b5 100644 > --- a/sysdeps/nptl/dl-tls_init_tp.c > +++ b/sysdeps/nptl/dl-tls_init_tp.c > @@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock) > #endif > > const unsigned int __rseq_flags; > -const unsigned int __rseq_size attribute_relro; > -const ptrdiff_t __rseq_offset attribute_relro; > + > +/* The variables are in .data.relro but are not yet write-protected. */ > +extern unsigned int _rseq_size attribute_relro attribute_hidden; > +extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden; > > void > __tls_pre_init_tp (void) > @@ -105,10 +107,7 @@ __tls_init_tp (void) > do_rseq = TUNABLE_GET (rseq, int, NULL); > if (rseq_register_current_thread (pd, do_rseq)) > { > - /* We need a writable view of the variables. They are in > - .data.relro and are not yet write-protected. */ > - extern unsigned int size __asm__ ("__rseq_size"); > - size = sizeof (pd->rseq_area); > + _rseq_size = sizeof (pd->rseq_area); > } > > #ifdef RSEQ_SIG > @@ -117,8 +116,7 @@ __tls_init_tp (void) > all targets support __thread_pointer, so set __rseq_offset only > if the rseq registration may have happened because RSEQ_SIG is > defined. */ > - extern ptrdiff_t offset __asm__ ("__rseq_offset"); > - offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > + _rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > #endif > } >
Michael Jeanson <mjeanson@efficios.com> writes: > We tracked this to the use of '-fmerge-all-constants' which allows the > compiler to merge identical constant variables. I assume the linker's constant (string?) merging won't affect this because the section is marked as writable to the linker? > diff --git a/csu/Makefile b/csu/Makefile > # Put it here to generate it earlier. > -gen-as-const-headers += rtld-sizes.sym > +gen-as-const-headers += rtld-sizes.sym rseq-sizes.sym Ok. > diff --git a/csu/rseq-sizes.sym b/csu/rseq-sizes.sym > new file mode 100644 > index 0000000000..c959758ff0 > --- /dev/null > +++ b/csu/rseq-sizes.sym > @@ -0,0 +1,8 @@ > +#include <stddef.h> > + > +-- > +RSEQ_SIZE_SIZE sizeof (unsigned int) > +RSEQ_SIZE_ALIGN __alignof (unsigned int) > + > +RSEQ_OFFSET_SIZE sizeof (ptrdiff_t) > +RSEQ_OFFSET_ALIGN __alignof (ptrdiff_t) Looks like rtld-sizes.sym, so OK. > diff --git a/elf/Makefile b/elf/Makefile > + dl-rseq-symbols \ Ok. > diff --git a/elf/dl-rseq-symbols.S b/elf/dl-rseq-symbols.S > +#include <rseq-sizes.h> > +#include <sysdep.h> Ok > +/* Some targets define a macro to denote the zero register. */ > +#undef zero Ok. > +/* Define 2 symbols, __rseq_size is public const and _rseq_size, which is an > + alias of __rseq_size, but hidden and writable for internal use. */ > + > + .globl __rseq_size > + .type __rseq_size, %object > + .size __rseq_size, RSEQ_SIZE_SIZE > + .hidden _rseq_size > + .globl _rseq_size > + .type _rseq_size, %object > + .size _rseq_size, RSEQ_SIZE_SIZE > + .section .data.rel.ro > + .balign RSEQ_SIZE_ALIGN > +__rseq_size: > +_rseq_size: > + .zero RSEQ_SIZE_SIZE Ok. > +/* Define 2 symbols, __rseq_offset is public const and _rseq_offset, which is an > + alias of __rseq_offset, but hidden and writable for internal use. */ > + > + .globl __rseq_offset > + .type __rseq_offset, %object > + .size __rseq_offset, RSEQ_OFFSET_SIZE > + .hidden _rseq_offset > + .globl _rseq_offset > + .type _rseq_offset, %object > + .size _rseq_offset, RSEQ_OFFSET_SIZE > + .section .data.rel.ro > + .balign RSEQ_OFFSET_ALIGN > +__rseq_offset: > +_rseq_offset: > + .zero RSEQ_OFFSET_SIZE Ok. > diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c > index 092c274f36..80eb0107b5 100644 > --- a/sysdeps/nptl/dl-tls_init_tp.c > +++ b/sysdeps/nptl/dl-tls_init_tp.c > @@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock) > #endif > > const unsigned int __rseq_flags; > -const unsigned int __rseq_size attribute_relro; > -const ptrdiff_t __rseq_offset attribute_relro; > + > +/* The variables are in .data.relro but are not yet write-protected. */ > +extern unsigned int _rseq_size attribute_relro attribute_hidden; > +extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden; er, extern *and* hidden? And relro? In theory the relro one is harmless but meaningless, as externs don't have sections in the local compilation unit. The hidden one will mark the symbol hidden, but does that really matter as long as it's marked hidden where it's defined? > void > __tls_pre_init_tp (void) > @@ -105,10 +107,7 @@ __tls_init_tp (void) > do_rseq = TUNABLE_GET (rseq, int, NULL); > if (rseq_register_current_thread (pd, do_rseq)) > { > - /* We need a writable view of the variables. They are in > - .data.relro and are not yet write-protected. */ > - extern unsigned int size __asm__ ("__rseq_size"); > - size = sizeof (pd->rseq_area); > + _rseq_size = sizeof (pd->rseq_area); > } Ok. > #ifdef RSEQ_SIG > @@ -117,8 +116,7 @@ __tls_init_tp (void) > all targets support __thread_pointer, so set __rseq_offset only > if the rseq registration may have happened because RSEQ_SIG is > defined. */ > - extern ptrdiff_t offset __asm__ ("__rseq_offset"); > - offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > + _rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); > #endif > } Ok.
On 2024-02-15 21 h 02, DJ Delorie wrote: > > Michael Jeanson <mjeanson@efficios.com> writes: >> We tracked this to the use of '-fmerge-all-constants' which allows the >> compiler to merge identical constant variables. > > I assume the linker's constant (string?) merging won't affect this > because the section is marked as writable to the linker? I checked and the .data.rel.ro section does indeed have the SHF_WRITE flag set. My understanding is that the section would need to be read-only and the SHF_MERGE flag would need to be set for the linker to try merging things. >> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c >> index 092c274f36..80eb0107b5 100644 >> --- a/sysdeps/nptl/dl-tls_init_tp.c >> +++ b/sysdeps/nptl/dl-tls_init_tp.c >> @@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock) >> #endif >> >> const unsigned int __rseq_flags; >> -const unsigned int __rseq_size attribute_relro; >> -const ptrdiff_t __rseq_offset attribute_relro; >> + >> +/* The variables are in .data.relro but are not yet write-protected. */ >> +extern unsigned int _rseq_size attribute_relro attribute_hidden; >> +extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden; > > er, extern *and* hidden? And relro? > > In theory the relro one is harmless but meaningless, as externs don't > have sections in the local compilation unit. > > The hidden one will mark the symbol hidden, but does that really matter > as long as it's marked hidden where it's defined? Both attributes can be removed, the definition in the assembly file is what matters. I'll clean this in the next patch series.
Michael Jeanson <mjeanson@efficios.com> writes: >> I assume the linker's constant (string?) merging won't affect this >> because the section is marked as writable to the linker? > > I checked and the .data.rel.ro section does indeed have the SHF_WRITE > flag set. My understanding is that the section would need to be > read-only and the SHF_MERGE flag would need to be set for the linker to > try merging things. That's what I figured too, but good to confirm.
diff --git a/csu/Makefile b/csu/Makefile index ac05ab24d5..0bf51a0e48 100644 --- a/csu/Makefile +++ b/csu/Makefile @@ -99,7 +99,7 @@ before-compile += $(objpfx)abi-tag.h generated += abi-tag.h # Put it here to generate it earlier. -gen-as-const-headers += rtld-sizes.sym +gen-as-const-headers += rtld-sizes.sym rseq-sizes.sym # These are the special initializer/finalizer files. They are always the # first and last file in the link. crti.o ... crtn.o define the global diff --git a/csu/rseq-sizes.sym b/csu/rseq-sizes.sym new file mode 100644 index 0000000000..c959758ff0 --- /dev/null +++ b/csu/rseq-sizes.sym @@ -0,0 +1,8 @@ +#include <stddef.h> + +-- +RSEQ_SIZE_SIZE sizeof (unsigned int) +RSEQ_SIZE_ALIGN __alignof (unsigned int) + +RSEQ_OFFSET_SIZE sizeof (ptrdiff_t) +RSEQ_OFFSET_ALIGN __alignof (ptrdiff_t) diff --git a/elf/Makefile b/elf/Makefile index 5d78b659ce..7d711aedf0 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -73,6 +73,7 @@ dl-routines = \ dl-origin \ dl-printf \ dl-reloc \ + dl-rseq-symbols \ dl-runtime \ dl-scope \ dl-setup_hash \ diff --git a/elf/dl-rseq-symbols.S b/elf/dl-rseq-symbols.S new file mode 100644 index 0000000000..2d8e88367f --- /dev/null +++ b/elf/dl-rseq-symbols.S @@ -0,0 +1,55 @@ +/* Define symbols used by rseq. + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <rseq-sizes.h> +#include <sysdep.h> + +/* Some targets define a macro to denote the zero register. */ +#undef zero + +/* Define 2 symbols, __rseq_size is public const and _rseq_size, which is an + alias of __rseq_size, but hidden and writable for internal use. */ + + .globl __rseq_size + .type __rseq_size, %object + .size __rseq_size, RSEQ_SIZE_SIZE + .hidden _rseq_size + .globl _rseq_size + .type _rseq_size, %object + .size _rseq_size, RSEQ_SIZE_SIZE + .section .data.rel.ro + .balign RSEQ_SIZE_ALIGN +__rseq_size: +_rseq_size: + .zero RSEQ_SIZE_SIZE + +/* Define 2 symbols, __rseq_offset is public const and _rseq_offset, which is an + alias of __rseq_offset, but hidden and writable for internal use. */ + + .globl __rseq_offset + .type __rseq_offset, %object + .size __rseq_offset, RSEQ_OFFSET_SIZE + .hidden _rseq_offset + .globl _rseq_offset + .type _rseq_offset, %object + .size _rseq_offset, RSEQ_OFFSET_SIZE + .section .data.rel.ro + .balign RSEQ_OFFSET_ALIGN +__rseq_offset: +_rseq_offset: + .zero RSEQ_OFFSET_SIZE diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c index 092c274f36..80eb0107b5 100644 --- a/sysdeps/nptl/dl-tls_init_tp.c +++ b/sysdeps/nptl/dl-tls_init_tp.c @@ -45,8 +45,10 @@ rtld_mutex_dummy (pthread_mutex_t *lock) #endif const unsigned int __rseq_flags; -const unsigned int __rseq_size attribute_relro; -const ptrdiff_t __rseq_offset attribute_relro; + +/* The variables are in .data.relro but are not yet write-protected. */ +extern unsigned int _rseq_size attribute_relro attribute_hidden; +extern ptrdiff_t _rseq_offset attribute_relro attribute_hidden; void __tls_pre_init_tp (void) @@ -105,10 +107,7 @@ __tls_init_tp (void) do_rseq = TUNABLE_GET (rseq, int, NULL); if (rseq_register_current_thread (pd, do_rseq)) { - /* We need a writable view of the variables. They are in - .data.relro and are not yet write-protected. */ - extern unsigned int size __asm__ ("__rseq_size"); - size = sizeof (pd->rseq_area); + _rseq_size = sizeof (pd->rseq_area); } #ifdef RSEQ_SIG @@ -117,8 +116,7 @@ __tls_init_tp (void) all targets support __thread_pointer, so set __rseq_offset only if the rseq registration may have happened because RSEQ_SIG is defined. */ - extern ptrdiff_t offset __asm__ ("__rseq_offset"); - offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); + _rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer (); #endif }
While working on a patch to add support for the extensible rseq ABI, we came across an issue where a new 'const' variable would be merged with the existing '__rseq_size' variable. We tracked this to the use of '-fmerge-all-constants' which allows the compiler to merge identical constant variables. This means that all 'const' variables in a compile unit that are of the same size and are initialized to the same value can be merged. In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t' are both 4 bytes and initialized to 0 which should trigger the merge. However for reasons we haven't delved into when the attribute 'section (".data.rel.ro")' is added to the mix, only variables of the same exact types are merged. As far as we know this behavior is not specified anywhere and could change with a new compiler version, hence this patch. Move the definitions of these variables into an assembler file and add hidden writable aliases for internal use. This has the added bonus of removing the asm workaround to set the values on rseq registration. Tested on Debian 12 with GCC 12.2. Signed-off-by: Michael Jeanson <mjeanson@efficios.com> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> --- csu/Makefile | 2 +- csu/rseq-sizes.sym | 8 +++++ elf/Makefile | 1 + elf/dl-rseq-symbols.S | 55 +++++++++++++++++++++++++++++++++++ sysdeps/nptl/dl-tls_init_tp.c | 14 ++++----- 5 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 csu/rseq-sizes.sym create mode 100644 elf/dl-rseq-symbols.S