| Message ID | 20170730000430.GF1956@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
Alan Modra <amodra@gmail.com> writes: > On Fri, Jul 28, 2017 at 01:07:44PM +0000, Adam Conrad wrote: >> On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote: >> > >> > Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation >> > processing, the body of the test ought to be put in a shared library >> > (*). I cobbled together such a test, and TRY_STATIC_TLS works fine. >> > So, not a powerpc64 glibc bug. >> >> Excellent. Should I expect said cobbled test to replace tst-tlsopt-powerpc >> in glibc trunk soonish (obviously, I'll just XFAIL it for now locally). > > This makes the __tls_get_addr_opt test run as a shared library, and so > actually test that DTPMOD64/DTPREL64 pairs are processed by ld.so to > support the __tls_get_adfr_opt call stub fast return. After a > 2017-01-24 patch (binutils f0158f4416) ld.bfd no longer emitted > unnecessary dynamic relocations against local thread variables, > instead setting up the __tls_index GOT entries for the call stub fast > return. This meant tst-tlsopt-powerpc passed but did not check ld.so > relocation support. After a 2017-07-16 patch (binutils 676ee2b5fa) > ld.bfd no longer set up the __tls_index GOT entries for the call stub > fast return, and tst-tlsopt-powerpc failed. > > Compiling mod-tlsopt-powerpc.c with -DSHARED exposed a bug in > powerpc64/tls-macros.h, which defines a __TLS_GET_ADDR macro that > clashes with one defined in dl-tls.h. The tls-macros.h version is > only used in that file, so delete it and expand. > > Regression tested powerpc64le-linux. Please verify the Makefile > changes. The test passes with "make -j64 check", but I may well have > missed something there. Tested on powerpc-linux and powerpc64-linux too. > * sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from > tst-tlsopt-powerpc.c with function name change and no test harness. > * sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test. > Call tls_get_addr_opt_test. > * sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define. > (modules-names): Add mod-tlsopt-powerpc. > (mod-tlsopt-powerpc.so-no-z-defs): Define. > (tst-tlsopt-powerpc): Depend on .so. > * sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't > define. Expand use in TLS_GD and TLS_LD. Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer it here and close the bug, which was reported to glibc. Looks good to me. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=21847
[cc trimmed] On Mon, Jul 31, 2017 at 01:40:48PM -0300, Tulio Magno Quites Machado Filho wrote: > Alan Modra <amodra@gmail.com> writes: > > > On Fri, Jul 28, 2017 at 01:07:44PM +0000, Adam Conrad wrote: > >> On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote: > >> > > >> > Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation > >> > processing, the body of the test ought to be put in a shared library > >> > (*). I cobbled together such a test, and TRY_STATIC_TLS works fine. > >> > So, not a powerpc64 glibc bug. > >> > >> Excellent. Should I expect said cobbled test to replace tst-tlsopt-powerpc > >> in glibc trunk soonish (obviously, I'll just XFAIL it for now locally). > > > > This makes the __tls_get_addr_opt test run as a shared library, and so > > actually test that DTPMOD64/DTPREL64 pairs are processed by ld.so to > > support the __tls_get_adfr_opt call stub fast return. After a > > 2017-01-24 patch (binutils f0158f4416) ld.bfd no longer emitted > > unnecessary dynamic relocations against local thread variables, > > instead setting up the __tls_index GOT entries for the call stub fast > > return. This meant tst-tlsopt-powerpc passed but did not check ld.so > > relocation support. After a 2017-07-16 patch (binutils 676ee2b5fa) > > ld.bfd no longer set up the __tls_index GOT entries for the call stub > > fast return, and tst-tlsopt-powerpc failed. > > > > Compiling mod-tlsopt-powerpc.c with -DSHARED exposed a bug in > > powerpc64/tls-macros.h, which defines a __TLS_GET_ADDR macro that > > clashes with one defined in dl-tls.h. The tls-macros.h version is > > only used in that file, so delete it and expand. > > > > Regression tested powerpc64le-linux. Please verify the Makefile > > changes. The test passes with "make -j64 check", but I may well have > > missed something there. > > Tested on powerpc-linux and powerpc64-linux too. Thanks! I'll commit this after the 2.26 freeze is over. > > * sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from > > tst-tlsopt-powerpc.c with function name change and no test harness. > > * sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test. > > Call tls_get_addr_opt_test. > > * sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define. > > (modules-names): Add mod-tlsopt-powerpc. > > (mod-tlsopt-powerpc.so-no-z-defs): Define. > > (tst-tlsopt-powerpc): Depend on .so. > > * sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't > > define. Expand use in TLS_GD and TLS_LD. > > Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer > it here and close the bug, which was reported to glibc. > > Looks good to me. > > [1] https://sourceware.org/bugzilla/show_bug.cgi?id=21847 No, 21847 is an entirely different issue. I've moved it from glibc/dynamic-linker to binutils/ld and mentioned the commits on the binutils side that have already fixed the bug.
On Mon, Jul 31, 2017 at 01:40:48PM -0300, Tulio Magno Quites Machado Filho wrote: > > Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer > it here and close the bug, which was reported to glibc. That's a complete unrelated bug, which is addressed here: https://sourceware.org/ml/binutils/2017-07/msg00342.html ... Adam
On 07/30/2017 02:04 AM, Alan Modra wrote: > * sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from > tst-tlsopt-powerpc.c with function name change and no test harness. > * sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test. > Call tls_get_addr_opt_test. > * sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define. > (modules-names): Add mod-tlsopt-powerpc. > (mod-tlsopt-powerpc.so-no-z-defs): Define. > (tst-tlsopt-powerpc): Depend on .so. > * sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't > define. Expand use in TLS_GD and TLS_LD. Should we backport this to the active release branches? Thanks, Florian
On 09/04/2017 04:39 PM, Florian Weimer wrote: > On 07/30/2017 02:04 AM, Alan Modra wrote: >> * sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from >> tst-tlsopt-powerpc.c with function name change and no test harness. >> * sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test. >> Call tls_get_addr_opt_test. >> * sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define. >> (modules-names): Add mod-tlsopt-powerpc. >> (mod-tlsopt-powerpc.so-no-z-defs): Define. >> (tst-tlsopt-powerpc): Depend on .so. >> * sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't >> define. Expand use in TLS_GD and TLS_LD. > > Should we backport this to the active release branches? Yes. Test fixes that make the active branches easier to maintain for the distributions are always a good idea.
diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile index 0d9206b..6aa683b 100644 --- a/sysdeps/powerpc/Makefile +++ b/sysdeps/powerpc/Makefile @@ -8,9 +8,11 @@ sysdep-dl-routines += dl-machine hwcapinfo sysdep_routines += dl-machine hwcapinfo # extra shared linker files to link only into dl-allobjs.so sysdep-rtld-routines += dl-machine hwcapinfo -# Don't optimize GD tls sequence to LE. -LDFLAGS-tst-tlsopt-powerpc += -Wl,--no-tls-optimize + +modules-names += mod-tlsopt-powerpc +mod-tlsopt-powerpc.so-no-z-defs = yes tests += tst-tlsopt-powerpc +$(objpfx)tst-tlsopt-powerpc: $(objpfx)mod-tlsopt-powerpc.so ifneq (no,$(multi-arch)) tests-static += tst-tlsifunc-static diff --git a/sysdeps/powerpc/mod-tlsopt-powerpc.c b/sysdeps/powerpc/mod-tlsopt-powerpc.c new file mode 100644 index 0000000..ee0db12 --- /dev/null +++ b/sysdeps/powerpc/mod-tlsopt-powerpc.c @@ -0,0 +1,49 @@ +/* shared library to test for __tls_get_addr optimization. */ +#include <stdio.h> + +#include "../../elf/tls-macros.h" +#include "dl-tls.h" + +/* common 'int' variable in TLS. */ +COMMON_INT_DEF(foo); + + +int +tls_get_addr_opt_test (void) +{ + int result = 0; + + /* Get variable using general dynamic model. */ + int *ap = TLS_GD (foo); + if (*ap != 0) + { + printf ("foo = %d\n", *ap); + result = 1; + } + + tls_index *tls_arg; +#ifdef __powerpc64__ + register unsigned long thread_pointer __asm__ ("r13"); + asm ("addi %0,2,foo@got@tlsgd" : "=r" (tls_arg)); +#else + register unsigned long thread_pointer __asm__ ("r2"); + asm ("bcl 20,31,1f\n1:\t" + "mflr %0\n\t" + "addis %0,%0,_GLOBAL_OFFSET_TABLE_-1b@ha\n\t" + "addi %0,%0,_GLOBAL_OFFSET_TABLE_-1b@l\n\t" + "addi %0,%0,foo@got@tlsgd" : "=b" (tls_arg)); +#endif + + if (tls_arg->ti_module != 0) + { + printf ("tls_index not optimized, binutils too old?\n"); + result = 1; + } + else if (tls_arg->ti_offset + thread_pointer != (unsigned long) ap) + { + printf ("tls_index->ti_offset wrong value\n"); + result = 1; + } + + return result; +} diff --git a/sysdeps/powerpc/powerpc64/tls-macros.h b/sysdeps/powerpc/powerpc64/tls-macros.h index 42a95ec..79a0b25 100644 --- a/sysdeps/powerpc/powerpc64/tls-macros.h +++ b/sysdeps/powerpc/powerpc64/tls-macros.h @@ -18,13 +18,11 @@ __result; \ }) -#define __TLS_GET_ADDR "__tls_get_addr" - /* PowerPC64 Local Dynamic TLS access. */ #define TLS_LD(x) \ ({ int * __result; \ asm ("addi 3,2," #x "@got@tlsld\n\t" \ - "bl " __TLS_GET_ADDR "\n\t" \ + "bl __tls_get_addr\n\t" \ "nop \n\t" \ "addis %0,3," #x "@dtprel@ha\n\t" \ "addi %0,%0," #x "@dtprel@l" \ @@ -36,7 +34,7 @@ #define TLS_GD(x) \ ({ register int *__result __asm__ ("r3"); \ asm ("addi 3,2," #x "@got@tlsgd\n\t" \ - "bl " __TLS_GET_ADDR "\n\t" \ + "bl __tls_get_addr\n\t" \ "nop " \ : "=r" (__result) : \ : __TLS_CALL_CLOBBERS); \ diff --git a/sysdeps/powerpc/tst-tlsopt-powerpc.c b/sysdeps/powerpc/tst-tlsopt-powerpc.c index 8ae928a..cc682b2 100644 --- a/sysdeps/powerpc/tst-tlsopt-powerpc.c +++ b/sysdeps/powerpc/tst-tlsopt-powerpc.c @@ -1,51 +1,11 @@ /* glibc test for __tls_get_addr optimization. */ -#include <stdio.h> - -#include "../../elf/tls-macros.h" -#include "dl-tls.h" - -/* common 'int' variable in TLS. */ -COMMON_INT_DEF(foo); - static int do_test (void) { - int result = 0; - - /* Get variable using general dynamic model. */ - int *ap = TLS_GD (foo); - if (*ap != 0) - { - printf ("foo = %d\n", *ap); - result = 1; - } - - tls_index *tls_arg; -#ifdef __powerpc64__ - register unsigned long thread_pointer __asm__ ("r13"); - asm ("addi %0,2,foo@got@tlsgd" : "=r" (tls_arg)); -#else - register unsigned long thread_pointer __asm__ ("r2"); - asm ("bcl 20,31,1f\n1:\t" - "mflr %0\n\t" - "addis %0,%0,_GLOBAL_OFFSET_TABLE_-1b@ha\n\t" - "addi %0,%0,_GLOBAL_OFFSET_TABLE_-1b@l\n\t" - "addi %0,%0,foo@got@tlsgd" : "=b" (tls_arg)); -#endif - - if (tls_arg->ti_module != 0) - { - printf ("tls_index not optimized, binutils too old?\n"); - result = 1; - } - else if (tls_arg->ti_offset + thread_pointer != (unsigned long) ap) - { - printf ("tls_index->ti_offset wrong value\n"); - result = 1; - } + extern int tls_get_addr_opt_test (void); - return result; + return tls_get_addr_opt_test (); } #include <support/test-driver.c>