Page MenuHomePhabricator

Assume `__cxa_allocate_exception` returns an under-aligned memory on Darwin if the version of libc++abi isn't new enough to include the fix in r319123
ClosedPublic

Authored by ahatanak on May 8 2019, 2:21 AM.

Details

Summary

This patch resurrects r264998, which was committed to work around a bug in libc++abi that was causing _cxa_allocate_exception to return a memory that wasn't double-word aligned.

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160328/154332.html

I reverted r264998 after fixing the bug in libc++abi in r319123 (see the link below), but didn't really realize that doing so could break projects using older versions of libc++abi that don't have the fix committed in r319123.

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171127/210878.html

In addition, this patch makes clang issue a warning if the type of the thrown object requires an alignment that is larger than the minimum alignment the libc++abi runtime guarantees.

rdar://problem/49864414

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.May 8 2019, 2:21 AM
ldionne added inline comments.May 8 2019, 7:39 AM
include/clang/Basic/AlignedExceptionObject.h
15 ↗(On Diff #198601)

The guards look wrong.

31 ↗(On Diff #198601)

Would it make more sense to return the alignment directly here?

rjmccall added inline comments.May 8 2019, 12:08 PM
include/clang/Basic/AlignedExceptionObject.h
31 ↗(On Diff #198601)

I agree: this should be a function returning the minimum expected exception alignment on the current target. Also, why does this have its own header instead of being, say, a function on TargetInfo?

rjmccall added inline comments.May 8 2019, 12:10 PM
include/clang/Basic/AlignedExceptionObject.h
31 ↗(On Diff #198601)

Or ASTContext or something similar.

ahatanak updated this revision to Diff 198716.May 8 2019, 1:36 PM
ahatanak marked 3 inline comments as done.
ahatanak edited the summary of this revision. (Show Details)

Move the code that was in AlignedExceptionObject.h to getExnObjectAlignment and remove the header.

ahatanak marked an inline comment as done.May 8 2019, 1:38 PM
ahatanak added inline comments.
include/clang/Basic/AlignedExceptionObject.h
31 ↗(On Diff #198601)

I moved the code in alignedExceptionObjectMinVersion to getExnObjectAlignment and deleted the header.

rjmccall added inline comments.May 8 2019, 2:24 PM
include/clang/Basic/DiagnosticGroups.td
411

Why is this name so clipped?

include/clang/Basic/DiagnosticSemaKinds.td
6584

Hyphens seem wrong here, and you're missing some words, and it shouldn't be capitalized. How about:

"required alignment of type %0 (%1 bytes) is larger than the supported alignment of C++ exception objects on this target (%2 bytes)"

include/clang/Basic/TargetInfo.h
643

CharUnits?

lib/Sema/SemaExprCXX.cpp
946

Why is this a Darwin-specific check? Seems like it ought to be an Itanium-specific check, but it's equally applicable on Linux to highly-aligned types.

ahatanak updated this revision to Diff 198939.May 9 2019, 4:28 PM
ahatanak marked 6 inline comments as done.

Address review comments.

ahatanak added inline comments.May 9 2019, 4:28 PM
include/clang/Basic/TargetInfo.h
643

I added a function to ASTContext that returns CharUnits.

lib/Sema/SemaExprCXX.cpp
946

Except when the C++ ABI is Microsoft, this is always diagnosed.

rjmccall added inline comments.May 9 2019, 4:43 PM
lib/Sema/SemaExprCXX.cpp
946

Thanks. Please don't refer to libc++abi in this comment, though, since this is a general Itanium issue and not specific to any particular implementation. How about something like:

Under the Itanium C++ ABI, memory for the exception object is allocated by the runtime with no ability for the compiler to request additional alignment.  Warn if the exception type requires alignment beyond the minimum guaranteed by the target C++ runtime.
ahatanak updated this revision to Diff 198959.May 9 2019, 5:22 PM

Update the comment in CheckCXXThrowOperand.

rjmccall accepted this revision.May 9 2019, 5:52 PM

Thanks, LGTM.

This revision is now accepted and ready to land.May 9 2019, 5:52 PM
This revision was automatically updated to reflect the committed changes.