This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenCXX] Fix ItaniumCXXABI::getAlignmentOfExnObject to return 8-byte alignment on Darwin
ClosedPublic

Authored by ahatanak on Mar 25 2016, 12:57 PM.

Details

Summary

r246985 made changes to give a higher alignment for exception objects on the grounds that Itanium says _Unwind_Exception should be "double-word" aligned and the structure is normally declared with attribute((aligned)) guaranteeing 16-byte alignment. It turns out that libc++abi doesn't declare the structure with attribute((aligned)) and therefore only guarantees 8-byte alignment on 32-bit and 64-bit platforms. This caused a crash in some cases when the backend emitted SIMD store instructions that requires 16-byte alignment (such as movaps).

This patch makes ItaniumCXXABI::getAlignmentOfExnObject return an 8-byte alignment on Darwin to fix the crash.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 51675.Mar 25 2016, 12:57 PM
ahatanak retitled this revision from to [CodeGenCXX] Fix ItaniumCXXABI::getAlignmentOfExnObject to return 8-byte alignment on Darwin.
ahatanak updated this object.
ahatanak added a reviewer: rjmccall.
ahatanak added a subscriber: cfe-commits.
rjmccall added inline comments.Mar 30 2016, 7:41 PM
lib/CodeGen/ItaniumCXXABI.cpp
174 ↗(On Diff #51675)

Just add a ExnObjectAlignment field to TargetInfo, please. We already have a DarwinTargetInfo mix-in that can set this to something lower.

Otherwise, this patch LGTM, although as mentioned elsewhere we should figure out whether other OSes also limit this more strictly.

ahatanak updated this revision to Diff 52171.Mar 30 2016, 9:47 PM

Thank you for the review, John.

I've updated the patch as per your request. I defined virtual function getExnObjectAlignment instead of adding a ExnObjectAlignment field to TargetInfo since I found that getDefaultAlignForAttributeAligned() can't be called in TargetInfo's constructor to initialize ExnObjectAlignment.

This revision was automatically updated to reflect the committed changes.
rjmccall added inline comments.Mar 31 2016, 9:31 AM
cfe/trunk/include/clang/Basic/TargetInfo.h
426

This should just be an ordinary comment, not a /// comment.

Please also add this to the documentation comment:

This is only meaningful for targets that allocate C++ exceptions in a system runtime, such as those using the Itanium C++ ABI.
rjmccall edited edge metadata.Mar 31 2016, 9:31 AM

Otherwise, LGTM.

ahatanak added inline comments.Mar 31 2016, 11:39 AM
cfe/trunk/include/clang/Basic/TargetInfo.h
426

Fixed in r265035.