This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604
Needs ReviewPublic

Authored by EricWF on Aug 31 2015, 6:11 PM.

Details

Summary

In 32 bit builds the __cxa_exception class has an alignment requirement greater than the maximal alignment supported by malloc. If this is the case we need to manually align the pointers returned from malloc in ordered to prevent undefined behavior. This patch does exactly that.

See PR24604 for more information - https://llvm.org/bugs/show_bug.cgi?id=24604

Diff Detail

Event Timeline

EricWF updated this revision to Diff 33656.Aug 31 2015, 6:11 PM
EricWF retitled this revision from to [libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604.
EricWF updated this object.
EricWF added a reviewer: jroelofs.
EricWF added a subscriber: cfe-commits.
majnemer edited edge metadata.Aug 31 2015, 6:45 PM

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

If foo.exe is already getting __cxa_allocate_exception(...) from one libc++abi version and __cxa_free_exception from another then they are already in trouble because __cxa_free_exception(ptr) checks to see if ptr has been allocated from a builtin buffer. This only happens when malloc is out of memory but it would still be an existing problem in the scenario you describe.

Furthermore the new code is only executed in cases where we already have undefined behavior. We don't add any extra padding to the pointers returned from malloc unless not doing so would lead to UB.

I know if multiple instances of libc++abi in one executable is not supported on OS X (see FAQ at the bottom of libcxxabi.llvm.org). I don't know if thats supported on linux though. People really shouldn't be using a static libc++abi. In order for the exception handling globals to work there should only be one copy of the in the entire program.

Hopefully this change isn't too problematic and thanks for looking at it.

compnerd edited edge metadata.Aug 31 2015, 7:28 PM

While I can see the argument you raise, the saving grace here is that this is buried in libc++abi. The only real piece of libc++abi of real use to users is __cxa_demangle (which is only relevant for Windows, and Darwin where you have two level namespaces). For everyone else, libc++abi is merely an implementation detail for libc++ and will not be linked to directly. If you are mixing and matching the ABI support interfaces, or the runtime, you are already in a pretty bad shape, and allowing things to break in such a circumstances doesn't seem entirely terrible. Unfortunately, for compatibility with GCC, we do need to maintain this alignment, which is not guaranteed without this patch.

joerg requested changes to this revision.Sep 1 2015, 4:53 AM
joerg added a reviewer: joerg.
joerg added a subscriber: joerg.

Please don't commit this as is. Many platforms have posix_memalign or equivalent, which makes this both simpler and potentially without wasting memory. Compare e.g. D12001.

This revision now requires changes to proceed.Sep 1 2015, 4:53 AM
jroelofs edited edge metadata.Sep 1 2015, 7:59 AM

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

I don't think that is/was supported. The abi library holds some global state, which wouldn't be shared between the two instances of it.

EricWF added a comment.Sep 4 2015, 2:28 PM

Please don't commit this as is. Many platforms have posix_memalign or equivalent, which makes this both simpler and potentially without wasting memory. Compare e.g. D12001.

Will do. Any advice on detecting posix_memalign? I don't see anything in the patch you pointed me to.

PS. Thanks for actually rejecting the patch. It makes the phab workflow easier to use.

danalbert edited edge metadata.Sep 5 2015, 12:29 PM

Android has posix_memalign too.

EricWF updated this revision to Diff 34128.Sep 6 2015, 11:45 PM
EricWF edited edge metadata.

Address @joerg's comment and use posix_memalign when available. This patch does not fix the case where posix_memalign is unavailable but that should be rare and can come in another patch.

Thi patch also defers all changes to fallback_malloc to D12669.

Whats preventing this from landing? @joerg @danalbert Do we want to use "posix_memalign" on Android or not?

emaste added a subscriber: emaste.Jul 20 2016, 10:17 AM

Is this going to be committed?
Libunwind is about to grow the same alignment attribute (D22543)

The configure-time check LGTM. Android has it for all recent versions (since JB), but that will cover the case of GB and ICS.

src/cxa_exception.cpp
127

#warning until that's done?

As it happens on FreeBSD/i386 malloc returns a 16-byte-aligned pointer for allocations with size >= 16 so will not be affected by this issue.

emaste added inline comments.Jul 20 2016, 2:37 PM
test/test_cxa_allocate_exception.pass.cpp
34

requires_over_alignment not used?

Note that the addition of the referenceCount at the beginning of __cxa_exception for 64-bit builds causes the _Unwind_Exception unwindHeader to become misaligned. libstdc++ had this same issue (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38732, now fixed) and libcxxrt does as well (https://github.com/pathscale/libcxxrt/issues/38).