| Message ID | ZCC3g2nKEtpz36nR@mx3210.localdomain |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] hppa: Drop 16-byte pthread lock alignment | expand |
* John David Anglin: > hppa: Drop 16-byte pthread lock alignment > > Linux threads were removed about 12 years ago and the current > nptl implementation only requires 4-byte alignment for pthread > locks. > > The 16-byte alignment causes various issues. For example in > building ignition-msgs, we have: > > /usr/include/google/protobuf/map.h:124:37: error: static assertion failed > 124 | static_assert(alignof(value_type) <= 8, ""); > | ~~~~~~~~~~~~~~~~~~~~^~~~ > > This is caused by the 16-byte pthread lock alignment. This was done deliberately to preserve ABI. This change needs a mass rebuild because struct offsets after pthread_mutex_t members are likely to change.
John David Anglin <dave@parisc-linux.org> writes: > [[PGP Signed Part:Undecided]] > hppa: Drop 16-byte pthread lock alignment > > Linux threads were removed about 12 years ago and the current > nptl implementation only requires 4-byte alignment for pthread > locks. > > The 16-byte alignment causes various issues. For example in > building ignition-msgs, we have: > > /usr/include/google/protobuf/map.h:124:37: error: static assertion failed > 124 | static_assert(alignof(value_type) <= 8, ""); > | ~~~~~~~~~~~~~~~~~~~~^~~~ > > This is caused by the 16-byte pthread lock alignment. Dave will be aware of the context but posting this just for completeness: see also: https://github.com/protocolbuffers/protobuf/issues/9433 where David Seifert (soap@, CC'd) provided some analysis. > > Signed-off-by: John David Anglin <dave.anglin@bell.net> > --- > > diff --git a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > index 999195c5b0..c1a46d66d0 100644 > --- a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > +++ b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > @@ -40,7 +40,7 @@ > #define __SIZEOF_PTHREAD_RWLOCK_T 64 > #define __SIZEOF_PTHREAD_RWLOCKATTR_T 8 > > -#define __LOCK_ALIGNMENT __attribute__ ((__aligned__(16))) > +#define __LOCK_ALIGNMENT > #define __ONCE_ALIGNMENT > > #endif /* bits/pthreadtypes.h */ > diff --git a/sysdeps/hppa/nptl/bits/struct_rwlock.h b/sysdeps/hppa/nptl/bits/struct_rwlock.h > index e83b4aab52..59bc9fe76f 100644 > --- a/sysdeps/hppa/nptl/bits/struct_rwlock.h > +++ b/sysdeps/hppa/nptl/bits/struct_rwlock.h > @@ -25,8 +25,14 @@ struct __pthread_rwlock_arch_t > /* In the old Linuxthreads pthread_rwlock_t, this is the > start of the 4-word 16-byte aligned lock structure. The > next four words are all set to 1 by the Linuxthreads > - PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. */ > - int __compat_padding[4] __attribute__ ((__aligned__(16))); > + PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. > + > + The 16-byte aligned lock stucture causes various pthread > + structures to be over aligned. This causes some builds > + to fail which assume a maximum alignment of 8 bytes. > + Linuxthreads has been removed for 12 years, so drop > + alignment of lock structure. */ > + int __compat_padding[4]; > unsigned int __readers; > unsigned int __writers; > unsigned int __wrphase_futex; > > [[End of PGP Signed Part]]
Florian Weimer <fw@deneb.enyo.de> writes: > * John David Anglin: > >> hppa: Drop 16-byte pthread lock alignment >> >> Linux threads were removed about 12 years ago and the current >> nptl implementation only requires 4-byte alignment for pthread >> locks. >> >> The 16-byte alignment causes various issues. For example in >> building ignition-msgs, we have: >> >> /usr/include/google/protobuf/map.h:124:37: error: static assertion failed >> 124 | static_assert(alignof(value_type) <= 8, ""); >> | ~~~~~~~~~~~~~~~~~~~~^~~~ >> >> This is caused by the 16-byte pthread lock alignment. > > This was done deliberately to preserve ABI. This change needs a mass > rebuild because struct offsets after pthread_mutex_t members are > likely to change. On HPPA, I think we _may_ be fine with ABI breaks, but of course we should use the proper mechanisms for that if we're doing it to advertise it fully and allow distributions and users to prepare. (We've done them before and there's not really any cache of binaries that we need to keep compat. with anyway, it's more important to keep things building usually.) thanks, sam
On 2023-03-26 8:37 p.m., Sam James wrote: > Florian Weimer <fw@deneb.enyo.de> writes: > >> * John David Anglin: >> >>> hppa: Drop 16-byte pthread lock alignment >>> >>> Linux threads were removed about 12 years ago and the current >>> nptl implementation only requires 4-byte alignment for pthread >>> locks. >>> >>> The 16-byte alignment causes various issues. For example in >>> building ignition-msgs, we have: >>> >>> /usr/include/google/protobuf/map.h:124:37: error: static assertion failed >>> 124 | static_assert(alignof(value_type) <= 8, ""); >>> | ~~~~~~~~~~~~~~~~~~~~^~~~ >>> >>> This is caused by the 16-byte pthread lock alignment. >> This was done deliberately to preserve ABI. This change needs a mass >> rebuild because struct offsets after pthread_mutex_t members are >> likely to change. Although the change may change some structure offsets, I don't believe the change requires a mass rebuild. I am running a Debian hppa system with the reduced lock alignment and so far I haven't observed any breakage. I checked this prior to committing the change. Packages in Debian unstable are rebuilt frequently. I don't see that we had much choice. Either we break the ABI or we live with packages that don't work on hppa. Here is comment in map.h: // MapAllocator does not support alignments beyond 8. Technically we should // support up to std::max_align_t, but this fails with ubsan and tcmalloc // debug allocation logic which assume 8 as default alignment. static_assert(alignof(value_type) <= 8, ""); It was the above comment that convinced me that we needed to change the pthread lock alignment. The analysis in the protobuf issue is exactly correct: https://github.com/protocolbuffers/protobuf/issues/9433 Protobuf is not the only package affected by this issue. Regards, Dave
* John David Anglin: > On 2023-03-26 8:37 p.m., Sam James wrote: >> Florian Weimer <fw@deneb.enyo.de> writes: >> >>> * John David Anglin: >>> >>>> hppa: Drop 16-byte pthread lock alignment >>>> >>>> Linux threads were removed about 12 years ago and the current >>>> nptl implementation only requires 4-byte alignment for pthread >>>> locks. >>>> >>>> The 16-byte alignment causes various issues. For example in >>>> building ignition-msgs, we have: >>>> >>>> /usr/include/google/protobuf/map.h:124:37: error: static assertion failed >>>> 124 | static_assert(alignof(value_type) <= 8, ""); >>>> | ~~~~~~~~~~~~~~~~~~~~^~~~ >>>> >>>> This is caused by the 16-byte pthread lock alignment. >>> This was done deliberately to preserve ABI. This change needs a mass >>> rebuild because struct offsets after pthread_mutex_t members are >>> likely to change. > Although the change may change some structure offsets, I don't > believe the change requires a mass rebuild. I am running a Debian > hppa system with the reduced lock alignment and so far I haven't > observed any breakage. I checked this prior to committing the > change. With nothing rebuilt, no struct offsets change, so this isn't unexpected. Maybe it's still the easiest way to fix the underlying issue, but it's likely not going to stop at just changing the glibc headers. > Here is comment in map.h: > // MapAllocator does not support alignments beyond 8. Technically we should > // support up to std::max_align_t, but this fails with ubsan and tcmalloc > // debug allocation logic which assume 8 as default alignment. > static_assert(alignof(value_type) <= 8, ""); > > It was the above comment that convinced me that we needed to change > the pthread lock alignment. It seems that current tcmalloc honors GCC's __STDCPP_DEFAULT_NEW_ALIGNMENT__: <https://github.com/google/tcmalloc/blob/master/tcmalloc/size_classes.cc> On panama.debian.net, it seems correct: $ gcc -x c++ -E - < /dev/null -dM | grep __STDCPP_DEFAULT_NEW_ALIGNMENT__ #define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16 Is it a matter of an outdated bundled tcmalloc copy? Or is the assert merely incorrect and should use __STDCPP_DEFAULT_NEW_ALIGNMENT__ as well?
On 2023-03-26 10:22 p.m., John David Anglin wrote: > + The 16-byte aligned lock stucture causes various pthread > + structures to be over aligned. This causes some builds > + to fail which assume a maximum alignment of 8 bytes. > + Linuxthreads has been removed for 12 years, so drop The "for 12 years" comment will be stale & incorrect already a year from now. Better say something more absolute, like "was removed in 2011" (just guessing, don't really know the year) or be more vague like "a long while ago", or some such.
On 2023-03-27 8:42 a.m., Florian Weimer wrote: >> Here is comment in map.h: >> // MapAllocator does not support alignments beyond 8. Technically we should >> // support up to std::max_align_t, but this fails with ubsan and tcmalloc >> // debug allocation logic which assume 8 as default alignment. >> static_assert(alignof(value_type) <= 8, ""); >> >> It was the above comment that convinced me that we needed to change >> the pthread lock alignment. > It seems that current tcmalloc honors GCC's > __STDCPP_DEFAULT_NEW_ALIGNMENT__: > > <https://github.com/google/tcmalloc/blob/master/tcmalloc/size_classes.cc> Agreed. The current tcmalloc is compiled with c++17. For earlier versions of c++, we have the following issue: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0035r4.html There are many packages in Debian and gentoo that are built with earlier versions of c++ and I don't see that changing. This includes protobuf. As a result, the over alignment of pthread types will cause inconsistencies between different versions of c++ and c on hppa. After discussion, the consensus was to remove the over alignment. > > On panama.debian.net, it seems correct: > > $ gcc -x c++ -E - < /dev/null -dM | grep __STDCPP_DEFAULT_NEW_ALIGNMENT__ > #define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16 That should change to 8 to minimize wasted bytes. The current pthread types do not need 16-byte alignment. Dave
* John David Anglin: > On 2023-03-27 8:42 a.m., Florian Weimer wrote: >>> Here is comment in map.h: >>> // MapAllocator does not support alignments beyond 8. Technically we should >>> // support up to std::max_align_t, but this fails with ubsan and tcmalloc >>> // debug allocation logic which assume 8 as default alignment. >>> static_assert(alignof(value_type) <= 8, ""); >>> >>> It was the above comment that convinced me that we needed to change >>> the pthread lock alignment. >> It seems that current tcmalloc honors GCC's >> __STDCPP_DEFAULT_NEW_ALIGNMENT__: >> >> <https://github.com/google/tcmalloc/blob/master/tcmalloc/size_classes.cc> > Agreed. > > The current tcmalloc is compiled with c++17. For earlier versions of c++, we have the > following issue: > https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0035r4.html > > There are many packages in Debian and gentoo that are built with > earlier versions of c++ and I don't see that changing. This > includes protobuf. Well, GCC nowadays defaults to C++17: $ echo __cplusplus | gcc -x c++ -E - # 0 "<stdin>" # 0 "<built-in>" # 0 "<command-line>" # 1 "/usr/include/stdc-predef.h" 1 3 4 # 0 "<command-line>" 2 # 1 "<stdin>" 201703L This is GCC 12 from Debian bookworm. >> On panama.debian.net, it seems correct: >> >> $ gcc -x c++ -E - < /dev/null -dM | grep __STDCPP_DEFAULT_NEW_ALIGNMENT__ >> #define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16 > That should change to 8 to minimize wasted bytes. The current > pthread types do not need 16-byte alignment. There might be other reasons why 16-byte alignment is needed. But perhaps not on HPPA.
After some discussion, I decided it would be best to revert this change for now. Commit 500054974667be3153ed760152ea0153df33c3d0 reverts " hppa: Drop 16-byte pthread lock alignment" (commits c4468cd3995b4236ea886901109b194641132b08 and ab991a3d1b401ded6bd4f027352da8262b021a11). Dave On 2023-03-26 5:22 p.m., John David Anglin wrote: > hppa: Drop 16-byte pthread lock alignment > > Linux threads were removed about 12 years ago and the current > nptl implementation only requires 4-byte alignment for pthread > locks. > > The 16-byte alignment causes various issues. For example in > building ignition-msgs, we have: > > /usr/include/google/protobuf/map.h:124:37: error: static assertion failed > 124 | static_assert(alignof(value_type) <= 8, ""); > | ~~~~~~~~~~~~~~~~~~~~^~~~ > > This is caused by the 16-byte pthread lock alignment. > > Signed-off-by: John David Anglin <dave.anglin@bell.net> > --- > > diff --git a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > index 999195c5b0..c1a46d66d0 100644 > --- a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > +++ b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h > @@ -40,7 +40,7 @@ > #define __SIZEOF_PTHREAD_RWLOCK_T 64 > #define __SIZEOF_PTHREAD_RWLOCKATTR_T 8 > > -#define __LOCK_ALIGNMENT __attribute__ ((__aligned__(16))) > +#define __LOCK_ALIGNMENT > #define __ONCE_ALIGNMENT > > #endif /* bits/pthreadtypes.h */ > diff --git a/sysdeps/hppa/nptl/bits/struct_rwlock.h b/sysdeps/hppa/nptl/bits/struct_rwlock.h > index e83b4aab52..59bc9fe76f 100644 > --- a/sysdeps/hppa/nptl/bits/struct_rwlock.h > +++ b/sysdeps/hppa/nptl/bits/struct_rwlock.h > @@ -25,8 +25,14 @@ struct __pthread_rwlock_arch_t > /* In the old Linuxthreads pthread_rwlock_t, this is the > start of the 4-word 16-byte aligned lock structure. The > next four words are all set to 1 by the Linuxthreads > - PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. */ > - int __compat_padding[4] __attribute__ ((__aligned__(16))); > + PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. > + > + The 16-byte aligned lock stucture causes various pthread > + structures to be over aligned. This causes some builds > + to fail which assume a maximum alignment of 8 bytes. > + Linuxthreads has been removed for 12 years, so drop > + alignment of lock structure. */ > + int __compat_padding[4]; > unsigned int __readers; > unsigned int __writers; > unsigned int __wrphase_futex;
diff --git a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h index 999195c5b0..c1a46d66d0 100644 --- a/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h +++ b/sysdeps/hppa/nptl/bits/pthreadtypes-arch.h @@ -40,7 +40,7 @@ #define __SIZEOF_PTHREAD_RWLOCK_T 64 #define __SIZEOF_PTHREAD_RWLOCKATTR_T 8 -#define __LOCK_ALIGNMENT __attribute__ ((__aligned__(16))) +#define __LOCK_ALIGNMENT #define __ONCE_ALIGNMENT #endif /* bits/pthreadtypes.h */ diff --git a/sysdeps/hppa/nptl/bits/struct_rwlock.h b/sysdeps/hppa/nptl/bits/struct_rwlock.h index e83b4aab52..59bc9fe76f 100644 --- a/sysdeps/hppa/nptl/bits/struct_rwlock.h +++ b/sysdeps/hppa/nptl/bits/struct_rwlock.h @@ -25,8 +25,14 @@ struct __pthread_rwlock_arch_t /* In the old Linuxthreads pthread_rwlock_t, this is the start of the 4-word 16-byte aligned lock structure. The next four words are all set to 1 by the Linuxthreads - PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. */ - int __compat_padding[4] __attribute__ ((__aligned__(16))); + PTHREAD_RWLOCK_INITIALIZER. We ignore them in NPTL. + + The 16-byte aligned lock stucture causes various pthread + structures to be over aligned. This causes some builds + to fail which assume a maximum alignment of 8 bytes. + Linuxthreads has been removed for 12 years, so drop + alignment of lock structure. */ + int __compat_padding[4]; unsigned int __readers; unsigned int __writers; unsigned int __wrphase_futex;
hppa: Drop 16-byte pthread lock alignment Linux threads were removed about 12 years ago and the current nptl implementation only requires 4-byte alignment for pthread locks. The 16-byte alignment causes various issues. For example in building ignition-msgs, we have: /usr/include/google/protobuf/map.h:124:37: error: static assertion failed 124 | static_assert(alignof(value_type) <= 8, ""); | ~~~~~~~~~~~~~~~~~~~~^~~~ This is caused by the 16-byte pthread lock alignment. Signed-off-by: John David Anglin <dave.anglin@bell.net> ---