tst-tlsopt-powerpc as a shared lib
Commit Message
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.
* 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.
Comments
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.
@@ -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
new file mode 100644
@@ -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;
+}
@@ -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); \
@@ -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>