| Message ID | 20200825164316.GQ3265@brightrain.aerifal.cx |
|---|---|
| State | New |
| Series | "cleanup/refactor pthread_arch.h, pthread_impl.h" |
| Headers | show |
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
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.
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
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(-)