Re: CVS commit: src/lib/libc/tls
>>> Joerg Sonnenberger wrote > > On Fri, Nov 22, 2019 at 08:53:25AM +0900, Takeshi Nakayama wrote: > > >>> Takeshi Nakayama wrote > > > > > >>> Joerg Sonnenberger wrote > > > > > > > On Thu, Nov 21, 2019 at 11:06:16PM +, Takeshi Nakayama wrote: > > > > > Module Name: src > > > > > Committed By: nakayama > > > > > Date: Thu Nov 21 23:06:16 UTC 2019 > > > > > > > > > > Modified Files: > > > > > src/lib/libc/tls: tls.c > > > > > > > > > > Log Message: > > > > > Fix PR/54074 and PR/54093 completely. > > > > > > > > > > More similar to the ld.elf_so logic, it is necessary to align with > > > > > p_align first. Also, invert the #ifdef condition for consistency. > > > > > > > > This commit just wastes space without reason. > > > > > > No, without this commit the TLS offset calculation is mismatch if > > > alignof(max_align_t)) != p_align. > > > > Ah, this is wrong. > > Correctly, the TLS offset calculation is mismatch with what ld(1) > > expected if p_memsz is not aligned with p_align. > > For TLS variant I, it literally just adds padding at the end of the > allocation. For TLS variant II, it is redundant, because the rounding is > already done in with max_align_t. We do not support p_align > malloc > alignment and the patch is certainly nowhere near enough to change that. > It is actively harmful in that it can make people believe that it could > ever work. I don't want to do anything about the case of p_align > malloc alignment. I just want to fix an usual 32-bit sparc statically linked binaries not working properly. The new jemalloc now uses TLS, and the 32-bit sparc statically linked binary ELF header looks like this: % readelf -l rescue/sh | egrep 'MemSiz|TLS' Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align TLS0x6dc000 0x0075c000 0x0075c000 0x00b10 0x00b14 R 0x8 The point is when MemSiz (p_memsz) is not aligned with Align (p_align) as above. In tls.c rev 1.13: tls_size = p_memsz --> 0x00b14 tls_allocation = roundup2(tls_size, alignof(max_align_t)) --> 0x00b18 tcb = calloc() + tls_allocation --> calloc() + 0x00b18 p = tcb - tls_size --> calloc() + 4 memcpy(p, tls_initaddr, tls_initsize) So, the TLS initialization image is copied to calloc() + 4. However, ld(1) assumes that the TLS area starts with: tcb - roundup2(p_memsz, p_align) = tcb - 0x00b18 --> calloc() Since this is 4 bytes different, the initial value of the thread local variable is not set correctly. To fix this problem, I added the same thing as this line in ld.elf_so: https://nxr.netbsd.org/xref/src/libexec/ld.elf_so/tls.c#236 * obj->tlssize = p_memsz, obj->tlsalign = p_align -- Takeshi Nakayama
Re: CVS commit: src/lib/libc/tls
On Fri, Nov 22, 2019 at 08:53:25AM +0900, Takeshi Nakayama wrote: > >>> Takeshi Nakayama wrote > > > >>> Joerg Sonnenberger wrote > > > > > On Thu, Nov 21, 2019 at 11:06:16PM +, Takeshi Nakayama wrote: > > > > Module Name:src > > > > Committed By: nakayama > > > > Date: Thu Nov 21 23:06:16 UTC 2019 > > > > > > > > Modified Files: > > > > src/lib/libc/tls: tls.c > > > > > > > > Log Message: > > > > Fix PR/54074 and PR/54093 completely. > > > > > > > > More similar to the ld.elf_so logic, it is necessary to align with > > > > p_align first. Also, invert the #ifdef condition for consistency. > > > > > > This commit just wastes space without reason. > > > > No, without this commit the TLS offset calculation is mismatch if > > alignof(max_align_t)) != p_align. > > Ah, this is wrong. > Correctly, the TLS offset calculation is mismatch with what ld(1) > expected if p_memsz is not aligned with p_align. For TLS variant I, it literally just adds padding at the end of the allocation. For TLS variant II, it is redundant, because the rounding is already done in with max_align_t. We do not support p_align > malloc alignment and the patch is certainly nowhere near enough to change that. It is actively harmful in that it can make people believe that it could ever work. Joerg
Re: CVS commit: src/lib/libc/tls
>>> Takeshi Nakayama wrote > >>> Joerg Sonnenberger wrote > > > On Thu, Nov 21, 2019 at 11:06:16PM +, Takeshi Nakayama wrote: > > > Module Name: src > > > Committed By: nakayama > > > Date: Thu Nov 21 23:06:16 UTC 2019 > > > > > > Modified Files: > > > src/lib/libc/tls: tls.c > > > > > > Log Message: > > > Fix PR/54074 and PR/54093 completely. > > > > > > More similar to the ld.elf_so logic, it is necessary to align with > > > p_align first. Also, invert the #ifdef condition for consistency. > > > > This commit just wastes space without reason. > > No, without this commit the TLS offset calculation is mismatch if > alignof(max_align_t)) != p_align. Ah, this is wrong. Correctly, the TLS offset calculation is mismatch with what ld(1) expected if p_memsz is not aligned with p_align. -- Takeshi Nakayama
Re: CVS commit: src/lib/libc/tls
>>> Joerg Sonnenberger wrote > On Thu, Nov 21, 2019 at 11:06:16PM +, Takeshi Nakayama wrote: > > Module Name:src > > Committed By: nakayama > > Date: Thu Nov 21 23:06:16 UTC 2019 > > > > Modified Files: > > src/lib/libc/tls: tls.c > > > > Log Message: > > Fix PR/54074 and PR/54093 completely. > > > > More similar to the ld.elf_so logic, it is necessary to align with > > p_align first. Also, invert the #ifdef condition for consistency. > > This commit just wastes space without reason. No, without this commit the TLS offset calculation is mismatch if alignof(max_align_t)) != p_align. -- Takeshi Nakayama
Re: CVS commit: src/lib/libc/tls
On Thu, Nov 21, 2019 at 11:06:16PM +, Takeshi Nakayama wrote: > Module Name: src > Committed By: nakayama > Date: Thu Nov 21 23:06:16 UTC 2019 > > Modified Files: > src/lib/libc/tls: tls.c > > Log Message: > Fix PR/54074 and PR/54093 completely. > > More similar to the ld.elf_so logic, it is necessary to align with > p_align first. Also, invert the #ifdef condition for consistency. This commit just wastes space without reason. Joerg
CVS commit: src/lib/libc/tls
Module Name:src Committed By: nakayama Date: Thu Nov 21 23:06:16 UTC 2019 Modified Files: src/lib/libc/tls: tls.c Log Message: Fix PR/54074 and PR/54093 completely. More similar to the ld.elf_so logic, it is necessary to align with p_align first. Also, invert the #ifdef condition for consistency. Should fix regression for static linking binaries: http://releng.netbsd.org/b5reports/sparc/commits-2019.11.html#2019.11.10.23.39.03 http://releng.netbsd.org/b5reports/sparc64/commits-2019.11.html#2019.11.16.04.10.33 To generate a diff of this commit: cvs rdiff -u -r1.12 -r1.13 src/lib/libc/tls/tls.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
CVS commit: src/lib/libc/tls
Module Name:src
Committed By: nakayama
Date: Thu Nov 21 23:06:16 UTC 2019
Modified Files:
src/lib/libc/tls: tls.c
Log Message:
Fix PR/54074 and PR/54093 completely.
More similar to the ld.elf_so logic, it is necessary to align with
p_align first. Also, invert the #ifdef condition for consistency.
Should fix regression for static linking binaries:
http://releng.netbsd.org/b5reports/sparc/commits-2019.11.html#2019.11.10.23.39.03
http://releng.netbsd.org/b5reports/sparc64/commits-2019.11.html#2019.11.16.04.10.33
To generate a diff of this commit:
cvs rdiff -u -r1.12 -r1.13 src/lib/libc/tls/tls.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/lib/libc/tls/tls.c
diff -u src/lib/libc/tls/tls.c:1.12 src/lib/libc/tls/tls.c:1.13
--- src/lib/libc/tls/tls.c:1.12 Thu Nov 7 22:25:21 2019
+++ src/lib/libc/tls/tls.c Thu Nov 21 23:06:15 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $ */
+/* $NetBSD: tls.c,v 1.13 2019/11/21 23:06:15 nakayama Exp $ */
/*-
* Copyright (c) 2011 The NetBSD Foundation, Inc.
@@ -30,7 +30,7 @@
*/
#include
-__RCSID("$NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $");
+__RCSID("$NetBSD: tls.c,v 1.13 2019/11/21 23:06:15 nakayama Exp $");
#include "namespace.h"
@@ -85,10 +85,10 @@ _rtld_tls_allocate(void)
uint8_t *p;
if (initial_thread_tcb == NULL) {
-#ifdef __HAVE_TLS_VARIANT_II
- tls_allocation = roundup2(tls_size, alignof(max_align_t));
-#else
+#ifdef __HAVE_TLS_VARIANT_I
tls_allocation = tls_size;
+#else
+ tls_allocation = roundup2(tls_size, alignof(max_align_t));
#endif
initial_thread_tcb = p = mmap(NULL,
@@ -152,7 +152,11 @@ __libc_static_tls_setup_cb(struct dl_phd
continue;
tls_initaddr = (void *)(phdr->p_vaddr + data->dlpi_addr);
tls_initsize = phdr->p_filesz;
+#ifdef __HAVE_TLS_VARIANT_I
tls_size = phdr->p_memsz;
+#else
+ tls_size = roundup2(phdr->p_memsz, phdr->p_align);
+#endif
}
return 0;
}
CVS commit: src/lib/libc/tls
Module Name:src
Committed By: joerg
Date: Thu Nov 7 22:25:22 UTC 2019
Modified Files:
src/lib/libc/tls: tls.c
Log Message:
Mirror the ld.elf_so logic for handling aligning the TLS size.
Most noticable, recompute the start of the TLS area for variant I
relative to the TCB. This makes a difference when the segment size and
base alignment don't agree.
To generate a diff of this commit:
cvs rdiff -u -r1.11 -r1.12 src/lib/libc/tls/tls.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/lib/libc/tls/tls.c
diff -u src/lib/libc/tls/tls.c:1.11 src/lib/libc/tls/tls.c:1.12
--- src/lib/libc/tls/tls.c:1.11 Tue Nov 5 22:22:42 2019
+++ src/lib/libc/tls/tls.c Thu Nov 7 22:25:21 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: tls.c,v 1.11 2019/11/05 22:22:42 joerg Exp $ */
+/* $NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $ */
/*-
* Copyright (c) 2011 The NetBSD Foundation, Inc.
@@ -30,7 +30,7 @@
*/
#include
-__RCSID("$NetBSD: tls.c,v 1.11 2019/11/05 22:22:42 joerg Exp $");
+__RCSID("$NetBSD: tls.c,v 1.12 2019/11/07 22:25:21 joerg Exp $");
#include "namespace.h"
@@ -86,14 +86,16 @@ _rtld_tls_allocate(void)
if (initial_thread_tcb == NULL) {
#ifdef __HAVE_TLS_VARIANT_II
- tls_size = roundup2(tls_size, alignof(max_align_t));
+ tls_allocation = roundup2(tls_size, alignof(max_align_t));
+#else
+ tls_allocation = tls_size;
#endif
- tls_allocation = tls_size + sizeof(*tcb);
- initial_thread_tcb = p = mmap(NULL, tls_allocation,
- PROT_READ | PROT_WRITE, MAP_ANON, -1, 0);
+ initial_thread_tcb = p = mmap(NULL,
+ tls_allocation + sizeof(*tcb), PROT_READ | PROT_WRITE,
+ MAP_ANON, -1, 0);
} else {
- p = calloc(1, tls_allocation);
+ p = calloc(1, tls_allocation + sizeof(*tcb));
}
if (p == NULL) {
static const char msg[] = "TLS allocation failed, terminating\n";
@@ -106,7 +108,8 @@ _rtld_tls_allocate(void)
p += sizeof(struct tls_tcb);
#else
/* LINTED tls_size is rounded above */
- tcb = (struct tls_tcb *)(p + tls_size);
+ tcb = (struct tls_tcb *)(p + tls_allocation);
+ p = (uint8_t *)tcb - tls_size;
tcb->tcb_self = tcb;
#endif
memcpy(p, tls_initaddr, tls_initsize);
@@ -126,10 +129,10 @@ _rtld_tls_free(struct tls_tcb *tcb)
p = (uint8_t *)tcb;
#else
/* LINTED */
- p = (uint8_t *)tcb - tls_size;
+ p = (uint8_t *)tcb - tls_allocation;
#endif
if (p == initial_thread_tcb)
- munmap(p, tls_allocation);
+ munmap(p, tls_allocation + sizeof(*tcb));
else
free(p);
}
CVS commit: src/lib/libc/tls
Module Name:src Committed By: joerg Date: Thu Nov 7 22:25:22 UTC 2019 Modified Files: src/lib/libc/tls: tls.c Log Message: Mirror the ld.elf_so logic for handling aligning the TLS size. Most noticable, recompute the start of the TLS area for variant I relative to the TCB. This makes a difference when the segment size and base alignment don't agree. To generate a diff of this commit: cvs rdiff -u -r1.11 -r1.12 src/lib/libc/tls/tls.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
