[PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Adhemerval Zanella]

The HPPA also requires a 16-byte alignment for locks, although it is
just a historical artifact to keep compatibility with old
implementation.
---
 sysdeps/nptl/libc-lockP.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
index d3a6837fd2..9efe962588 100644
--- a/sysdeps/nptl/libc-lockP.h
+++ b/sysdeps/nptl/libc-lockP.h
@@ -34,7 +34,7 @@
 #include <tls.h>
 
 /* Mutex type.  */
-typedef int __libc_lock_t;
+typedef int __libc_lock_t __LOCK_ALIGNMENT;
 typedef struct { pthread_mutex_t mutex; } __rtld_lock_recursive_t;
 typedef pthread_rwlock_t __libc_rwlock_t;
 
-- 
2.34.1

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[John Paul Adrian Glaubitz]

Hi Adhemerval!

On 8/30/22 15:35, Adhemerval Zanella wrote:
> The HPPA also requires a 16-byte alignment for locks, although it is
> just a historical artifact to keep compatibility with old
> implementation.

Thanks for the quick fix.

Can you fix the typo in the subject?

s/m68/m68k/?

Thanks,
Adrian

-- 
  .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Adhemerval Zanella Netto]

On 30/08/22 10:46, John Paul Adrian Glaubitz wrote:
> Hi Adhemerval!
> 
> On 8/30/22 15:35, Adhemerval Zanella wrote:
>> The HPPA also requires a 16-byte alignment for locks, although it is
>> just a historical artifact to keep compatibility with old
>> implementation.
> 
> Thanks for the quick fix.
> 
> Can you fix the typo in the subject?
> 
> s/m68/m68k/?

Sure.

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Carlos O'Donell]

On 8/30/22 09:35, Adhemerval Zanella via Libc-alpha wrote:
> The HPPA also requires a 16-byte alignment for locks, although it is
> just a historical artifact to keep compatibility with old
> implementation.
> ---
>  sysdeps/nptl/libc-lockP.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
> index d3a6837fd2..9efe962588 100644
> --- a/sysdeps/nptl/libc-lockP.h
> +++ b/sysdeps/nptl/libc-lockP.h
> @@ -34,7 +34,7 @@
>  #include <tls.h>
>  
>  /* Mutex type.  */
> -typedef int __libc_lock_t;
> +typedef int __libc_lock_t __LOCK_ALIGNMENT;
>  typedef struct { pthread_mutex_t mutex; } __rtld_lock_recursive_t;
>  typedef pthread_rwlock_t __libc_rwlock_t;
  
Are you able to verify that this doesn't change the ABI of any other
structures that might have an internal lock?

Normally I would use libabigail[1] before and after the patch to look
for subtype size changes in public ABIs/APIs.

-- 
Cheers,
Carlos.

[1] https://sourceware.org/libabigail/

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Adhemerval Zanella Netto]

On 30/08/22 11:18, Carlos O'Donell wrote:
> On 8/30/22 09:35, Adhemerval Zanella via Libc-alpha wrote:
>> The HPPA also requires a 16-byte alignment for locks, although it is
>> just a historical artifact to keep compatibility with old
>> implementation.
>> ---
>>  sysdeps/nptl/libc-lockP.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
>> index d3a6837fd2..9efe962588 100644
>> --- a/sysdeps/nptl/libc-lockP.h
>> +++ b/sysdeps/nptl/libc-lockP.h
>> @@ -34,7 +34,7 @@
>>  #include <tls.h>
>>  
>>  /* Mutex type.  */
>> -typedef int __libc_lock_t;
>> +typedef int __libc_lock_t __LOCK_ALIGNMENT;
>>  typedef struct { pthread_mutex_t mutex; } __rtld_lock_recursive_t;
>>  typedef pthread_rwlock_t __libc_rwlock_t;
>   
> Are you able to verify that this doesn't change the ABI of any other
> structures that might have an internal lock?
> 
> Normally I would use libabigail[1] before and after the patch to look
> for subtype size changes in public ABIs/APIs.
> 

It should not and I would consider this an bug since the header is internal
only and all definitions are used solely on internal code (although 
pthreadtypes-arch.h is an installed header, libc-lockP.h is not).

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Richard Henderson]

On 8/30/22 06:35, Adhemerval Zanella via Libc-alpha wrote:
> The HPPA also requires a 16-byte alignment for locks, although it is
> just a historical artifact to keep compatibility with old
> implementation.
> ---
>   sysdeps/nptl/libc-lockP.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
> index d3a6837fd2..9efe962588 100644
> --- a/sysdeps/nptl/libc-lockP.h
> +++ b/sysdeps/nptl/libc-lockP.h
> @@ -34,7 +34,7 @@
>   #include <tls.h>
>   
>   /* Mutex type.  */
> -typedef int __libc_lock_t;
> +typedef int __libc_lock_t __LOCK_ALIGNMENT;

As Carlos explained, hppa doesn't require 16-byte alignment for kernel-assisted CAS.

I was on my way towards testing

-/* Mutex type.  */

-typedef int __libc_lock_t;

+/* Mutex type.

+   It is a requirement of the futex interface that the data be

+   naturally aligned.  This is almost always already the case

+   for 'int', but some older ABIs, e.g. m68k, do not.

+   The pthread types will have been aligned by __LOCK_ALIGNMENT.  */

+typedef int __libc_lock_t __attribute__((aligned(4)));




r~

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Adhemerval Zanella Netto]

On 30/08/22 13:32, Richard Henderson wrote:
> On 8/30/22 06:35, Adhemerval Zanella via Libc-alpha wrote:
>> The HPPA also requires a 16-byte alignment for locks, although it is
>> just a historical artifact to keep compatibility with old
>> implementation.
>> ---
>>   sysdeps/nptl/libc-lockP.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
>> index d3a6837fd2..9efe962588 100644
>> --- a/sysdeps/nptl/libc-lockP.h
>> +++ b/sysdeps/nptl/libc-lockP.h
>> @@ -34,7 +34,7 @@
>>   #include <tls.h>
>>     /* Mutex type.  */
>> -typedef int __libc_lock_t;
>> +typedef int __libc_lock_t __LOCK_ALIGNMENT;
> 
> As Carlos explained, hppa doesn't require 16-byte alignment for kernel-assisted CAS.
> 
> I was on my way towards testing
> 
> -/* Mutex type.  */
> 
> -typedef int __libc_lock_t;
> 
> +/* Mutex type.
> 
> +   It is a requirement of the futex interface that the data be
> 
> +   naturally aligned.  This is almost always already the case
> 
> +   for 'int', but some older ABIs, e.g. m68k, do not.
> 
> +   The pthread types will have been aligned by __LOCK_ALIGNMENT.  */
> 
> +typedef int __libc_lock_t __attribute__((aligned(4)));
> 
> 
> 
> 
> r~

I don't have a strong preference, using __LOCK_ALIGNMENT at least is a no-op
for other architectures (although it most likely won't change anything assuming
int has 4-bytes alignment as usual).

Re: [PATCH] m68: Enforce 4-byte alignment on internal locks (BZ #29537)

[Andreas K. Huettel]

Am Dienstag, 30. August 2022, 15:35:04 CEST schrieb Adhemerval Zanella via Libc-alpha:
> The HPPA also requires a 16-byte alignment for locks, although it is
> just a historical artifact to keep compatibility with old
> implementation.
> ---
>  sysdeps/nptl/libc-lockP.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
> index d3a6837fd2..9efe962588 100644
> --- a/sysdeps/nptl/libc-lockP.h
> +++ b/sysdeps/nptl/libc-lockP.h
> @@ -34,7 +34,7 @@
>  #include <tls.h>
>  
>  /* Mutex type.  */
> -typedef int __libc_lock_t;
> +typedef int __libc_lock_t __LOCK_ALIGNMENT;
>  typedef struct { pthread_mutex_t mutex; } __rtld_lock_recursive_t;
>  typedef pthread_rwlock_t __libc_rwlock_t;
>  
> 

I've added this to glibc-2.35, recompiled and reinstalled glibc, and was then 
able to 
* update my qemu-m68k chroot to newest packages
* and have it rebuild itself fully
at MAKEOPTS="-j17" without any issues. 

(This means, whereas building python always failed before, now I built python-3.10
and python-3.11 each twice, without problems. The chroot contains the Gentoo
@system set, so glibc, binutils, gcc, ...)

So, LGTM and thank you!!!

-- 
Andreas K. Hüttel
dilfridge@gentoo.org
Gentoo Linux developer
(council, toolchain, base-system, perl, libreoffice)