mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] [RFC] cleanup/refactor pthread_arch.h, pthread_impl.h
@ 2020-08-25 16:43 Rich Felker
  2020-08-26 21:52 ` Bartosz Brachaczek
  0 siblings, 1 reply; 3+ messages in thread
From: Rich Felker @ 2020-08-25 16:43 UTC (permalink / raw)
  To: musl

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

The attached patch series refactors fragments from pthread_arch.h to
reduce redundancy and allow pthread_arch.h to be included at the top
of pthread_impl.h, where it can be used to influence the
definition/layout of struct __pthread, rather than the middle of the
file. This makes it possible to get rid of the duplicate canary and
dtv members that existed just to match multiple ABIs silumtaneously.

This involved a good deal of manual conversion/deduplication so it's
possible there are bugs for some archs. That's why I've posted the
series for review rather than just pushing. Reports of success/failure
(disassembly of pthread_self.o before/after probably suffice to
confirm no regression) would be very helpful.

Once this is upstream I may try to further clean up struct __pthread,
possibly moving "important" fields like tid to be near end for
TLS_ABOVE_TP archs to ensure that small negative offsets suffice to
access them.

Rich

[-- Attachment #2: 0001-deduplicate-TP_ADJ-logic-out-of-each-arch-replace-wi.patch --]
[-- Type: text/plain, Size: 7647 bytes --]

From ea71a9004e08030a15d45186e263fd2b0c51cc25 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Mon, 24 Aug 2020 22:04:52 -0400
Subject: [PATCH 1/3] deduplicate TP_ADJ logic out of each arch, replace with
 TP_OFFSET

the only part of TP_ADJ that was not uniquely determined by
TLS_ABOVE_TP was the 0x7000 adjustment used mainly on mips and powerpc
variants.
---
 arch/aarch64/pthread_arch.h    |  1 -
 arch/arm/pthread_arch.h        |  1 -
 arch/i386/pthread_arch.h       |  2 --
 arch/m68k/pthread_arch.h       |  2 +-
 arch/microblaze/pthread_arch.h |  2 --
 arch/mips/pthread_arch.h       |  2 +-
 arch/mips64/pthread_arch.h     |  2 +-
 arch/mipsn32/pthread_arch.h    |  2 +-
 arch/or1k/pthread_arch.h       |  1 -
 arch/powerpc/pthread_arch.h    |  2 +-
 arch/powerpc64/pthread_arch.h  |  2 +-
 arch/riscv64/pthread_arch.h    |  1 -
 arch/s390x/pthread_arch.h      |  2 --
 arch/sh/pthread_arch.h         |  1 -
 arch/x32/pthread_arch.h        |  2 --
 arch/x86_64/pthread_arch.h     |  2 --
 src/internal/pthread_impl.h    | 10 ++++++++++
 17 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/arch/aarch64/pthread_arch.h b/arch/aarch64/pthread_arch.h
index e64b126d..f3c005c7 100644
--- a/arch/aarch64/pthread_arch.h
+++ b/arch/aarch64/pthread_arch.h
@@ -7,6 +7,5 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 16
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread))
 
 #define MC_PC pc
diff --git a/arch/arm/pthread_arch.h b/arch/arm/pthread_arch.h
index e689ea21..48640985 100644
--- a/arch/arm/pthread_arch.h
+++ b/arch/arm/pthread_arch.h
@@ -28,6 +28,5 @@ static inline pthread_t __pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 8
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread))
 
 #define MC_PC arm_pc
diff --git a/arch/i386/pthread_arch.h b/arch/i386/pthread_arch.h
index 6f600b9e..32570a17 100644
--- a/arch/i386/pthread_arch.h
+++ b/arch/i386/pthread_arch.h
@@ -5,6 +5,4 @@ static inline struct pthread *__pthread_self()
 	return self;
 }
 
-#define TP_ADJ(p) (p)
-
 #define MC_PC gregs[REG_EIP]
diff --git a/arch/m68k/pthread_arch.h b/arch/m68k/pthread_arch.h
index 02d5b8a0..7c9990c2 100644
--- a/arch/m68k/pthread_arch.h
+++ b/arch/m68k/pthread_arch.h
@@ -6,8 +6,8 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 #define MC_PC gregs[R_PC]
diff --git a/arch/microblaze/pthread_arch.h b/arch/microblaze/pthread_arch.h
index f6ba8de9..c327f4eb 100644
--- a/arch/microblaze/pthread_arch.h
+++ b/arch/microblaze/pthread_arch.h
@@ -5,6 +5,4 @@ static inline struct pthread *__pthread_self()
 	return self;
 }
 
-#define TP_ADJ(p) (p)
-
 #define MC_PC regs.pc
diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
index 1e7839ea..c22eb34d 100644
--- a/arch/mips/pthread_arch.h
+++ b/arch/mips/pthread_arch.h
@@ -12,8 +12,8 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 #define MC_PC pc
diff --git a/arch/mips64/pthread_arch.h b/arch/mips64/pthread_arch.h
index 1e7839ea..c22eb34d 100644
--- a/arch/mips64/pthread_arch.h
+++ b/arch/mips64/pthread_arch.h
@@ -12,8 +12,8 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 #define MC_PC pc
diff --git a/arch/mipsn32/pthread_arch.h b/arch/mipsn32/pthread_arch.h
index 1e7839ea..c22eb34d 100644
--- a/arch/mipsn32/pthread_arch.h
+++ b/arch/mipsn32/pthread_arch.h
@@ -12,8 +12,8 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 #define MC_PC pc
diff --git a/arch/or1k/pthread_arch.h b/arch/or1k/pthread_arch.h
index 1b806f89..76d0a8bc 100644
--- a/arch/or1k/pthread_arch.h
+++ b/arch/or1k/pthread_arch.h
@@ -13,6 +13,5 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread))
 
 #define MC_PC regs.pc
diff --git a/arch/powerpc/pthread_arch.h b/arch/powerpc/pthread_arch.h
index ae0f28d6..9697046b 100644
--- a/arch/powerpc/pthread_arch.h
+++ b/arch/powerpc/pthread_arch.h
@@ -7,8 +7,8 @@ static inline struct pthread *__pthread_self()
                         
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 // the kernel calls the ip "nip", it's the first saved value after the 32
diff --git a/arch/powerpc64/pthread_arch.h b/arch/powerpc64/pthread_arch.h
index 79c3ecd8..e9dba43f 100644
--- a/arch/powerpc64/pthread_arch.h
+++ b/arch/powerpc64/pthread_arch.h
@@ -7,8 +7,8 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define TP_OFFSET 0x7000
 #define DTP_OFFSET 0x8000
 
 // the kernel calls the ip "nip", it's the first saved value after the 32
diff --git a/arch/riscv64/pthread_arch.h b/arch/riscv64/pthread_arch.h
index db414b17..50f0868d 100644
--- a/arch/riscv64/pthread_arch.h
+++ b/arch/riscv64/pthread_arch.h
@@ -7,7 +7,6 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 0
-#define TP_ADJ(p) ((char *)p + sizeof(struct pthread))
 
 #define DTP_OFFSET 0x800
 
diff --git a/arch/s390x/pthread_arch.h b/arch/s390x/pthread_arch.h
index e2251f1f..5d22546b 100644
--- a/arch/s390x/pthread_arch.h
+++ b/arch/s390x/pthread_arch.h
@@ -9,6 +9,4 @@ static inline struct pthread *__pthread_self()
 	return self;
 }
 
-#define TP_ADJ(p) (p)
-
 #define MC_PC psw.addr
diff --git a/arch/sh/pthread_arch.h b/arch/sh/pthread_arch.h
index 3ee9c1a9..c2252908 100644
--- a/arch/sh/pthread_arch.h
+++ b/arch/sh/pthread_arch.h
@@ -7,7 +7,6 @@ static inline struct pthread *__pthread_self()
 
 #define TLS_ABOVE_TP
 #define GAP_ABOVE_TP 8
-#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread))
 
 #define MC_PC sc_pc
 
diff --git a/arch/x32/pthread_arch.h b/arch/x32/pthread_arch.h
index f640a1a1..fa452839 100644
--- a/arch/x32/pthread_arch.h
+++ b/arch/x32/pthread_arch.h
@@ -5,8 +5,6 @@ static inline struct pthread *__pthread_self()
 	return self;
 }
 
-#define TP_ADJ(p) (p)
-
 #define MC_PC gregs[REG_RIP]
 
 #define CANARY canary2
diff --git a/arch/x86_64/pthread_arch.h b/arch/x86_64/pthread_arch.h
index 65e880c6..1c64a840 100644
--- a/arch/x86_64/pthread_arch.h
+++ b/arch/x86_64/pthread_arch.h
@@ -5,6 +5,4 @@ static inline struct pthread *__pthread_self()
 	return self;
 }
 
-#define TP_ADJ(p) (p)
-
 #define MC_PC gregs[REG_RIP]
diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
index 5749a336..3c2bd767 100644
--- a/src/internal/pthread_impl.h
+++ b/src/internal/pthread_impl.h
@@ -105,10 +105,20 @@ struct __timer {
 #define CANARY canary
 #endif
 
+#ifndef TP_OFFSET
+#define TP_OFFSET 0
+#endif
+
 #ifndef DTP_OFFSET
 #define DTP_OFFSET 0
 #endif
 
+#ifdef TLS_ABOVE_TP
+#define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + TP_OFFSET)
+#else
+#define TP_ADJ(p) (p)
+#endif
+
 #ifndef tls_mod_off_t
 #define tls_mod_off_t size_t
 #endif
-- 
2.21.0


[-- Attachment #3: 0002-deduplicate-__pthread_self-thread-pointer-adjustment.patch --]
[-- Type: text/plain, Size: 10629 bytes --]

From 8b31da3578af2b46908d1a8145c1e5a5f3f6fbc0 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Mon, 24 Aug 2020 22:23:08 -0400
Subject: [PATCH 2/3] deduplicate __pthread_self thread pointer adjustment out
 of each arch

the adjustment made is entirely a function of TLS_ABOVE_TP and
TP_OFFSET. aside from avoiding repetition of the TP_OFFSET value and
arithmetic, this change makes pthread_arch.h independent of the
definition of struct __pthread from pthread_impl.h. this in turn will
allow inclusion of pthread_arch.h to be moved to the top of
pthread_impl.h so that it can influence the definition of the
structure.
---
 arch/aarch64/pthread_arch.h    |  8 ++++----
 arch/arm/pthread_arch.h        | 16 ++++++++--------
 arch/i386/pthread_arch.h       |  8 ++++----
 arch/m68k/pthread_arch.h       |  5 ++---
 arch/microblaze/pthread_arch.h |  8 ++++----
 arch/mips/pthread_arch.h       |  8 ++++----
 arch/mips64/pthread_arch.h     |  8 ++++----
 arch/mipsn32/pthread_arch.h    |  8 ++++----
 arch/or1k/pthread_arch.h       |  9 ++++-----
 arch/powerpc/pthread_arch.h    |  6 +++---
 arch/powerpc64/pthread_arch.h  |  6 +++---
 arch/riscv64/pthread_arch.h    |  4 ++--
 arch/s390x/pthread_arch.h      |  8 ++++----
 arch/sh/pthread_arch.h         |  8 ++++----
 arch/x32/pthread_arch.h        |  8 ++++----
 arch/x86_64/pthread_arch.h     |  8 ++++----
 src/internal/pthread_impl.h    |  2 ++
 17 files changed, 64 insertions(+), 64 deletions(-)

diff --git a/arch/aarch64/pthread_arch.h b/arch/aarch64/pthread_arch.h
index f3c005c7..3909616c 100644
--- a/arch/aarch64/pthread_arch.h
+++ b/arch/aarch64/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	char *self;
-	__asm__ ("mrs %0,tpidr_el0" : "=r"(self));
-	return (void*)(self - sizeof(struct pthread));
+	uintptr_t tp;
+	__asm__ ("mrs %0,tpidr_el0" : "=r"(tp));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/arm/pthread_arch.h b/arch/arm/pthread_arch.h
index 48640985..157e2eae 100644
--- a/arch/arm/pthread_arch.h
+++ b/arch/arm/pthread_arch.h
@@ -1,11 +1,11 @@
 #if ((__ARM_ARCH_6K__ || __ARM_ARCH_6KZ__ || __ARM_ARCH_6ZK__) && !__thumb__) \
  || __ARM_ARCH_7A__ || __ARM_ARCH_7R__ || __ARM_ARCH >= 7
 
-static inline pthread_t __pthread_self()
+static inline uintptr_t __get_tp()
 {
-	char *p;
-	__asm__ ( "mrc p15,0,%0,c13,c0,3" : "=r"(p) );
-	return (void *)(p-sizeof(struct pthread));
+	uintptr_t tp;
+	__asm__ ( "mrc p15,0,%0,c13,c0,3" : "=r"(tp) );
+	return tp;
 }
 
 #else
@@ -16,12 +16,12 @@ static inline pthread_t __pthread_self()
 #define BLX "blx"
 #endif
 
-static inline pthread_t __pthread_self()
+static inline uintptr_t __get_tp()
 {
 	extern hidden uintptr_t __a_gettp_ptr;
-	register uintptr_t p __asm__("r0");
-	__asm__ ( BLX " %1" : "=r"(p) : "r"(__a_gettp_ptr) : "cc", "lr" );
-	return (void *)(p-sizeof(struct pthread));
+	register uintptr_t tp __asm__("r0");
+	__asm__ ( BLX " %1" : "=r"(tp) : "r"(__a_gettp_ptr) : "cc", "lr" );
+	return tp;
 }
 
 #endif
diff --git a/arch/i386/pthread_arch.h b/arch/i386/pthread_arch.h
index 32570a17..a639c382 100644
--- a/arch/i386/pthread_arch.h
+++ b/arch/i386/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	struct pthread *self;
-	__asm__ ("movl %%gs:0,%0" : "=r" (self) );
-	return self;
+	uintptr_t tp;
+	__asm__ ("movl %%gs:0,%0" : "=r" (tp) );
+	return tp;
 }
 
 #define MC_PC gregs[REG_EIP]
diff --git a/arch/m68k/pthread_arch.h b/arch/m68k/pthread_arch.h
index 7c9990c2..5bea4e1a 100644
--- a/arch/m68k/pthread_arch.h
+++ b/arch/m68k/pthread_arch.h
@@ -1,7 +1,6 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	uintptr_t tp = __syscall(SYS_get_thread_area);
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return __syscall(SYS_get_thread_area);
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/microblaze/pthread_arch.h b/arch/microblaze/pthread_arch.h
index c327f4eb..ff26624e 100644
--- a/arch/microblaze/pthread_arch.h
+++ b/arch/microblaze/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	struct pthread *self;
-	__asm__ ("ori %0, r21, 0" : "=r" (self) );
-	return self;
+	uintptr_t tp;
+	__asm__ ("ori %0, r21, 0" : "=r" (tp) );
+	return tp;
 }
 
 #define MC_PC regs.pc
diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
index c22eb34d..c45347ab 100644
--- a/arch/mips/pthread_arch.h
+++ b/arch/mips/pthread_arch.h
@@ -1,13 +1,13 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
 #if __mips_isa_rev < 2
-	register char *tp __asm__("$3");
+	register uintptr_t tp __asm__("$3");
 	__asm__ (".word 0x7c03e83b" : "=r" (tp) );
 #else
-	char *tp;
+	uintptr_t tp;
 	__asm__ ("rdhwr %0, $29" : "=r" (tp) );
 #endif
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/mips64/pthread_arch.h b/arch/mips64/pthread_arch.h
index c22eb34d..c45347ab 100644
--- a/arch/mips64/pthread_arch.h
+++ b/arch/mips64/pthread_arch.h
@@ -1,13 +1,13 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
 #if __mips_isa_rev < 2
-	register char *tp __asm__("$3");
+	register uintptr_t tp __asm__("$3");
 	__asm__ (".word 0x7c03e83b" : "=r" (tp) );
 #else
-	char *tp;
+	uintptr_t tp;
 	__asm__ ("rdhwr %0, $29" : "=r" (tp) );
 #endif
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/mipsn32/pthread_arch.h b/arch/mipsn32/pthread_arch.h
index c22eb34d..c45347ab 100644
--- a/arch/mipsn32/pthread_arch.h
+++ b/arch/mipsn32/pthread_arch.h
@@ -1,13 +1,13 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
 #if __mips_isa_rev < 2
-	register char *tp __asm__("$3");
+	register uintptr_t tp __asm__("$3");
 	__asm__ (".word 0x7c03e83b" : "=r" (tp) );
 #else
-	char *tp;
+	uintptr_t tp;
 	__asm__ ("rdhwr %0, $29" : "=r" (tp) );
 #endif
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/or1k/pthread_arch.h b/arch/or1k/pthread_arch.h
index 76d0a8bc..f75ea7e4 100644
--- a/arch/or1k/pthread_arch.h
+++ b/arch/or1k/pthread_arch.h
@@ -1,14 +1,13 @@
-/* or1k use variant I, but with the twist that tp points to the end of TCB */
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
 #ifdef __clang__
-	char *tp;
+	uintptr_t tp;
 	__asm__ ("l.ori %0, r10, 0" : "=r" (tp) );
 #else
-	register char *tp __asm__("r10");
+	register uintptr_t tp __asm__("r10");
 	__asm__ ("" : "=r" (tp) );
 #endif
-	return (struct pthread *) (tp - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/powerpc/pthread_arch.h b/arch/powerpc/pthread_arch.h
index 9697046b..a0947763 100644
--- a/arch/powerpc/pthread_arch.h
+++ b/arch/powerpc/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	register char *tp __asm__("r2");
+	register uintptr_t tp __asm__("r2");
 	__asm__ ("" : "=r" (tp) );
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return tp;
 }
                         
 #define TLS_ABOVE_TP
diff --git a/arch/powerpc64/pthread_arch.h b/arch/powerpc64/pthread_arch.h
index e9dba43f..08a557d2 100644
--- a/arch/powerpc64/pthread_arch.h
+++ b/arch/powerpc64/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	register char *tp __asm__("r13");
+	register uintptr_t tp __asm__("r13");
 	__asm__ ("" : "=r" (tp) );
-	return (pthread_t)(tp - 0x7000 - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/riscv64/pthread_arch.h b/arch/riscv64/pthread_arch.h
index 50f0868d..d0bc87c4 100644
--- a/arch/riscv64/pthread_arch.h
+++ b/arch/riscv64/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
 	char *tp;
 	__asm__ __volatile__("mv %0, tp" : "=r"(tp));
-	return (void *)(tp - sizeof(struct pthread));
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/s390x/pthread_arch.h b/arch/s390x/pthread_arch.h
index 5d22546b..e54fec3f 100644
--- a/arch/s390x/pthread_arch.h
+++ b/arch/s390x/pthread_arch.h
@@ -1,12 +1,12 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	struct pthread *self;
+	uintptr_t tp;
 	__asm__ (
 		"ear  %0, %%a0\n"
 		"sllg %0, %0, 32\n"
 		"ear  %0, %%a1\n"
-		: "=r"(self));
-	return self;
+		: "=r"(tp));
+	return tp;
 }
 
 #define MC_PC psw.addr
diff --git a/arch/sh/pthread_arch.h b/arch/sh/pthread_arch.h
index c2252908..0fcf70d2 100644
--- a/arch/sh/pthread_arch.h
+++ b/arch/sh/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	char *self;
-	__asm__ ("stc gbr,%0" : "=r" (self) );
-	return (struct pthread *) (self - sizeof(struct pthread));
+	uintptr_t tp;
+	__asm__ ("stc gbr,%0" : "=r" (tp) );
+	return tp;
 }
 
 #define TLS_ABOVE_TP
diff --git a/arch/x32/pthread_arch.h b/arch/x32/pthread_arch.h
index fa452839..6e2495da 100644
--- a/arch/x32/pthread_arch.h
+++ b/arch/x32/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	struct pthread *self;
-	__asm__ ("mov %%fs:0,%0" : "=r" (self) );
-	return self;
+	uintptr_t tp;
+	__asm__ ("mov %%fs:0,%0" : "=r" (tp) );
+	return tp;
 }
 
 #define MC_PC gregs[REG_RIP]
diff --git a/arch/x86_64/pthread_arch.h b/arch/x86_64/pthread_arch.h
index 1c64a840..c8c63f2e 100644
--- a/arch/x86_64/pthread_arch.h
+++ b/arch/x86_64/pthread_arch.h
@@ -1,8 +1,8 @@
-static inline struct pthread *__pthread_self()
+static inline uintptr_t __get_tp()
 {
-	struct pthread *self;
-	__asm__ ("mov %%fs:0,%0" : "=r" (self) );
-	return self;
+	uintptr_t tp;
+	__asm__ ("mov %%fs:0,%0" : "=r" (tp) );
+	return tp;
 }
 
 #define MC_PC gregs[REG_RIP]
diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
index 3c2bd767..58e06136 100644
--- a/src/internal/pthread_impl.h
+++ b/src/internal/pthread_impl.h
@@ -115,8 +115,10 @@ struct __timer {
 
 #ifdef TLS_ABOVE_TP
 #define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + TP_OFFSET)
+#define __pthread_self() ((pthread_t)(__get_tp() - sizeof(struct __pthread) - TP_OFFSET))
 #else
 #define TP_ADJ(p) (p)
+#define __pthread_self() ((pthread_t)__get_tp())
 #endif
 
 #ifndef tls_mod_off_t
-- 
2.21.0


[-- Attachment #4: 0003-remove-redundant-pthread-struct-members-repeated-for.patch --]
[-- Type: text/plain, Size: 4898 bytes --]

From b2f6f2863f73d87b6783aa76c95a4f3eecc8b815 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Mon, 24 Aug 2020 22:45:51 -0400
Subject: [PATCH 3/3] remove redundant pthread struct members repeated for
 layout purposes

dtv_copy, canary2, and canary_at_end existed solely to match multiple
ABI and asm-accessed layouts simultaneously. now that pthread_arch.h
can be included before struct __pthread is defined, the struct layout
can depend on macros defined by pthread_arch.h.
---
 arch/powerpc/pthread_arch.h   |  2 --
 arch/powerpc64/pthread_arch.h |  2 --
 arch/x32/pthread_arch.h       |  2 +-
 ldso/dynlink.c                |  2 +-
 src/env/__init_tls.c          |  2 +-
 src/env/__stack_chk_fail.c    |  2 +-
 src/internal/pthread_impl.h   | 23 ++++++++++++++---------
 src/thread/pthread_create.c   |  2 +-
 8 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/pthread_arch.h b/arch/powerpc/pthread_arch.h
index a0947763..42e88b07 100644
--- a/arch/powerpc/pthread_arch.h
+++ b/arch/powerpc/pthread_arch.h
@@ -14,5 +14,3 @@ static inline uintptr_t __get_tp()
 // the kernel calls the ip "nip", it's the first saved value after the 32
 // GPRs.
 #define MC_PC gregs[32]
-
-#define CANARY canary_at_end
diff --git a/arch/powerpc64/pthread_arch.h b/arch/powerpc64/pthread_arch.h
index 08a557d2..1b7b9079 100644
--- a/arch/powerpc64/pthread_arch.h
+++ b/arch/powerpc64/pthread_arch.h
@@ -14,5 +14,3 @@ static inline uintptr_t __get_tp()
 // the kernel calls the ip "nip", it's the first saved value after the 32
 // GPRs.
 #define MC_PC gp_regs[32]
-
-#define CANARY canary_at_end
diff --git a/arch/x32/pthread_arch.h b/arch/x32/pthread_arch.h
index 6e2495da..c1e7716d 100644
--- a/arch/x32/pthread_arch.h
+++ b/arch/x32/pthread_arch.h
@@ -7,6 +7,6 @@ static inline uintptr_t __get_tp()
 
 #define MC_PC gregs[REG_RIP]
 
-#define CANARY canary2
+#define CANARY_PAD
 
 #define tls_mod_off_t unsigned long long
diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index d3d4ddd2..f7474743 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1579,7 +1579,7 @@ static void install_new_tls(void)
 
 	/* Install new dtv for each thread. */
 	for (j=0, td=self; !j || td!=self; j++, td=td->next) {
-		td->dtv = td->dtv_copy = newdtv[j];
+		td->dtv = newdtv[j];
 	}
 
 	__tl_unlock();
diff --git a/src/env/__init_tls.c b/src/env/__init_tls.c
index 772baba3..a93141ed 100644
--- a/src/env/__init_tls.c
+++ b/src/env/__init_tls.c
@@ -67,7 +67,7 @@ void *__copy_tls(unsigned char *mem)
 	}
 #endif
 	dtv[0] = libc.tls_cnt;
-	td->dtv = td->dtv_copy = dtv;
+	td->dtv = dtv;
 	return td;
 }
 
diff --git a/src/env/__stack_chk_fail.c b/src/env/__stack_chk_fail.c
index e32596d1..bf5a280a 100644
--- a/src/env/__stack_chk_fail.c
+++ b/src/env/__stack_chk_fail.c
@@ -9,7 +9,7 @@ void __init_ssp(void *entropy)
 	if (entropy) memcpy(&__stack_chk_guard, entropy, sizeof(uintptr_t));
 	else __stack_chk_guard = (uintptr_t)&__stack_chk_guard * 1103515245;
 
-	__pthread_self()->CANARY = __stack_chk_guard;
+	__pthread_self()->canary = __stack_chk_guard;
 }
 
 void __stack_chk_fail(void)
diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
index 58e06136..4d709bbc 100644
--- a/src/internal/pthread_impl.h
+++ b/src/internal/pthread_impl.h
@@ -11,16 +11,25 @@
 #include "atomic.h"
 #include "futex.h"
 
+#include "pthread_arch.h"
+
 #define pthread __pthread
 
 struct pthread {
 	/* Part 1 -- these fields may be external or
 	 * internal (accessed via asm) ABI. Do not change. */
 	struct pthread *self;
+#ifndef TLS_ABOVE_TP
 	uintptr_t *dtv;
+#endif
 	struct pthread *prev, *next; /* non-ABI */
 	uintptr_t sysinfo;
-	uintptr_t canary, canary2;
+#ifndef TLS_ABOVE_TP
+#ifdef CANARY_PAD
+	uintptr_t canary_pad;
+#endif
+	uintptr_t canary;
+#endif
 
 	/* Part 2 -- implementation details, non-ABI. */
 	int tid;
@@ -52,8 +61,10 @@ struct pthread {
 
 	/* Part 3 -- the positions of these fields relative to
 	 * the end of the structure is external and internal ABI. */
-	uintptr_t canary_at_end;
-	uintptr_t *dtv_copy;
+#ifdef TLS_ABOVE_TP
+	uintptr_t canary;
+	uintptr_t *dtv;
+#endif
 };
 
 enum {
@@ -99,12 +110,6 @@ struct __timer {
 #define _b_waiters2 __u.__vi[4]
 #define _b_inst __u.__p[3]
 
-#include "pthread_arch.h"
-
-#ifndef CANARY
-#define CANARY canary
-#endif
-
 #ifndef TP_OFFSET
 #define TP_OFFSET 0
 #endif
diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index 10f1b7d8..55744155 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -314,7 +314,7 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
 		new->detach_state = DT_JOINABLE;
 	}
 	new->robust_list.head = &new->robust_list.head;
-	new->CANARY = self->CANARY;
+	new->canary = self->canary;
 	new->sysinfo = self->sysinfo;
 
 	/* Setup argument structure for the new thread on its stack.
-- 
2.21.0


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

* Re: [musl] [PATCH] [RFC] cleanup/refactor pthread_arch.h, pthread_impl.h
  2020-08-25 16:43 [musl] [PATCH] [RFC] cleanup/refactor pthread_arch.h, pthread_impl.h Rich Felker
@ 2020-08-26 21:52 ` Bartosz Brachaczek
  2020-08-27 22:38   ` Rich Felker
  0 siblings, 1 reply; 3+ messages in thread
From: Bartosz Brachaczek @ 2020-08-26 21:52 UTC (permalink / raw)
  To: musl

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

On Tue, Aug 25, 2020 at 6:43 PM Rich Felker <dalias@libc.org> wrote:

> The attached patch series refactors fragments from pthread_arch.h to
> reduce redundancy and allow pthread_arch.h to be included at the top
> of pthread_impl.h, where it can be used to influence the
> definition/layout of struct __pthread, rather than the middle of the
> file. This makes it possible to get rid of the duplicate canary and
> dtv members that existed just to match multiple ABIs silumtaneously.
>
> This involved a good deal of manual conversion/deduplication so it's
> possible there are bugs for some archs. That's why I've posted the
> series for review rather than just pushing. Reports of success/failure
> (disassembly of pthread_self.o before/after probably suffice to
> confirm no regression) would be very helpful.
>

I mechanically confirmed for all archs that with the first two patches
applied, disassembly of pthread_self.o before/after doesn't change. The
third patch obviously changes the offset for most archs due to change in
sizeof struct pthread.

Only other remark: ricv64's __get_tp misses the change of the variable type
from char * to uintptr_t.

[-- Attachment #2: Type: text/html, Size: 1555 bytes --]

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

* Re: [musl] [PATCH] [RFC] cleanup/refactor pthread_arch.h, pthread_impl.h
  2020-08-26 21:52 ` Bartosz Brachaczek
@ 2020-08-27 22:38   ` Rich Felker
  0 siblings, 0 replies; 3+ messages in thread
From: Rich Felker @ 2020-08-27 22:38 UTC (permalink / raw)
  To: musl

On Wed, Aug 26, 2020 at 11:52:46PM +0200, Bartosz Brachaczek wrote:
> On Tue, Aug 25, 2020 at 6:43 PM Rich Felker <dalias@libc.org> wrote:
> 
> > The attached patch series refactors fragments from pthread_arch.h to
> > reduce redundancy and allow pthread_arch.h to be included at the top
> > of pthread_impl.h, where it can be used to influence the
> > definition/layout of struct __pthread, rather than the middle of the
> > file. This makes it possible to get rid of the duplicate canary and
> > dtv members that existed just to match multiple ABIs silumtaneously.
> >
> > This involved a good deal of manual conversion/deduplication so it's
> > possible there are bugs for some archs. That's why I've posted the
> > series for review rather than just pushing. Reports of success/failure
> > (disassembly of pthread_self.o before/after probably suffice to
> > confirm no regression) would be very helpful.
> >
> 
> I mechanically confirmed for all archs that with the first two patches
> applied, disassembly of pthread_self.o before/after doesn't change. The
> third patch obviously changes the offset for most archs due to change in
> sizeof struct pthread.
> 
> Only other remark: ricv64's __get_tp misses the change of the variable type
> from char * to uintptr_t.

Thanks -- that's very helpful. It also raises a point that we should
probably add -Werror=int-conversion to the default CFLAGS. As far as I
can tell it only catches invalid (constraint-violation) implicit
conversions like the one you found, and does not have any
style-policing component to it.

Rich

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

end of thread, other threads:[~2020-08-27 22:39 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-25 16:43 [musl] [PATCH] [RFC] cleanup/refactor pthread_arch.h, pthread_impl.h Rich Felker
2020-08-26 21:52 ` Bartosz Brachaczek
2020-08-27 22:38   ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

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 NNTP newsgroup(s).