Question about alignment of struct _Unwind_Exception

Question about alignment of struct _Unwind_Exception

[Matheus Castanho]

Hi,

Recently I've been investigating an issue with an invalid TLS access on
powerpc64le when __libc_malloc tries to read SINGLE_THREAD_P [1]. The
issue could only be reproduced when using a glibc compiled with
-mcpu=power10. We found out that the size of 'struct pthread' was
changing between compiling with -mcpu=power9 and -mcpu=power10.

After inspecting that struct we found out that one specific field was
changing sizes:
	struct _Unwind_Exception exc;

That struct is declared without a fixed alignment value:

struct _Unwind_Exception
{
  _Unwind_Exception_Class exception_class;
  _Unwind_Exception_Cleanup_Fn exception_cleanup;
  _Unwind_Word private_1;
  _Unwind_Word private_2;
  /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
     Taking that literally does not make much sense generically.  Instead we
     provide the maximum alignment required by any type for the machine.  */
} __attribute__((__aligned__));

The GCC manual says [2]:

> Specifying no alignment argument implies the maximum alignment for the
target

The maximum alignment has been recently increased for P10, which we
suspect is causing the behavior we see.

Is there an specific reason why a fixed alignment value was not used for
struct _Unwind_Exception?

[1] https://github.com/advancetoolchain/advance-toolchain/issues/1780
[2]
https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes

Thanks,
Matheus Castanho

Re: Question about alignment of struct _Unwind_Exception

[Matheus Castanho]

+CC: bergner@linux.ibm.com

On 11/4/20 9:44 AM, Matheus Castanho via Libc-alpha wrote:
> Hi,
> 
> Recently I've been investigating an issue with an invalid TLS access on
> powerpc64le when __libc_malloc tries to read SINGLE_THREAD_P [1]. The
> issue could only be reproduced when using a glibc compiled with
> -mcpu=power10. We found out that the size of 'struct pthread' was
> changing between compiling with -mcpu=power9 and -mcpu=power10.
> 
> After inspecting that struct we found out that one specific field was
> changing sizes:
> 	struct _Unwind_Exception exc;
> 
> That struct is declared without a fixed alignment value:
> 
> struct _Unwind_Exception
> {
>   _Unwind_Exception_Class exception_class;
>   _Unwind_Exception_Cleanup_Fn exception_cleanup;
>   _Unwind_Word private_1;
>   _Unwind_Word private_2;
>   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>      Taking that literally does not make much sense generically.  Instead we
>      provide the maximum alignment required by any type for the machine.  */
> } __attribute__((__aligned__));
> 
> The GCC manual says [2]:
> 
>> Specifying no alignment argument implies the maximum alignment for the
> target
> 
> The maximum alignment has been recently increased for P10, which we
> suspect is causing the behavior we see.
> 
> Is there an specific reason why a fixed alignment value was not used for
> struct _Unwind_Exception?
> 
> [1] https://github.com/advancetoolchain/advance-toolchain/issues/1780
> [2]
> https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes
> 
> Thanks,
> Matheus Castanho
> 

Re: Question about alignment of struct _Unwind_Exception

[Florian Weimer]

* Matheus Castanho via Libc-alpha:

> Hi,
>
> Recently I've been investigating an issue with an invalid TLS access on
> powerpc64le when __libc_malloc tries to read SINGLE_THREAD_P [1]. The
> issue could only be reproduced when using a glibc compiled with
> -mcpu=power10. We found out that the size of 'struct pthread' was
> changing between compiling with -mcpu=power9 and -mcpu=power10.
>
> After inspecting that struct we found out that one specific field was
> changing sizes:
> 	struct _Unwind_Exception exc;
>
> That struct is declared without a fixed alignment value:
>
> struct _Unwind_Exception
> {
>   _Unwind_Exception_Class exception_class;
>   _Unwind_Exception_Cleanup_Fn exception_cleanup;
>   _Unwind_Word private_1;
>   _Unwind_Word private_2;
>   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>      Taking that literally does not make much sense generically.  Instead we
>      provide the maximum alignment required by any type for the machine.  */
> } __attribute__((__aligned__));
>
> The GCC manual says [2]:
>
>> Specifying no alignment argument implies the maximum alignment for the target
>
> The maximum alignment has been recently increased for P10, which we
> suspect is causing the behavior we see.
>
> Is there an specific reason why a fixed alignment value was not used for
> struct _Unwind_Exception?

This looks like a historical accident.  IA-64 probably had a double-word
as the maximum alignment.

Is the problem you are seeing that ld.so has a different struct pthread
layout than libc.so if a multilib is used?  The struct pthread layout
itself is not part of the external ABI, only bits of it are.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Question about alignment of struct _Unwind_Exception

[Matheus Castanho]

On 11/4/20 10:38 AM, Florian Weimer wrote:
> * Matheus Castanho via Libc-alpha:
> 
>> Hi,
>>
>> Recently I've been investigating an issue with an invalid TLS access on
>> powerpc64le when __libc_malloc tries to read SINGLE_THREAD_P [1]. The
>> issue could only be reproduced when using a glibc compiled with
>> -mcpu=power10. We found out that the size of 'struct pthread' was
>> changing between compiling with -mcpu=power9 and -mcpu=power10.
>>
>> After inspecting that struct we found out that one specific field was
>> changing sizes:
>> 	struct _Unwind_Exception exc;
>>
>> That struct is declared without a fixed alignment value:
>>
>> struct _Unwind_Exception
>> {
>>   _Unwind_Exception_Class exception_class;
>>   _Unwind_Exception_Cleanup_Fn exception_cleanup;
>>   _Unwind_Word private_1;
>>   _Unwind_Word private_2;
>>   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>>      Taking that literally does not make much sense generically.  Instead we
>>      provide the maximum alignment required by any type for the machine.  */
>> } __attribute__((__aligned__));
>>
>> The GCC manual says [2]:
>>
>>> Specifying no alignment argument implies the maximum alignment for the target
>>
>> The maximum alignment has been recently increased for P10, which we
>> suspect is causing the behavior we see.
>>
>> Is there an specific reason why a fixed alignment value was not used for
>> struct _Unwind_Exception?
> 
> This looks like a historical accident.  IA-64 probably had a double-word
> as the maximum alignment.
> 
> Is the problem you are seeing that ld.so has a different struct pthread
> layout than libc.so if a multilib is used? 

Looks like so. Since the ld.so was not compiled with -mcpu=power10, it was not
affected by the new default alignment and it sees the struct with a different size.

$ /opt/at14.0/bin/gdb -batch -ex "file /opt/at14.0/lib64/ld64.so.2" -ex "ptype /o struct pthread"
[...]
                           /* total size (bytes): 1936 */
                         }

$ /opt/at14.0/bin/gdb -batch -ex "file /opt/at14.0/lib64/power10/libc.so.6" -ex "ptype /o struct pthread"
[...]
                           /* total size (bytes): 1984 */
                         } 

> The struct pthread layout
> itself is not part of the external ABI, only bits of it are.


> 
> Thanks,
> Florian
> 

Re: Question about alignment of struct _Unwind_Exception

[Peter Bergner]

On 11/4/20 6:44 AM, Matheus Castanho wrote:
> That struct is declared without a fixed alignment value:
> 
> struct _Unwind_Exception
> {
>   _Unwind_Exception_Class exception_class;
>   _Unwind_Exception_Cleanup_Fn exception_cleanup;
>   _Unwind_Word private_1;
>   _Unwind_Word private_2;
>   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>      Taking that literally does not make much sense generically.  Instead we
>      provide the maximum alignment required by any type for the machine.  */
> } __attribute__((__aligned__));
> 
> The GCC manual says [2]:
> 
>> Specifying no alignment argument implies the maximum alignment for the
> target
> 
> The maximum alignment has been recently increased for P10, which we
> suspect is causing the behavior we see.

FYI, I am looking at backing out the GCC BIGGEST_ALIGNMENT change.
I had done that as a way to increase the alignment of the MMA types,
which we want more aligned than 128 (the current BIGGEST_ALIGNMENT
value) for performance reasons.  Clearly, changing BIGGEST_ALIGNMENT
is the wrong way to do that given the problems you're encountering.
If I can align the MMA types a different way, then the default
alignment will go back to 128 bits, which should "fix" the issue
you are seeing.

That said, who's to say that a valid reason to increase BIGGEST_ALIGNMENT
won't come along and break this for real, so it seems adding a specific
alignment value in the code above rather than relying on the default
seems like a good idea to me.

Peter

Re: Question about alignment of struct _Unwind_Exception

[Florian Weimer]

* Peter Bergner:

> FYI, I am looking at backing out the GCC BIGGEST_ALIGNMENT change.
> I had done that as a way to increase the alignment of the MMA types,
> which we want more aligned than 128 (the current BIGGEST_ALIGNMENT
> value) for performance reasons.  Clearly, changing BIGGEST_ALIGNMENT
> is the wrong way to do that given the problems you're encountering.
> If I can align the MMA types a different way, then the default
> alignment will go back to 128 bits, which should "fix" the issue
> you are seeing.

x86-64 does this as well if 512-bit vector types exist.  We need to fix
this in glibc.  (Maybe things work out due to the struct pthread layout
on x86-64, but it's still a bug.)

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Question about alignment of struct _Unwind_Exception

[Peter Bergner]

On 11/4/20 1:31 PM, Florian Weimer wrote:
>> If I can align the MMA types a different way, then the default
>> alignment will go back to 128 bits, which should "fix" the issue
>> you are seeing.
> 
> x86-64 does this as well if 512-bit vector types exist.  We need to fix
> this in glibc.  (Maybe things work out due to the struct pthread layout
> on x86-64, but it's still a bug.)

I agree GLIBC has a problem here.  That said, GCC should not have changed
the default alignment either.  I just committed a fix to GCC trunk that
reverts the change to BIGGEST_ALIGNMENT and fixes the alignment of the MMA
types a different way.  I will backport the fix to thje GCC 10 release
branch after it's baked a couple of days on trunk.

Peter

Re: Question about alignment of struct _Unwind_Exception

[Matheus Castanho]

On 11/6/20 8:16 PM, Peter Bergner wrote:
> On 11/4/20 1:31 PM, Florian Weimer wrote:
>>> If I can align the MMA types a different way, then the default
>>> alignment will go back to 128 bits, which should "fix" the issue
>>> you are seeing.
>>
>> x86-64 does this as well if 512-bit vector types exist.  We need to fix
>> this in glibc.  (Maybe things work out due to the struct pthread layout
>> on x86-64, but it's still a bug.)
> 
> I agree GLIBC has a problem here.  That said, GCC should not have changed
> the default alignment either.  I just committed a fix to GCC trunk that
> reverts the change to BIGGEST_ALIGNMENT and fixes the alignment of the MMA
> types a different way.  I will backport the fix to thje GCC 10 release
> branch after it's baked a couple of days on trunk.
> 
> Peter
> 

Thanks for the GCC fix, Peter.

Florian, on the glibc side, I'm experimenting with declaring a new macro to be used
as the alignment value for struct _Unwind_Exception and then fixing that macro to
the current BIGGEST_ALIGNMENT value for each arch, according to GCC source.

This should effectively be a no-op and protect struct pthread from future changes to
BIGGEST_ALIGNMENT. I tried something like (powerpc-only example):

diff --git a/sysdeps/generic/unwind-arch.h b/sysdeps/generic/unwind-arch.h
index d712e5e11d..17f3efd271 100644
--- a/sysdeps/generic/unwind-arch.h
+++ b/sysdeps/generic/unwind-arch.h
@@ -19,7 +19,9 @@
 #ifndef _UNWIND_ARCH_H
 #define _UNWIND_ARCH_H
 
-#include <unwind.h>
+#ifndef UNWIND_EXCEPTION_ALIGN
+# define UNWIND_EXCEPTION_ALIGN
+#endif
 
 static inline void *
 unwind_arch_adjustment (void *prev, void *addr)
diff --git a/sysdeps/generic/unwind.h b/sysdeps/generic/unwind.h
index b667a5b652..da1b5e6ca7 100644
--- a/sysdeps/generic/unwind.h
+++ b/sysdeps/generic/unwind.h
@@ -27,6 +27,8 @@
 extern "C" {
 #endif
 
+#include <unwind-arch.h>
+
 /* Level 1: Base ABI  */
 
 /* @@@ The IA-64 ABI uses uint64 throughout.  Most places this is
@@ -83,7 +85,7 @@ struct _Unwind_Exception
   /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
      Taking that literally does not make much sense generically.  Instead we
      provide the maximum alignment required by any type for the machine.  */
-} __attribute__((__aligned__));
+} __attribute__((__aligned__(UNWIND_EXCEPTION_ALIGN)));
 
 
 /* The ACTIONS argument to the personality routine is a bitwise OR of one
diff --git a/sysdeps/powerpc/unwind-arch.h b/sysdeps/powerpc/unwind-arch.h
new file mode 100644
index 0000000000..b7756da28a
--- /dev/null
+++ b/sysdeps/powerpc/unwind-arch.h
@@ -0,0 +1,6 @@
+#ifndef _UNWIND_ARCH_H
+#define _UNWIND_ARCH_H
+
+#define UNWIND_EXCEPTION_ALIGN 16
+
+#endif

--

Do you have any suggestions on a better way to deal with this?

Thanks,
Matheus Castanho

Re: Question about alignment of struct _Unwind_Exception

[Florian Weimer]

* Matheus Castanho via Libc-alpha:

> diff --git a/sysdeps/generic/unwind.h b/sysdeps/generic/unwind.h
> index b667a5b652..da1b5e6ca7 100644
> --- a/sysdeps/generic/unwind.h
> +++ b/sysdeps/generic/unwind.h
> @@ -27,6 +27,8 @@
>  extern "C" {
>  #endif
>  
> +#include <unwind-arch.h>
> +
>  /* Level 1: Base ABI  */
>  
>  /* @@@ The IA-64 ABI uses uint64 throughout.  Most places this is
> @@ -83,7 +85,7 @@ struct _Unwind_Exception
>    /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>       Taking that literally does not make much sense generically.  Instead we
>       provide the maximum alignment required by any type for the machine.  */
> -} __attribute__((__aligned__));
> +} __attribute__((__aligned__(UNWIND_EXCEPTION_ALIGN)));

Can you use an anonymous union to do just what the comment says,
something like this (untested)?

struct _Unwind_Exception
{
  union
  {
    struct
    {
      _Unwind_Exception_Class exception_class;
      _Unwind_Exception_Cleanup_Fn exception_cleanup;
      _Unwind_Word private_1;
      _Unwind_Word private_2;
    };

   /* The IA-64 ABI says that this structure must be double-word
      aligned.  */
   _Unwind_Word unwind_exception_align[2]
     __attributed__ ((aligned (2 * sizeof (_Unwind_Word))));
  };
};

The union ensures that the aligned attribute does not reduce alignment
for the entire struct.

As discussed, it's an internal ABI change on most targets, but that
should be okay.  The dependency on BIGGEST_ALIGNMENT is gone.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: Question about alignment of struct _Unwind_Exception

[Matheus Castanho]

On 11/10/20 3:13 PM, Florian Weimer wrote:
> * Matheus Castanho via Libc-alpha:
> 
>> diff --git a/sysdeps/generic/unwind.h b/sysdeps/generic/unwind.h
>> index b667a5b652..da1b5e6ca7 100644
>> --- a/sysdeps/generic/unwind.h
>> +++ b/sysdeps/generic/unwind.h
>> @@ -27,6 +27,8 @@
>>  extern "C" {
>>  #endif
>>  
>> +#include <unwind-arch.h>
>> +
>>  /* Level 1: Base ABI  */
>>  
>>  /* @@@ The IA-64 ABI uses uint64 throughout.  Most places this is
>> @@ -83,7 +85,7 @@ struct _Unwind_Exception
>>    /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
>>       Taking that literally does not make much sense generically.  Instead we
>>       provide the maximum alignment required by any type for the machine.  */
>> -} __attribute__((__aligned__));
>> +} __attribute__((__aligned__(UNWIND_EXCEPTION_ALIGN)));
> 
> Can you use an anonymous union to do just what the comment says,
> something like this (untested)?
> 
> struct _Unwind_Exception
> {
>   union
>   {
>     struct
>     {
>       _Unwind_Exception_Class exception_class;
>       _Unwind_Exception_Cleanup_Fn exception_cleanup;
>       _Unwind_Word private_1;
>       _Unwind_Word private_2;
>     };
> 
>    /* The IA-64 ABI says that this structure must be double-word
>       aligned.  */
>    _Unwind_Word unwind_exception_align[2]
>      __attributed__ ((aligned (2 * sizeof (_Unwind_Word))));
>   };
> };
> 
> The union ensures that the aligned attribute does not reduce alignment
> for the entire struct.
> 
> As discussed, it's an internal ABI change on most targets, but that
> should be okay.  The dependency on BIGGEST_ALIGNMENT is gone.
> 
> Thanks,
> Florian
> 

That sounds like a cleaner solution. I tested it on powerpc64le and it fixes the original issue I saw.
All tests passing on x86-64 as well.

The diff I tested is below, with a simple typo fix.

Please feel free to commit under your name since it you provided the solution =)

Thanks,
Matheus Castanho

---8<---

diff --git a/sysdeps/generic/unwind.h b/sysdeps/generic/unwind.h
index b667a5b652..c229603af3 100644
--- a/sysdeps/generic/unwind.h
+++ b/sysdeps/generic/unwind.h
@@ -75,15 +75,21 @@ typedef void (*_Unwind_Exception_Cleanup_Fn) (_Unwind_Reason_Code,
 
 struct _Unwind_Exception
 {
-  _Unwind_Exception_Class exception_class;
-  _Unwind_Exception_Cleanup_Fn exception_cleanup;
-  _Unwind_Word private_1;
-  _Unwind_Word private_2;
-
-  /* @@@ The IA-64 ABI says that this structure must be double-word aligned.
-     Taking that literally does not make much sense generically.  Instead we
-     provide the maximum alignment required by any type for the machine.  */
-} __attribute__((__aligned__));
+  union
+  {
+    struct
+    {
+      _Unwind_Exception_Class exception_class;
+      _Unwind_Exception_Cleanup_Fn exception_cleanup;
+      _Unwind_Word private_1;
+      _Unwind_Word private_2;
+    };
+
+    /* The IA-64 ABI says that this structure must be double-word aligned.  */
+    _Unwind_Word unwind_exception_align[2]
+      __attribute__ ((__aligned__ (2 * sizeof (_Unwind_Word))));
+  };
+};

Re: Question about alignment of struct _Unwind_Exception

[Florian Weimer]

* Matheus Castanho via Libc-alpha:

> That sounds like a cleaner solution. I tested it on powerpc64le and it
> fixes the original issue I saw.  All tests passing on x86-64 as well.
>
> The diff I tested is below, with a simple typo fix.
>
> Please feel free to commit under your name since it you provided the
> solution =)

Okay, I'll addd Tested-By: for you.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill