public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
@ 2015-05-06 20:35 Roland McGrath
  2015-05-07 19:59 ` H.J. Lu
  2015-05-08 11:44 ` Szabolcs Nagy
  0 siblings, 2 replies; 10+ messages in thread
From: Roland McGrath @ 2015-05-06 20:35 UTC (permalink / raw)
  To: GNU C. Library

I just filed this bug and committed this test case for it.  I've marked
both flavors of the test as XFAIL.  On ARM, both flavors fail (wrong
alignment).  On x86-64 the dynamically-linked test works right but the
statically-linked test crashes in startup.  I suspect other machines are
affected as well and that the bug is not actually machine-dependent.

I will probably try to look into this next week, but it would be lovely
if someone else wants to take a crack at it first.


Thanks,
Roland



	[BZ #18383]
	* elf/tst-tlsalign.c: New file.
	* elf/tst-tlsalign-static.c: New file.
	* elf/tst-tlsalign-lib.c: New file.
	* elf/Makefile [$(build-shared) = yes] (tests): Add tst-tlsalign.
	(tests-static): Add tst-tlsalign-static.
	(modules-names): Add tst-tlsalign-lib.
	(test-xfail-tst-tlsalign): New variable.
	(test-xfail-tst-tlsalign-static): New variable.

diff --git a/elf/Makefile b/elf/Makefile
index e852b5f..34450ea 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -121,7 +121,7 @@ tests = tst-tls1 tst-tls2 tst-tls9 tst-leaks1 \
 	tst-auxv
 tests-static = tst-tls1-static tst-tls2-static tst-stackguard1-static \
 	       tst-leaks1-static tst-array1-static tst-array5-static \
-	       tst-ptrguard1-static tst-dl-iter-static
+	       tst-ptrguard1-static tst-dl-iter-static tst-tlsalign-static
 ifeq (yes,$(build-shared))
 tests-static += tst-tls9-static
 tst-tls9-static-ENV = \
@@ -146,7 +146,7 @@ tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-stackguard1 tst-addr1 tst-thrlock \
 	 tst-unique1 tst-unique2 $(if $(CXX),tst-unique3 tst-unique4) \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
-	 tst-ptrguard1
+	 tst-ptrguard1 tst-tlsalign
 #	 reldep9
 ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-dlopen-aout
@@ -212,7 +212,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-initorder2a tst-initorder2b tst-initorder2c \
 		tst-initorder2d \
 		tst-relsort1mod1 tst-relsort1mod2 tst-array2dep \
-		tst-array5dep tst-null-argv-lib
+		tst-array5dep tst-null-argv-lib \
+		tst-tlsalign-lib
 ifeq (yes,$(have-protected-data))
 modules-names += tst-protected1moda tst-protected1modb
 tests += tst-protected1a tst-protected1b
@@ -525,6 +526,11 @@ $(objpfx)tst-initordera3.so: $(objpfx)tst-initorderb2.so $(objpfx)tst-initorderb
 $(objpfx)tst-initordera4.so: $(objpfx)tst-initordera3.so
 $(objpfx)tst-initorder: $(objpfx)tst-initordera4.so $(objpfx)tst-initordera1.so $(objpfx)tst-initorderb2.so
 $(objpfx)tst-null-argv: $(objpfx)tst-null-argv-lib.so
+$(objpfx)tst-tlsalign: $(objpfx)tst-tlsalign-lib.so
+
+# BZ#18383: broken on at least ARM (both) and x86-64 (static only).
+test-xfail-tst-tlsalign = yes
+test-xfail-tst-tlsalign-static = yes
 
 tst-null-argv-ENV = LD_DEBUG=all LD_DEBUG_OUTPUT=$(objpfx)tst-null-argv.debug.out
 LDFLAGS-nodel2mod3.so = $(no-as-needed)
diff --git a/elf/tst-tlsalign-lib.c b/elf/tst-tlsalign-lib.c
new file mode 100644
index 0000000..4371e58
--- /dev/null
+++ b/elf/tst-tlsalign-lib.c
@@ -0,0 +1,6 @@
+__thread int mod_tdata1 = 1;
+__thread int mod_tdata2 __attribute__ ((aligned (0x10))) = 2;
+__thread int mod_tdata3 __attribute__ ((aligned (0x1000))) = 4;
+__thread int mod_tbss1;
+__thread int mod_tbss2 __attribute__ ((aligned (0x10)));
+__thread int mod_tbss3 __attribute__ ((aligned (0x1000)));
diff --git a/elf/tst-tlsalign-static.c b/elf/tst-tlsalign-static.c
new file mode 100644
index 0000000..1671abf
--- /dev/null
+++ b/elf/tst-tlsalign-static.c
@@ -0,0 +1,2 @@
+#define NO_LIB
+#include "tst-tlsalign.c"
diff --git a/elf/tst-tlsalign.c b/elf/tst-tlsalign.c
new file mode 100644
index 0000000..da04f57
--- /dev/null
+++ b/elf/tst-tlsalign.c
@@ -0,0 +1,85 @@
+/* Test for large alignment in TLS blocks, BZ#18383.
+   Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static __thread int tdata1 = 1;
+static __thread int tdata2 __attribute__ ((aligned (0x10))) = 2;
+static __thread int tdata3 __attribute__ ((aligned (0x1000))) = 4;
+static __thread int tbss1;
+static __thread int tbss2 __attribute__ ((aligned (0x10)));
+static __thread int tbss3 __attribute__ ((aligned (0x1000)));
+
+#ifndef NO_LIB
+extern __thread int mod_tdata1;
+extern __thread int mod_tdata2;
+extern __thread int mod_tdata3;
+extern __thread int mod_tbss1;
+extern __thread int mod_tbss2;
+extern __thread int mod_tbss3;
+#endif
+
+static int
+test_one (const char *which, unsigned int alignment, int *var, int value)
+{
+  uintptr_t addr = (uintptr_t) var;
+  unsigned int misalign = addr & (alignment - 1);
+
+  printf ("%s TLS address %p %% %u = %u\n",
+          which, (void *) var, alignment, misalign);
+
+  int got = *var;
+  if (got != value)
+    {
+      printf ("%s value %d should be %d\n", which, got, value);
+      return 1;
+    }
+
+  return misalign != 0;
+}
+
+static int
+do_test (void)
+{
+  int fail = 0;
+
+  fail |= test_one ("tdata1", 4, &tdata1, 1);
+  fail |= test_one ("tdata2", 0x10, &tdata2, 2);
+  fail |= test_one ("tdata3", 0x1000, &tdata3, 4);
+
+  fail |= test_one ("tbss1", 4, &tbss1, 0);
+  fail |= test_one ("tbss2", 0x10, &tbss2, 0);
+  fail |= test_one ("tbss3", 0x1000, &tbss3, 0);
+
+#ifndef NO_LIB
+  fail |= test_one ("mod_tdata1", 4, &mod_tdata1, 1);
+  fail |= test_one ("mod_tdata2", 0x10, &mod_tdata2, 2);
+  fail |= test_one ("mod_tdata3", 0x1000, &mod_tdata3, 4);
+
+  fail |= test_one ("mod_tbss1", 4, &mod_tbss1, 0);
+  fail |= test_one ("mod_tbss2", 0x10, &mod_tbss2, 0);
+  fail |= test_one ("mod_tbss3", 0x1000, &mod_tbss3, 0);
+#endif
+
+  return fail ? EXIT_FAILURE : EXIT_SUCCESS;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-06 20:35 [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks Roland McGrath
@ 2015-05-07 19:59 ` H.J. Lu
  2015-05-07 20:15   ` Roland McGrath
  2015-05-08 11:44 ` Szabolcs Nagy
  1 sibling, 1 reply; 10+ messages in thread
From: H.J. Lu @ 2015-05-07 19:59 UTC (permalink / raw)
  To: Roland McGrath; +Cc: GNU C. Library

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On Wed, May 6, 2015 at 1:35 PM, Roland McGrath <roland@hack.frob.com> wrote:
> I just filed this bug and committed this test case for it.  I've marked
> both flavors of the test as XFAIL.  On ARM, both flavors fail (wrong
> alignment).  On x86-64 the dynamically-linked test works right but the
> statically-linked test crashes in startup.  I suspect other machines are
> affected as well and that the bug is not actually machine-dependent.
>
> I will probably try to look into this next week, but it would be lovely
> if someone else wants to take a crack at it first.
>

It is a typo.  I am testing the following patch on ia32, x86-64 and x32
which define TLS_TCB_AT_TP.

Can someone please test it on aarch64, arm and powerpc, which
define TLS_DTV_AT_TP?

Thanks.

-- 
H.J.
---

[-- Attachment #2: static-tls.patch --]
[-- Type: text/x-patch, Size: 761 bytes --]

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 64d1779..d797ef4 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -138,10 +138,10 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
      to request some surplus that permits dynamic loading of modules with
      IE-model TLS.  */
 #if TLS_TCB_AT_TP
-  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
+  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);
   tlsblock = __sbrk (tcb_offset + tcbsize + max_align);
 #elif TLS_DTV_AT_TP
-  tcb_offset = roundup (tcbsize, align ?: 1);
+  tcb_offset = roundup (tcbsize, max_align ?: 1);
   tlsblock = __sbrk (tcb_offset + memsz + max_align
 		     + TLS_PRE_TCB_SIZE + GL(dl_tls_static_size));
   tlsblock += TLS_PRE_TCB_SIZE;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-07 19:59 ` H.J. Lu
@ 2015-05-07 20:15   ` Roland McGrath
  2015-05-07 20:30     ` H.J. Lu
  0 siblings, 1 reply; 10+ messages in thread
From: Roland McGrath @ 2015-05-07 20:15 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C. Library

Note that your change only affects static linking, while the bug on ARM
affects dynamic linking as well.

On arm-linux-gnueabihf (test runs under qemu), your change did not fix
tst-tlsalign-static and it caused these regressions:

+FAIL: elf/ifuncmain1picstatic
+FAIL: elf/ifuncmain1static
+FAIL: elf/ifuncmain2picstatic
+FAIL: elf/ifuncmain2static
+FAIL: elf/ifuncmain4picstatic
+FAIL: elf/ifuncmain4static
+FAIL: elf/ifuncmain5picstatic
+FAIL: elf/ifuncmain5static
+FAIL: elf/ifuncmain7picstatic
+FAIL: elf/ifuncmain7static
+FAIL: elf/tst-array1-static
+FAIL: elf/tst-array1-static-cmp
+FAIL: elf/tst-array5-static
+FAIL: elf/tst-array5-static-cmp
+FAIL: elf/tst-dl-iter-static
+FAIL: elf/tst-leaks1-static
+FAIL: elf/tst-tls1-static
+FAIL: elf/tst-tls2-static
+FAIL: elf/tst-tls9-static

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-07 20:15   ` Roland McGrath
@ 2015-05-07 20:30     ` H.J. Lu
  2015-05-07 21:10       ` Roland McGrath
  0 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu @ 2015-05-07 20:30 UTC (permalink / raw)
  To: Roland McGrath; +Cc: GNU C. Library

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Thu, May 7, 2015 at 1:15 PM, Roland McGrath <roland@hack.frob.com> wrote:
> Note that your change only affects static linking, while the bug on ARM
> affects dynamic linking as well.
>
> On arm-linux-gnueabihf (test runs under qemu), your change did not fix
> tst-tlsalign-static and it caused these regressions:
>

Someone needs to investigate TLS_DTV_AT_TP targets.  Here is a patch
for TLS_TCB_AT_TP targets.  OK for master?


-- 
H.J.
----
We need to align TCB offset to the maximum alignment for TLS_TCB_AT_TP
targets.

* csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
maximum alignment for TLS_TCB_AT_TP targets.

[-- Attachment #2: 0001-Fix-a-typo-in-__libc_setup_tls.patch --]
[-- Type: text/x-patch, Size: 1038 bytes --]

From dbb8cf8d1d0acdaa9384c995b18f50083ba8f4c9 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 7 May 2015 13:26:34 -0700
Subject: [PATCH] Fix a typo in __libc_setup_tls

We need to align TCB offset to the maximum alignment for TLS_TCB_AT_TP
targets.

	* csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
	maximum alignment for TLS_TCB_AT_TP targets.
---
 csu/libc-tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 64d1779..eead735 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -138,7 +138,7 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
      to request some surplus that permits dynamic loading of modules with
      IE-model TLS.  */
 #if TLS_TCB_AT_TP
-  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
+  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);
   tlsblock = __sbrk (tcb_offset + tcbsize + max_align);
 #elif TLS_DTV_AT_TP
   tcb_offset = roundup (tcbsize, align ?: 1);
-- 
1.9.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-07 20:30     ` H.J. Lu
@ 2015-05-07 21:10       ` Roland McGrath
  2015-05-07 22:30         ` H.J. Lu
  0 siblings, 1 reply; 10+ messages in thread
From: Roland McGrath @ 2015-05-07 21:10 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C. Library

The log entry needs the BZ# marker.  

I'm having a hard time finding the calculation in the dynamic-linking case
that corresponds to this piece of __libc_setup_tls.  Since there are two
places where the logic should match, we really should make it much more
clear how they relate.  If there isn't any obvious refactoring that would
share more of this logic between the two cases, can you at least add a
comment in __libc_setup_tls that points to the implementation of the
matching logic in the dynamic-linking case?


Thanks,
Roland

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-07 21:10       ` Roland McGrath
@ 2015-05-07 22:30         ` H.J. Lu
  2015-06-10  0:13           ` Roland McGrath
  0 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu @ 2015-05-07 22:30 UTC (permalink / raw)
  To: Roland McGrath; +Cc: GNU C. Library

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On Thu, May 7, 2015 at 2:10 PM, Roland McGrath <roland@hack.frob.com> wrote:
> The log entry needs the BZ# marker.
>
> I'm having a hard time finding the calculation in the dynamic-linking case
> that corresponds to this piece of __libc_setup_tls.  Since there are two
> places where the logic should match, we really should make it much more
> clear how they relate.  If there isn't any obvious refactoring that would
> share more of this logic between the two cases, can you at least add a
> comment in __libc_setup_tls that points to the implementation of the
> matching logic in the dynamic-linking case?
>

Here is the updated patch.


-- 
H.J.
--
We need to align TCB offset to the maximum alignment for TLS_TCB_AT_TP
targets, similar to what _dl_allocate_tls_storage in elf/dl-tls.c does.

[BZ #18383]
* csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
maximum alignment for TLS_TCB_AT_TP targets.

[-- Attachment #2: 0001-Align-TCB-offset-to-the-maximum-alignment.patch --]
[-- Type: text/x-patch, Size: 1254 bytes --]

From bfa0232943937b861a482aeed9cbe9f2d90e181e Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 7 May 2015 13:26:34 -0700
Subject: [PATCH] Align TCB offset to the maximum alignment

We need to align TCB offset to the maximum alignment for TLS_TCB_AT_TP
targets, similar to what _dl_allocate_tls_storage in elf/dl-tls.c does.

	[BZ #18383]
	* csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
	maximum alignment for TLS_TCB_AT_TP targets.
---
 csu/libc-tls.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 64d1779..452c1b6 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -138,7 +138,9 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
      to request some surplus that permits dynamic loading of modules with
      IE-model TLS.  */
 #if TLS_TCB_AT_TP
-  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
+  /* Align the TCB offset to the maximum alignment, similar to what
+     _dl_allocate_tls_storage in elf/dl-tls.c does.  */
+  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);
   tlsblock = __sbrk (tcb_offset + tcbsize + max_align);
 #elif TLS_DTV_AT_TP
   tcb_offset = roundup (tcbsize, align ?: 1);
-- 
1.9.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-06 20:35 [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks Roland McGrath
  2015-05-07 19:59 ` H.J. Lu
@ 2015-05-08 11:44 ` Szabolcs Nagy
  1 sibling, 0 replies; 10+ messages in thread
From: Szabolcs Nagy @ 2015-05-08 11:44 UTC (permalink / raw)
  To: Roland McGrath, GNU C. Library

On 06/05/15 21:35, Roland McGrath wrote:
> I just filed this bug and committed this test case for it.  I've marked
> both flavors of the test as XFAIL.  On ARM, both flavors fail (wrong
> alignment).  On x86-64 the dynamically-linked test works right but the
> statically-linked test crashes in startup.  I suspect other machines are
> affected as well and that the bug is not actually machine-dependent.

I ran the new tests on AArch64 and they XPASS.

$ ./testrun.sh elf/tst-tlsalign
tdata1 TLS address 0x7fa5a4f000 % 4 = 0
tdata2 TLS address 0x7fa5a4f010 % 16 = 0
tdata3 TLS address 0x7fa5a50000 % 4096 = 0
tbss1 TLS address 0x7fa5a51000 % 4 = 0
tbss2 TLS address 0x7fa5a51010 % 16 = 0
tbss3 TLS address 0x7fa5a52000 % 4096 = 0
mod_tdata1 TLS address 0x7fa5a53014 % 4 = 0
mod_tdata2 TLS address 0x7fa5a53010 % 16 = 0
mod_tdata3 TLS address 0x7fa5a53000 % 4096 = 0
mod_tbss1 TLS address 0x7fa5a54014 % 4 = 0
mod_tbss2 TLS address 0x7fa5a54010 % 16 = 0
mod_tbss3 TLS address 0x7fa5a54000 % 4096 = 0
$ elf/tst-tlsalign-static
tdata1 TLS address 0x30ad000 % 4 = 0
tdata2 TLS address 0x30ad010 % 16 = 0
tdata3 TLS address 0x30ae000 % 4096 = 0
tbss1 TLS address 0x30af000 % 4 = 0
tbss2 TLS address 0x30af010 % 16 = 0
tbss3 TLS address 0x30b0000 % 4096 = 0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-05-07 22:30         ` H.J. Lu
@ 2015-06-10  0:13           ` Roland McGrath
  2015-06-25 12:23             ` H.J. Lu
  0 siblings, 1 reply; 10+ messages in thread
From: Roland McGrath @ 2015-06-10  0:13 UTC (permalink / raw)
  To: H.J. Lu; +Cc: GNU C. Library

> 	[BZ #18383]
> 	* csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
> 	maximum alignment for TLS_TCB_AT_TP targets.

	* csu/libc-tls.c (__libc_setup_tls) [TLS_TCB_AT_TP]:
	Align TCB_OFFSET to MAX_ALIGN, not just TCBALIGN.  Add comment.

> @@ -138,7 +138,9 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
>       to request some surplus that permits dynamic loading of modules with
>       IE-model TLS.  */
>  #if TLS_TCB_AT_TP
> -  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
> +  /* Align the TCB offset to the maximum alignment, similar to what
> +     _dl_allocate_tls_storage in elf/dl-tls.c does.  */
> +  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);

This would be yet more clear if it said:

  /* Align the TCB offset to the maximum alignment, as
     _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign
     and dl_tls_static_align.  */

The patch should also remove the XFAIL for tst-align-extern-static
and update the comment on the XFAILs for tst-tlsalign{,-static}
not to say that x86 is broken.

OK with those details.


Thanks,
Roland

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-06-10  0:13           ` Roland McGrath
@ 2015-06-25 12:23             ` H.J. Lu
  2015-07-09 17:59               ` Siddhesh Poyarekar
  0 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu @ 2015-06-25 12:23 UTC (permalink / raw)
  To: Roland McGrath; +Cc: GNU C. Library

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On Tue, Jun 9, 2015 at 4:34 PM, Roland McGrath <roland@hack.frob.com> wrote:
>>       [BZ #18383]
>>       * csu/libc-tls.c (__libc_setup_tls): Align TCB offset to the
>>       maximum alignment for TLS_TCB_AT_TP targets.
>
>         * csu/libc-tls.c (__libc_setup_tls) [TLS_TCB_AT_TP]:
>         Align TCB_OFFSET to MAX_ALIGN, not just TCBALIGN.  Add comment.
>
>> @@ -138,7 +138,9 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
>>       to request some surplus that permits dynamic loading of modules with
>>       IE-model TLS.  */
>>  #if TLS_TCB_AT_TP
>> -  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
>> +  /* Align the TCB offset to the maximum alignment, similar to what
>> +     _dl_allocate_tls_storage in elf/dl-tls.c does.  */
>> +  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);
>
> This would be yet more clear if it said:
>
>   /* Align the TCB offset to the maximum alignment, as
>      _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign
>      and dl_tls_static_align.  */
>
> The patch should also remove the XFAIL for tst-align-extern-static
> and update the comment on the XFAILs for tst-tlsalign{,-static}
> not to say that x86 is broken.
>
> OK with those details.

This is what I checked in.


-- 
H.J.

[-- Attachment #2: 0001-Align-TCB-offset-to-the-maximum-alignment.patch --]
[-- Type: text/x-patch, Size: 2976 bytes --]

From a7fcc2f8edb26e4d54b6a740aaa3f3bb0caebd14 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 7 May 2015 13:26:34 -0700
Subject: [PATCH] Align TCB offset to the maximum alignment

We need to align TCB offset to the maximum alignment for TLS_TCB_AT_TP
targets, as _dl_allocate_tls_storage (in elf/dl-tls.c) does using
__libc_memalign and dl_tls_static_align.

	[BZ #18383]
	* csu/libc-tls.c (__libc_setup_tls) [TLS_TCB_AT_TP]: Align
	TCB_OFFSET to MAX_ALIGN, not just TCBALIGN.  Add comment.
	* elf/Makefile (test-xfail-tst-tlsalign{,-static}): Remove
	comment for i386/x86-64.
	(test-xfail-tst-tlsalign-extern-static): Removed.
---
 ChangeLog      | 9 +++++++++
 csu/libc-tls.c | 5 ++++-
 elf/Makefile   | 5 +----
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 97fbe48..eb44829 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2015-06-24  H.J. Lu  <hongjiu.lu@intel.com>
+
+	[BZ #18383]
+	* csu/libc-tls.c (__libc_setup_tls) [TLS_TCB_AT_TP]: Align
+	TCB_OFFSET to MAX_ALIGN, not just TCBALIGN.  Add comment.
+	* elf/Makefile (test-xfail-tst-tlsalign{,-static}): Remove
+	comment for i386/x86-64.
+	(test-xfail-tst-tlsalign-extern-static): Removed.
+
 2015-06-24  Joseph Myers  <joseph@codesourcery.com>
 
 	* math/test-double.h: New file.
diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 64d1779..3f13425 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -138,7 +138,10 @@ __libc_setup_tls (size_t tcbsize, size_t tcbalign)
      to request some surplus that permits dynamic loading of modules with
      IE-model TLS.  */
 #if TLS_TCB_AT_TP
-  tcb_offset = roundup (memsz + GL(dl_tls_static_size), tcbalign);
+  /* Align the TCB offset to the maximum alignment, as
+     _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign
+     and dl_tls_static_align.  */
+  tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align);
   tlsblock = __sbrk (tcb_offset + tcbsize + max_align);
 #elif TLS_DTV_AT_TP
   tcb_offset = roundup (tcbsize, align ?: 1);
diff --git a/elf/Makefile b/elf/Makefile
index 871cb4f..4ea3ccf 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -523,16 +523,13 @@ $(objpfx)tst-initorder: $(objpfx)tst-initordera4.so $(objpfx)tst-initordera1.so
 $(objpfx)tst-null-argv: $(objpfx)tst-null-argv-lib.so
 $(objpfx)tst-tlsalign: $(objpfx)tst-tlsalign-lib.so
 
-# BZ#18383: broken on at least ARM (both) and i386/x86-64 (static only).
+# BZ#18383: broken on at least ARM (both).
 test-xfail-tst-tlsalign = yes
 test-xfail-tst-tlsalign-static = yes
 
 $(objpfx)tst-tlsalign-extern: $(objpfx)tst-tlsalign-vars.o
 $(objpfx)tst-tlsalign-extern-static: $(objpfx)tst-tlsalign-vars.o
 
-# BZ#18383: broken on at least i386/x86-64 (static only).
-test-xfail-tst-tlsalign-extern-static = yes
-
 tst-null-argv-ENV = LD_DEBUG=all LD_DEBUG_OUTPUT=$(objpfx)tst-null-argv.debug.out
 LDFLAGS-nodel2mod3.so = $(no-as-needed)
 LDFLAGS-reldepmod5.so = $(no-as-needed)
-- 
1.9.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks.
  2015-06-25 12:23             ` H.J. Lu
@ 2015-07-09 17:59               ` Siddhesh Poyarekar
  0 siblings, 0 replies; 10+ messages in thread
From: Siddhesh Poyarekar @ 2015-07-09 17:59 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Roland McGrath, GNU C. Library

On Wed, Jun 24, 2015 at 04:31:51PM -0700, H.J. Lu wrote:
> -# BZ#18383: broken on at least ARM (both) and i386/x86-64 (static only).
> +# BZ#18383: broken on at least ARM (both).
>  test-xfail-tst-tlsalign = yes
>  test-xfail-tst-tlsalign-static = yes
>  

Shouldn't these be removed too?  They're now XPASSing on my builds.

Siddhesh

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-07-09 17:59 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06 20:35 [COMMITTED PATCH] BZ#18383: Add test case for large alignment in TLS blocks Roland McGrath
2015-05-07 19:59 ` H.J. Lu
2015-05-07 20:15   ` Roland McGrath
2015-05-07 20:30     ` H.J. Lu
2015-05-07 21:10       ` Roland McGrath
2015-05-07 22:30         ` H.J. Lu
2015-06-10  0:13           ` Roland McGrath
2015-06-25 12:23             ` H.J. Lu
2015-07-09 17:59               ` Siddhesh Poyarekar
2015-05-08 11:44 ` Szabolcs Nagy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).