[RFC] cleanup/refactor pthread_arch.h, pthread_impl.h

Submitted by Rich Felker on Aug. 25, 2020, 4:43 p.m.

Details

Message ID 20200825164316.GQ3265@brightrain.aerifal.cx
State New
Series "cleanup/refactor pthread_arch.h, pthread_impl.h"
Headers show

Commit Message

Rich Felker Aug. 25, 2020, 4:43 p.m.
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
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(-)

Patch hide | download patch | download mbox

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

Comments

Bartosz Brachaczek Aug. 26, 2020, 9:52 p.m.
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.
Rich Felker Aug. 27, 2020, 10:38 p.m.
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