[PATCH v3] Set the retain attribute on _elf_set_element if CC supports [BZ #27492]
Fāng-ruì Sòng
maskray@google.com
Wed Mar 31 18:09:16 GMT 2021
On 2021-03-31, H.J. Lu wrote:
>On Tue, Mar 30, 2021 at 9:34 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>>
>> On 2021-03-30, H.J. Lu wrote:
>> >On Tue, Mar 30, 2021 at 12:17 PM Fāng-ruì Sòng <maskray@google.com> wrote:
>> >>
>> >> On Tue, Mar 30, 2021 at 11:17 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>> >> >
>> >> > On Mon, Mar 29, 2021 at 4:24 PM Fangrui Song <maskray@google.com> wrote:
>> >> > >
>> >> > > So that text_set_element/data_set_element/bss_set_element defined
>> >> > > variables will be retained by the linker.
>> >> > >
>> >> > > Note: 'used' and 'retain' are orthogonal: 'used' makes sure the variable
>> >> > > will not be optimized out; 'retain' prevents section garbage collection
>> >> > > if the linker support SHF_GNU_RETAIN.
>> >> > >
>> >> > > GNU ld 2.37 and LLD 13 will support -z start-stop-gc which allow C
>> >> > > identifier name sections to be GCed even if there are live
>> >> > > __start_/__stop_ references.
>> >> > >
>> >> > > Without the change, there are some static linking problems, e.g.
>> >> > > _IO_cleanup (libio/genops.c) may be discarded by ld --gc-sections, so
>> >> > > stdout is not flushed on exit.
>> >> > >
>> >> > > Note: GCC may warning ‘retain’ attribute ignored while __has_attribute(retain) is 1
>> >> > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587).
>> >> > > ---
>> >> > > Changes in v1 -> v2:
>> >> > > * Define attribute_used_retain_section
>> >> > > Changes in v2 -> v3:
>> >> > > * Use attribute_used_retain instead attribute_used_retain_section
>> >> > > ---
>> >> > > config.h.in | 3 +++
>> >> > > configure | 23 +++++++++++++++++++++++
>> >> > > configure.ac | 16 ++++++++++++++++
>> >> > > include/libc-symbols.h | 14 ++++++++++----
>> >> > > 4 files changed, 52 insertions(+), 4 deletions(-)
>> >> > >
>> >> > > diff --git a/config.h.in b/config.h.in
>> >> > > index ca1547ae67..95af1ef229 100644
>> >> > > --- a/config.h.in
>> >> > > +++ b/config.h.in
>> >> > > @@ -187,6 +187,9 @@
>> >> > > /* Define if gcc supports attribute ifunc. */
>> >> > > #undef HAVE_GCC_IFUNC
>> >> > >
>> >> > > +/* Define if gcc supports attribute retain. */
>> >> > > +#undef HAVE_GCC_RETAIN
>> >> > > +
>> >> > > /* Define if the linker defines __ehdr_start. */
>> >> > > #undef HAVE_EHDR_START
>> >> > >
>> >> > > diff --git a/configure b/configure
>> >> > > index fcf43bf7de..6a6b8ac264 100755
>> >> > > --- a/configure
>> >> > > +++ b/configure
>> >> > > @@ -4105,6 +4105,29 @@ fi
>> >> > > $as_echo "$libc_cv_textrel_ifunc" >&6; }
>> >> > >
>> >> > >
>> >> > > +# Check if gcc supports attribute ifunc as it is used in libc_ifunc macro.
>> >> > > +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for gcc attribute retain support" >&5
>> >> > > +$as_echo_n "checking for gcc attribute retain support... " >&6; }
>> >> > > +if ${libc_cv_gcc_retain+:} false; then :
>> >> > > + $as_echo_n "(cached) " >&6
>> >> > > +else
>> >> > > + cat > conftest.c <<EOF
>> >> > > +static int var __attribute__ ((used, retain, section ("__libc_atexit")));
>> >> > > +EOF
>> >> > > +libc_cv_gcc_retain=no
>> >> > > +if ${CC-cc} -Werror -c conftest.c -o /dev/null 1>&5 \
>> >> > > + 2>&5 ; then
>> >> > > + libc_cv_gcc_retain=yes
>> >> > > +fi
>> >> > > +rm -f conftest*
>> >> > > +fi
>> >> > > +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libc_cv_gcc_retain" >&5
>> >> > > +$as_echo "$libc_cv_gcc_retain" >&6; }
>> >> > > +if test $libc_cv_gcc_retain = yes; then
>> >> > > + $as_echo "#define HAVE_GCC_RETAIN 1" >>confdefs.h
>> >> > > +
>> >> > > +fi
>> >> > > +
>> >> > > # Check if gcc warns about alias for function with incompatible types.
>> >> > > { $as_echo "$as_me:${as_lineno-$LINENO}: checking if compiler warns about alias for function with incompatible types" >&5
>> >> > > $as_echo_n "checking if compiler warns about alias for function with incompatible types... " >&6; }
>> >> > > diff --git a/configure.ac b/configure.ac
>> >> > > index fce967f2c2..031b8c3fb3 100644
>> >> > > --- a/configure.ac
>> >> > > +++ b/configure.ac
>> >> > > @@ -707,6 +707,22 @@ fi
>> >> > > rm -f conftest*])
>> >> > > AC_SUBST(libc_cv_textrel_ifunc)
>> >> > >
>> >> > > +# Check if gcc supports attribute ifunc as it is used in libc_ifunc macro.
>> >> > > +AC_CACHE_CHECK([for gcc attribute retain support],
>> >> > > + libc_cv_gcc_retain, [dnl
>> >> > > +cat > conftest.c <<EOF
>> >> > > +static int var __attribute__ ((used, retain, section ("__libc_atexit")));
>> >> > > +EOF
>> >> > > +libc_cv_gcc_retain=no
>> >> > > +if ${CC-cc} -Werror -c conftest.c -o /dev/null 1>&AS_MESSAGE_LOG_FD \
>> >> > > + 2>&AS_MESSAGE_LOG_FD ; then
>> >> > > + libc_cv_gcc_retain=yes
>> >> > > +fi
>> >> > > +rm -f conftest*])
>> >> > > +if test $libc_cv_gcc_retain = yes; then
>> >> > > + AC_DEFINE(HAVE_GCC_RETAIN)
>> >> > > +fi
>> >> > > +
>> >> > > # Check if gcc warns about alias for function with incompatible types.
>> >> > > AC_CACHE_CHECK([if compiler warns about alias for function with incompatible types],
>> >> > > libc_cv_gcc_incompatible_alias, [dnl
>> >> > > diff --git a/include/libc-symbols.h b/include/libc-symbols.h
>> >> > > index 546fc26a7b..b311c90f0c 100644
>> >> > > --- a/include/libc-symbols.h
>> >> > > +++ b/include/libc-symbols.h
>> >> > > @@ -352,6 +352,12 @@ for linking")
>> >> > >
>> >> > > */
>> >> > >
>> >> > > +#ifdef HAVE_GCC_RETAIN
>> >> > > +# define attribute_used_retain __attribute__ ((__used__, __retain__))
>> >> > > +#else
>> >> > > +# define attribute_used_retain __attribute__ ((__used__))
>> >> > > +#endif
>> >> > > +
>> >> > > /* Symbol set support macros. */
>> >> > >
>> >> > > /* Make SYMBOL, which is in the text segment, an element of SET. */
>> >> > > @@ -367,12 +373,12 @@ for linking")
>> >> > > /* When building a shared library, make the set section writable,
>> >> > > because it will need to be relocated at run time anyway. */
>> >> > > # define _elf_set_element(set, symbol) \
>> >> > > - static const void *__elf_set_##set##_element_##symbol##__ \
>> >> > > - __attribute__ ((used, section (#set))) = &(symbol)
>> >> > > + static const void *__elf_set_##set##_element_##symbol##__ \
>> >> > > + attribute_used_retain __attribute__ ((section (#set))) = &(symbol)
>> >> > > #else
>> >> > > # define _elf_set_element(set, symbol) \
>> >> > > - static const void *const __elf_set_##set##_element_##symbol##__ \
>> >> > > - __attribute__ ((used, section (#set))) = &(symbol)
>> >> > > + static const void *const __elf_set_##set##_element_##symbol##__ \
>> >> > > + attribute_used_retain __attribute__ ((section (#set))) = &(symbol)
>> >> > > #endif
>> >> > >
>> >> > > /* Define SET as a symbol set. This may be required (it is in a.out) to
>> >> > > --
>> >> > > 2.31.0.291.g576ba9dcdaf-goog
>> >> > >
>> >> >
>> >> > Need gc-sections tests for linkers with and without -z start-stop-gc.
>> >> >
>> >> > --
>> >> > H.J.
>> >>
>> >> I don't know how to add such a test... It needs to check flush at ext
>> >> time but I cannot find a template in libio/tst-* ...
>> >
>> >You can add a special rule to check for linker output with
>> >--print-gc-sections.
>>
>> I can come up runtime tests. ( https://github.com/MaskRay/glibc/tree/retain)
>> Runtime tests are direct.
>> (Unsure how to test --print-gc-sections with tests-static...)
>>
>>
>> diff --git a/libio/Makefile b/libio/Makefile
>> index 12ce41038f..5e0e7f00cd 100644
>> --- a/libio/Makefile
>> +++ b/libio/Makefile
>> @@ -195,6 +195,13 @@ ifeq (yes,$(build-shared))
>> tests-special += $(objpfx)tst-fopenloc-cmp.out $(objpfx)tst-fopenloc-mem.out \
>> $(objpfx)tst-bz24228-mem.out
>> endif
>> +
>> +tests += tst-cleanup-start-stop-gc tst-cleanup-nostart-stop-gc
>> +tests-static += tst-cleanup-start-stop-gc tst-cleanup-nostart-stop-gc
>> +tests-special += $(objpfx)tst-cleanup-start-stop-gc.out \
>> + $(objpfx)tst-cleanup-nostart-stop-gc.out
>> +LDFLAGS-tst-cleanup-start-stop-gc = -Wl,--gc-sections,-z,start-stop-gc
>> +LDFLAGS-tst-cleanup-nostart-stop-gc = -Wl,--gc-sections,-z,nostart-stop-gc
>
>1. Need to check if linker supports -z start-stop-gc.
This seems difficult to do with Makefile... Second, do we need to check it?
ld warns instead of errors for an unknown -z option, so not checking it
may be fine. (GNU ld warns even with --fatal-warnings.)
>2. Need to test with only -Wl,--gc-sections.
OK, removed -z,nostart-stop-gc
-LDFLAGS-tst-cleanup-nostart-stop-gc = -Wl,--gc-sections,-z,nostart-stop-gc
+LDFLAGS-tst-cleanup-nostart-stop-gc = -Wl,--gc-sections
>> endif
>>
>> include ../Rules
>> @@ -224,6 +231,20 @@ $(objpfx)tst_wprintf2.out: $(gen-locales)
>> $(objpfx)tst-wfile-sync.out: $(gen-locales)
>> endif
>>
>> +$(objpfx)tst-cleanup-start-stop-gc.out: $(objpfx)tst-cleanup-start-stop-gc
>> + $< > $@T
>> + grep hello $@T; $(evaluate-test)
>> + rm -f $@T
>
>tst-cleanup-start-stop-gc.out isn't generated. You should add a ".exp" file
>and compare the .out file against it.
Thanks for the tip. Added tst-cleanup.exp with the content "hello\n" (see the usage below).
>
>> +$(objpfx)tst-cleanup-start-stop-gc.o: tst-cleanup.c
>> + $(compile.c) -o $@
>> +
>> +$(objpfx)tst-cleanup-nostart-stop-gc.out: $(objpfx)tst-cleanup-nostart-stop-gc
>> + $< > $@T
>> + grep hello $@T; $(evaluate-test)
>> + rm -f $@T
>> +$(objpfx)tst-cleanup-nostart-stop-gc.o: tst-cleanup.c
>> + $(compile.c) -o $@
>> +
>> $(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
>> $(SHELL) $< $(common-objpfx) '$(test-program-prefix)' \
>> $(common-objpfx)libio/; \
>> diff --git a/libio/tst-cleanup.c b/libio/tst-cleanup.c
>> new file mode 100644
>> index 0000000000..de8130a482
>> --- /dev/null
>> +++ b/libio/tst-cleanup.c
>> @@ -0,0 +1,14 @@
>
>Missing comments and copyright notice.
OK. I find that many tst* files don't have a copyright notice..
Added.
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +
>> +void
>> +hook (void)
>> +{
>> + puts ("hello");
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> + atexit (hook);
>> +}
diff --git a/libio/Makefile b/libio/Makefile
index 12ce41038f..472a1e064c 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -195,6 +195,13 @@ ifeq (yes,$(build-shared))
tests-special += $(objpfx)tst-fopenloc-cmp.out $(objpfx)tst-fopenloc-mem.out \
$(objpfx)tst-bz24228-mem.out
endif
+
+tests += tst-cleanup-start-stop-gc tst-cleanup-nostart-stop-gc
+tests-static += tst-cleanup-start-stop-gc tst-cleanup-nostart-stop-gc
+tests-special += $(objpfx)tst-cleanup-start-stop-gc-cmp.out \
+ $(objpfx)tst-cleanup-nostart-stop-gc-cmp.out
+LDFLAGS-tst-cleanup-start-stop-gc = -Wl,--gc-sections,-z,start-stop-gc
+LDFLAGS-tst-cleanup-nostart-stop-gc = -Wl,--gc-sections,-z,nostart-stop-gc
endif
include ../Rules
@@ -224,6 +231,18 @@ $(objpfx)tst_wprintf2.out: $(gen-locales)
$(objpfx)tst-wfile-sync.out: $(gen-locales)
endif
+$(objpfx)tst-cleanup-start-stop-gc-cmp.out: tst-cleanup.exp $(objpfx)tst-cleanup-start-stop-gc.out
+ cmp $^ > $@; \
+ $(evaluate-test)
+$(objpfx)tst-cleanup-start-stop-gc.o: tst-cleanup.c
+ $(compile.c) -o $@
+
+$(objpfx)tst-cleanup-nostart-stop-gc-cmp.out: tst-cleanup.exp $(objpfx)tst-cleanup-nostart-stop-gc.out
+ cmp $^ > $@; \
+ $(evaluate-test)
+$(objpfx)tst-cleanup-nostart-stop-gc.o: tst-cleanup.c
+ $(compile.c) -o $@
+
$(objpfx)test-freopen.out: test-freopen.sh $(objpfx)test-freopen
$(SHELL) $< $(common-objpfx) '$(test-program-prefix)' \
$(common-objpfx)libio/; \
diff --git a/libio/tst-cleanup.c b/libio/tst-cleanup.c
new file mode 100644
index 0000000000..7f0a34a91e
--- /dev/null
+++ b/libio/tst-cleanup.c
@@ -0,0 +1,33 @@
+/* Copyright (C) 2021 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/>. */
+
+/* Test that stdout is flushed after atexit callbacks were run. */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+void
+hook (void)
+{
+ puts ("hello");
+}
+
+int
+main (void)
+{
+ atexit (hook);
+}
More information about the Libc-alpha
mailing list