[libcxxabi] Insert padding in __cxa_exception struct for compatibility

[steven_wu]

Summary

Preserve the old ABI for cxa_exception and cxa_dependent_exception
on 64 bit platforms or ARM_EHABI platforms.

After r276215, libunwind in llvm-project labels _Unwind_Exception to be
double word aligned. That change implictly adds a padding before
unwindHeader field in cxa_exception and cxa_dependent_exception.
Preserve the same negative offsets in those struct by moving the padding
to the beginning of the field.

The assumption here is that if the ABI is not aware of the padding before
unwindHeader and put the referenceCount/primaryException in there, no padding
should exist before unwindHeader.

[rjmccall]

Do we need to be careful about the alignment used by __attribute__((aligned)) on different targets? And would be it useful to put the static_asserts in the headers to make it more likely that we catch problems?

[compnerd]

Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?

[rjmccall]

In D72543#1817753, @compnerd wrote:

Do we need to worry about compatibility? What happens if there is a mix between an old binary and a new binary?

To be clear, libUnwind accidentally introduced an ABI break by changing the alignment of _Unwind_Exception without making sure that layout didn't change in client code. So there's actually *three* eras here: old, intermediate, and new. The old and new ABIs agree in layout except about sizeof cxa*exception, which is not intended to be stable for code outside of the C++ runtime library, so the problem is just with code that might be have compiled with the intermediate code.

On Darwin, we happened to not have picked up a new libUnwind since the intermediate era began. (And as soon as we did, we ran into the ABI compatibility problem.)

[smeenai]

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

Additionally, is this something that should be picked to 10.0 after it's committed?

[rjmccall]

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Additionally, is this something that should be picked to 10.0 after it's committed?

I think this should be put in 10.0, yes.

[smeenai]

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

Additionally, is this something that should be picked to 10.0 after it's committed?

I think this should be put in 10.0, yes.

CC @hans

libcxxabi/src/cxa_exception.cpp

59 ↗

(On Diff #237439)

There's no adjustedPtr field in the _LIBCXXABI_ARM_EHABI case ... the field right before the unwindHeader in that case is int propagationCount.

63 ↗

(On Diff #237439)

Same here.

[rjmccall]

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

[smeenai]

In D72543#1825454, @rjmccall wrote:

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

You're right; I understand the case rL319123 was intended for now.

[hans]

Additionally, is this something that should be picked to 10.0 after it's committed?

I think this should be put in 10.0, yes.

CC @hans

Thanks! I've put it on my watch-list.

[steven_wu]

In D72543#1825496, @smeenai wrote:

In D72543#1825454, @rjmccall wrote:

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

You're right; I understand the case rL319123 was intended for now.

Sorry I was on vacation. I intended to keep rL319123 as least for now. The current setup for MacOS is that clang header is setup to use unwind.h from SDK, which it will currently find the unaligned exception struct. After this patch, rL319123 is a no-op for people building libcxxabi against TOT llvm libunwind but it will help people building against the old header to get the correct alignment fro exception struct.

@rjmccall are you suggesting to put the static_assert directly in the header so users can get build failures if they expect different layout? That can help but I need to figure out what is the correct layout different users are expecting.

Since the unwind change is a while back, is there any system that picked up the intermediate layout and expected to keep compatibility with that layout?

[steven_wu]

Fix ARM_EHABI.

[rjmccall]

In D72543#1829870, @steven_wu wrote:

In D72543#1825496, @smeenai wrote:

In D72543#1825454, @rjmccall wrote:

In D72543#1825422, @smeenai wrote:

In D72543#1825178, @rjmccall wrote:

In D72543#1825150, @smeenai wrote:

From spelunking through history, it looks like D33030 tried to solve the same problem, and then rL319123 undid that solution and came up with a different one to solve the same underlying issue. rL319123 is still in place; does it need to be adjusted for the change in this diff?

It looks like those patches were attempts to ensure adequate alignment of the exception object given the absence of an alignment attribute on the _Unwind_Exception type itself. rL319123 became inadequate after _Unwind_Exception became an aligned type because that change still affected the layout of the C++ exception types. What we're fixing here is essentially the same problem that was introduced by D33030.

Makes sense. Should rL319123 be reverted then? The extra space that's being allocated by that change doesn't do what it's meant to do, so it seems wasteful to keep allocating it.

If __cxa_exception is already sufficiently aligned, I believe that patch does not allocate any extra space.

You're right; I understand the case rL319123 was intended for now.

Sorry I was on vacation. I intended to keep rL319123 as least for now. The current setup for MacOS is that clang header is setup to use unwind.h from SDK, which it will currently find the unaligned exception struct. After this patch, rL319123 is a no-op for people building libcxxabi against TOT llvm libunwind but it will help people building against the old header to get the correct alignment fro exception struct.

@rjmccall are you suggesting to put the static_assert directly in the header so users can get build failures if they expect different layout? That can help but I need to figure out what is the correct layout different users are expecting.

That makes sense.

Since the unwind change is a while back, is there any system that picked up the intermediate layout and expected to keep compatibility with that layout?

I wouldn't think so, but of course I can't know for certain.

[steven_wu]

Move static_assert into header

[rjmccall]

Thanks, LGTM!

[steven_wu]

@hans I landed this on master. Let me know if I need to do something for release branch. I will wait a bit to see if it breaks any unexpected users.

[hans]

In D72543#1850031, @steven_wu wrote:

@hans I landed this on master. Let me know if I need to do something for release branch. I will wait a bit to see if it breaks any unexpected users.

I've cherry-picked it to 10.x in 674ec1eb16678b8addc02a4b0534ab383d22fa77, figuring it's better to get it in sooner rather than later. Please let me know if you hear about any problems or if there are follow-ups.

[mstorsjo]

In D72543#1854571, @hans wrote:

In D72543#1850031, @steven_wu wrote:

@hans I landed this on master. Let me know if I need to do something for release branch. I will wait a bit to see if it breaks any unexpected users.

I've cherry-picked it to 10.x in 674ec1eb16678b8addc02a4b0534ab383d22fa77, figuring it's better to get it in sooner rather than later. Please let me know if you hear about any problems or if there are follow-ups.

If this one was cherrypicked, you need D73838 / 09dc884eb2e4a433eb8c5ed20a17108091279295 as well, otherwise building for mingw is broken.

[hans]

If this one was cherrypicked, you need D73838 / 09dc884eb2e4a433eb8c5ed20a17108091279295 as well, otherwise building for mingw is broken.

Thanks! Picked that as 165a6367631d643f8f186d7a809013652cf321d6

libcxxabi/src/cxa_exception.h

 static const uint64_t get_vendor_and_language     = 0xFFFFFFFFFFFFFF00; // mask for CLNGC++
 
 _LIBCXXABI_HIDDEN uint64_t __getExceptionClass  (const _Unwind_Exception*);
 _LIBCXXABI_HIDDEN void     __setExceptionClass  (      _Unwind_Exception*, uint64_t);
 _LIBCXXABI_HIDDEN bool     __isOurExceptionClass(const _Unwind_Exception*);
 
 struct _LIBCXXABI_HIDDEN __cxa_exception {
 #if defined(__LP64__) || defined(_LIBCXXABI_ARM_EHABI)
+    // Now _Unwind_Exception is marked with __attribute__((aligned)),
+    // which implies __cxa_exception is also aligned. Insert padding
+    // in the beginning of the struct, rather than before unwindHeader.
+    void *reserve;
+
     // This is a new field to support C++ 0x exception_ptr.
     // For binary compatibility it is at the start of this
     // struct which is prepended to the object thrown in
     // __cxa_allocate_exception.
     size_t referenceCount;
 #endif
 
     //  Manage the exception object itself.
#endif
     _Unwind_Exception unwindHeader;
 };
 
 // http://sourcery.mentor.com/archives/cxx-abi-dev/msg01924.html
 // The layout of this structure MUST match the layout of __cxa_exception, with
 // primaryException instead of referenceCount.
 struct _LIBCXXABI_HIDDEN __cxa_dependent_exception {
 #if defined(__LP64__) || defined(_LIBCXXABI_ARM_EHABI)
+    void* reserve; // padding.
     void* primaryException;
 #endif
 
     std::type_info *exceptionType;
     void (*exceptionDestructor)(void *);
     std::unexpected_handler unexpectedHandler;
     std::terminate_handler terminateHandler;
 

 #endif
 
 #if !defined(__LP64__) && !defined(_LIBCXXABI_ARM_EHABI)
     void* primaryException;
 #endif
     _Unwind_Exception unwindHeader;
 };
 
+// Verify the negative offsets of different fields.
+static_assert(sizeof(_Unwind_Exception) +
+                      offsetof(__cxa_exception, unwindHeader) ==
+                  sizeof(__cxa_exception),
+              "unwindHeader has wrong negative offsets");
+static_assert(sizeof(_Unwind_Exception) +
+                      offsetof(__cxa_dependent_exception, unwindHeader) ==
+                  sizeof(__cxa_dependent_exception),
+              "unwindHeader has wrong negative offsets");
+
+#if defined(_LIBCXXABI_ARM_EHABI)
+static_assert(offsetof(__cxa_exception, propagationCount) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_exception),
+              "propagationCount has wrong negative offset");
+static_assert(offsetof(__cxa_dependent_exception, propagationCount) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_dependent_exception),
+              "propagationCount has wrong negative offset");
+#elif defined(__LP64__)
+static_assert(offsetof(__cxa_exception, adjustedPtr) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_exception),
+              "adjustedPtr has wrong negative offset");
+static_assert(offsetof(__cxa_dependent_exception, adjustedPtr) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_dependent_exception),
+              "adjustedPtr has wrong negative offset");
+#else
+static_assert(offsetof(__cxa_exception, referenceCount) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_exception),
+              "referenceCount has wrong negative offset");
+static_assert(offsetof(__cxa_dependent_exception, primaryException) +
+                      sizeof(_Unwind_Exception) + sizeof(void*) ==
+                  sizeof(__cxa_dependent_exception),
+              "primaryException has wrong negative offset");
+#endif
+
 struct _LIBCXXABI_HIDDEN __cxa_eh_globals {
     __cxa_exception *   caughtExceptions;
     unsigned int        uncaughtExceptions;
 #if defined(_LIBCXXABI_ARM_EHABI)
     __cxa_exception* propagatingExceptions;
 #endif
 };