Page MenuHomePhabricator

[libcxxabi] Insert padding in __cxa_exception struct for compatibility
ClosedPublic

Authored by steven_wu on Jan 10 2020, 2:53 PM.

Details

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.

Diff Detail

Event Timeline

steven_wu created this revision.Jan 10 2020, 2:53 PM

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?

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

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 added a subscriber: 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?

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 added a subscriber: hans.Jan 16 2020, 3:46 PM

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.

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.

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 added a comment.Jan 17 2020, 1:18 AM

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.

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?

Fix ARM_EHABI.

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 updated this revision to Diff 241195.Jan 29 2020, 9:26 AM

Move static_assert into header

rjmccall accepted this revision.Jan 29 2020, 10:02 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Jan 29 2020, 10:02 AM
This revision was automatically updated to reflect the committed changes.

@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 added a comment.Mon, Feb 3, 5:18 AM

@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.

@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 added a comment.Tue, Feb 4, 2:00 AM

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

Thanks! Picked that as 165a6367631d643f8f186d7a809013652cf321d6