This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Teach SimplifyPersonality about the updated LandingPadInst
ClosedPublic

Authored by vsk on Sep 9 2015, 5:17 PM.

Details

Summary

When uses of personality functions were moved from LandingPadInst to Function, we forgot to update SimplifyPersonality(). This patch corrects that.

Note: SimplifyPersonality() is an optimization which replaces personality functions with the default C++ personality when possible. Without this update, some ObjC++ projects fail to link against C++ libraries (seeing as the exception ABI had effectively changed).

Diff Detail

Repository
rL LLVM

Event Timeline

vsk updated this revision to Diff 34391.Sep 9 2015, 5:17 PM
vsk retitled this revision from to [CodeGen] Teach SimplifyPersonality about the updated LandingPadInst.
vsk updated this object.
vsk added reviewers: majnemer, rjmccall.
vsk added a subscriber: cfe-commits.
rjmccall added inline comments.Sep 9 2015, 8:36 PM
lib/CodeGen/CGException.cpp
276 ↗(On Diff #34391)

if (!F)

You should test this by adding an uncommon use of the personality function. Just declare it as a global function (the type doesn't really matter) and take its address somewhere.

vsk updated this revision to Diff 34408.Sep 9 2015, 9:34 PM
vsk updated this object.

Thanks for the review.

  • Addressed if (!U) bug.
  • Added test which loads a personality function, confirmed that we crash without the proper `if (!F)' check.
rjmccall edited edge metadata.Sep 10 2015, 9:37 PM

LGTM, thanks.

vsk accepted this revision.Sep 11 2015, 8:43 AM
vsk added a reviewer: vsk.
vsk marked an inline comment as done.

Committed r247421

This revision is now accepted and ready to land.Sep 11 2015, 8:43 AM
rnk added a subscriber: rnk.Sep 11 2015, 9:29 AM

I'm confused. I thought SimplifyPersonalityFunction was an optimization, but somehow it caused link failures? Why do you think this was an ABI break?

test/CodeGenObjCXX/exception-cxx.mm
8 ↗(On Diff #34408)

Don't you want to test the 'catch (int e)' case? That introduces interesting uses of the selector that catch-all doesn't have.

vsk added a comment.Sep 11 2015, 10:33 AM

I think 'optimization' is a bit of a misnomer. There's a comment in this code that reads: "Can't do the optimization if it has non-C++ uses", so that's why I picked up the word. Without SimplifyPersonality(), some objective c++ code can no longer link against c++ libraries.

Consider the following test:
$ echo "#include <stdexcept>" "\n" 'void foo() { throw std::runtime_error("foo"); }' | clang -x objective-cxx -S -emit-llvm - -o - | grep personality

After we moved personality from landingpadinst to functions, the output of the test changes (from gxx_personality to objc_personality). This commit just takes us back to the original behavior.

The original comment, "Otherwise, it has to be a landingpad instruction.", is wrong after David's patch.

This revision was automatically updated to reflect the committed changes.
vsk marked an inline comment as done.Sep 11 2015, 10:42 AM

Addressed Reid's comment w.r.t exception object type.

rnk added a comment.Sep 11 2015, 10:46 AM

Right, I understand the behavior change, I'm just wondering why it results in link failures. There isn't a ton of public info about how ObjC++ EH interacts with C++ EH.

vsk added a comment.Sep 11 2015, 11:06 AM

Ah, ok. We have some objective-c++ code which calls into a boost routine which throws an exception. That results in an undefined reference to ___objc_personality_v0, because the boost library we linked against doesn't have ___objc_personality_v0.

Should the compiler have found the ___objc_personality_v0 symbol in this case regardless?

In D12743#244375, @vsk wrote:

Ah, ok. We have some objective-c++ code which calls into a boost routine which throws an exception. That results in an undefined reference to ___objc_personality_v0, because the boost library we linked against doesn't have ___objc_personality_v0.

Should the compiler have found the ___objc_personality_v0 symbol in this case regardless?

It's pretty straightforward. Sometimes people write code in ObjC++ files that's really pure C++. Such code is generally compiled with -fexceptions because that's the default, and so it has cleanups, and those cleanups require us to pick a personality function. When they do so, they generally don't link against the ObjC runtime, and so __objc_personality_v0 isn't found. The workaround here is to recognize that they're not actually catching ObjC exception types and just quietly degrade to use the C++ personality.

It is probably technically an optimization, because it removes some overhead from double-parsing the exception tables (because the ObjC personality delegates to the C++ personality, instead of being tightly integrated with it), but that's not the real reason we do it.