This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Force emission of personality directive if explicitly specified
ClosedPublic

Authored by loladiro on Jul 1 2015, 3:00 PM.

Details

Summary

Before this change, personality directives were not emitted if there was no landing pad left in the function
(of course until recently this also meant that we couldn't know what the personality actually was).
Allow this now and add a test case for it.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 28903.Jul 1 2015, 3:00 PM
loladiro retitled this revision from to [CodeGen] Force emission of personality directive if explicitly specified.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: majnemer.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: Unknown Object (MLST).
majnemer added inline comments.Jul 1 2015, 3:14 PM
lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
96

should we require that it's not nounwind?

loladiro added inline comments.Jul 1 2015, 3:15 PM
lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
96

Right, yes, we discussed that on IRC.

loladiro updated this revision to Diff 28961.Jul 2 2015, 9:51 AM

Updated to check attributes.

majnemer edited edge metadata.Jul 2 2015, 11:35 AM

I don't see a test case

loladiro updated this revision to Diff 28971.Jul 2 2015, 1:13 PM
loladiro edited edge metadata.

Re-added test case that was lost in the diff.

Does this look good now?

rnk added a subscriber: rnk.Jul 13 2015, 11:57 AM

This seems like it's going to produce a lot of useless personality data in C++. I'd be OK with this if our mid-level optimizers were more aggressive about removing known Itanium personalities from functions with no landingpads.

loladiro updated this revision to Diff 29600.Jul 13 2015, 1:33 PM

Updated based on discussion with to use EHPersonality @rnk to still omit known personality function if they are known to be noops.

rnk added inline comments.Jul 13 2015, 2:05 PM
include/llvm/Analysis/LibCallSemantics.h
209–211

We're about to add new blocks that invokes can unwind to, so perhaps let's flip this around and talk about invokes. Maybe isNoOpWithoutInvoke?

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
108

For historical reasons, we often have bitcasts around personality functions. You should do getPersonalityFn()->stripPointerCasts().

110–111

Now that the personality is on the IR function, this should never return anything other than the value computed above. After fixing the casting issue, can we change this to an assertion?

rnk added a comment.Jul 13 2015, 2:08 PM

Oh, you should also update WinException.cpp and ARMException.cpp to have this behavior.

loladiro updated this revision to Diff 29614.Jul 13 2015, 2:45 PM

Addresses @rnk's review comments:
isNoOpWithoutLpads -> isNoOpWithoutInvoke and add the same logic to Win and ARM

rnk accepted this revision.Jul 13 2015, 3:14 PM
rnk added a reviewer: rnk.

lgtm

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp
110–111

I was trying to get away from relying on MMI->getPersonality() at all. Something like:

const Function *Per = nullptr;
if (F->hasPersonalityFn())
  Per = dyn_cast<Function>(F->getPersonalityFn()->stripPointerCasts());
assert(Per == MMI->getPersonality());

Eventually we probably want to rip out MMI's tracking of EH personalities, now that they aren't on landingpads. For now, though, we should ensure that MMI is in sync with the IR.

This revision is now accepted and ready to land.Jul 13 2015, 3:14 PM
loladiro updated this revision to Diff 29616.Jul 13 2015, 3:25 PM
loladiro edited edge metadata.

Changed to assert that the MMI agrees with the Function on the personality.

Unfortunately it looks like the assertion does not hold true on ARM. Shall I just go back to using either or shall I figure out why the assertions doesn't hold?

Actually, the difference seems to be running the tests on my local Mac vs my (Linux) development machine. Will investigate.

Actually, duh. It's precisely not true in the case where there is no landing pads remaining, which is the whole point of this change - I should go home!

loladiro updated this revision to Diff 29623.Jul 13 2015, 4:29 PM

Updated to only check for matching personalities if there is one recorded in the MMI. @rnk does that sound ok?

rnk added a comment.Jul 14 2015, 11:27 AM

Updated to only check for matching personalities if there is one recorded in the MMI. @rnk does that sound ok?

Yep! Go for it.

This revision was automatically updated to reflect the committed changes.