[CodeGen] Force emission of personality directive if explicitly specified

[loladiro]

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.

[majnemer]

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp

95 ↗

(On Diff #28903)

should we require that it's not nounwind?

[loladiro]

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp

95 ↗

(On Diff #28903)

Right, yes, we discussed that on IRC.

[loladiro]

Updated to check attributes.

[majnemer]

I don't see a test case

[loladiro]

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

[loladiro]

Does this look good now?

[rnk]

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 based on discussion with to use EHPersonality @rnk to still omit known personality function if they are known to be noops.

[rnk]

include/llvm/Analysis/LibCallSemantics.h

209–211 ↗

(On Diff #29600)

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

119 ↗

(On Diff #29600)

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

121–122 ↗

(On Diff #29600)

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]

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

[loladiro]

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

[rnk]

lgtm

lib/CodeGen/AsmPrinter/DwarfCFIException.cpp

110–111 ↗

(On Diff #29614)

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.

[loladiro]

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

[loladiro]

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?

[loladiro]

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

[loladiro]

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 to only check for matching personalities if there is one recorded in the MMI. @rnk does that sound ok?

[rnk]

In D10884#204178, @loladiro wrote:

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

Yep! Go for it.

llvm/trunk/include/llvm/Analysis/LibCallSemantics.h

inline bool isMSVCEHPersonality(EHPersonality Pers) {
     case EHPersonality::MSVC_X86SEH:
     case EHPersonality::MSVC_Win64SEH:
       return true;
     default: return false;
     }
     llvm_unreachable("invalid enum");
   }
 
+  /// \brief Return true if this personality may be safely removed if there
+  /// are no invoke instructions remaining in the current function.
+  inline bool isNoOpWithoutInvoke(EHPersonality Pers) {
+    switch (Pers) {
+    case EHPersonality::Unknown:
+      return false;
+    // All known personalities currently have this behavior
+    default: return true;
+    }
+    llvm_unreachable("invalid enum");
+  }
+
   bool canSimplifyInvokeNoUnwind(const Function *F);
 
 } // end namespace llvm
 
 #endif

llvm/trunk/include/llvm/CodeGen/MachineModuleInfo.h

public:
   /// addLandingPad - Add a new panding pad.  Returns the label ID for the
   /// landing pad entry.
   MCSymbol *addLandingPad(MachineBasicBlock *LandingPad);
 
   /// addPersonality - Provide the personality function for the exception
   /// information.
   void addPersonality(MachineBasicBlock *LandingPad,
                       const Function *Personality);
+  void addPersonality(const Function *Personality);
 
   void addWinEHState(MachineBasicBlock *LandingPad, int State);
 
   /// getPersonalityIndex - Get index of the current personality function inside
   /// Personalitites array
   unsigned getPersonalityIndex() const;
 
   /// getPersonalities - Return array of personality functions ever seen.

llvm/trunk/lib/CodeGen/AsmPrinter/ARMException.cpp

if (MoveType == AsmPrinter::CFI_M_Debug) {
     Asm->OutStreamer->EmitCFIStartProc(false);
   }
 }
 
 /// endFunction - Gather and emit post-function exception information.
 ///
 void ARMException::endFunction(const MachineFunction *MF) {
   ARMTargetStreamer &ATS = getTargetStreamer();
+  const Function *F = MF->getFunction();
+  const Function *Per = nullptr;
+  if (F->hasPersonalityFn())
+    Per = dyn_cast<Function>(F->getPersonalityFn()->stripPointerCasts());
+  assert(!MMI->getPersonality() || Per == MMI->getPersonality());
+  bool forceEmitPersonality =
+    F->hasPersonalityFn() && !isNoOpWithoutInvoke(classifyEHPersonality(Per)) &&
+    F->needsUnwindTableEntry();
+  bool shouldEmitPersonality = forceEmitPersonality ||
+    !MMI->getLandingPads().empty();
   if (!Asm->MF->getFunction()->needsUnwindTableEntry() &&
-      MMI->getLandingPads().empty())
+      !shouldEmitPersonality)
     ATS.emitCantUnwind();
-  else {
-    if (!MMI->getLandingPads().empty()) {
-      // Emit references to personality.
-      if (const Function *Personality = MMI->getPersonality()) {
-        MCSymbol *PerSym = Asm->getSymbol(Personality);
-        Asm->OutStreamer->EmitSymbolAttribute(PerSym, MCSA_Global);
-        ATS.emitPersonality(PerSym);
-      }
+  else if (shouldEmitPersonality) {
+    if (Per) {
+      MCSymbol *PerSym = Asm->getSymbol(Per);
 
-      // Emit .handlerdata directive.
-      ATS.emitHandlerData();
 
-      // Emit actual exception table
-      emitExceptionTable();
-    }
-  }
 
   if (Asm->MAI->getExceptionHandlingType() == ExceptionHandling::ARM)
     ATS.emitFnEnd();
 }
 
 void ARMException::emitTypeInfos(unsigned TTypeEncoding) {
   const std::vector<const GlobalValue *> &TypeInfos = MMI->getTypeInfos();
   const std::vector<unsigned> &FilterIds = MMI->getFilterIds();

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCFIException.cpp

if (!Personalities[i])
       continue;
     MCSymbol *Sym = Asm->getSymbol(Personalities[i]);
     TLOF.emitPersonalityValue(*Asm->OutStreamer, Asm->TM, Sym);
   }
 }
 
 void DwarfCFIException::beginFunction(const MachineFunction *MF) {
   shouldEmitMoves = shouldEmitPersonality = shouldEmitLSDA = false;
+  const Function *F = MF->getFunction();
 
   // If any landing pads survive, we need an EH table.
   bool hasLandingPads = !MMI->getLandingPads().empty();
 
   // See if we need frame move info.
   AsmPrinter::CFIMoveType MoveType = Asm->needsCFIMoves();
   if (MoveType == AsmPrinter::CFI_M_EH ||
       (MoveType == AsmPrinter::CFI_M_Debug &&
        moveTypeModule == AsmPrinter::CFI_M_None))
     moveTypeModule = MoveType;
 
   shouldEmitMoves = MoveType != AsmPrinter::CFI_M_None;
 
   const TargetLoweringObjectFile &TLOF = Asm->getObjFileLowering();
   unsigned PerEncoding = TLOF.getPersonalityEncoding();
-  const Function *Per = MMI->getPersonality();
-
-  shouldEmitPersonality = hasLandingPads &&
-    PerEncoding != dwarf::DW_EH_PE_omit && Per;
+  const Function *Per = nullptr;
+  if (F->hasPersonalityFn())
+    Per = dyn_cast<Function>(F->getPersonalityFn()->stripPointerCasts());
+  assert(!MMI->getPersonality() || Per == MMI->getPersonality());
+
+  // Emit a personality function even when there are no landing pads
+  bool forceEmitPersonality =
+      // ...if a personality function is explicitly specified
+      F->hasPersonalityFn() &&
+      // ... and it's not known to be a noop in the absence of invokes
+      !isNoOpWithoutInvoke(classifyEHPersonality(Per)) &&
+      // ... and we're not explicitly asked not to emit it
+      F->needsUnwindTableEntry();
+
+  shouldEmitPersonality =
+      (forceEmitPersonality ||
+       (hasLandingPads && PerEncoding != dwarf::DW_EH_PE_omit)) &&
+      Per;
 
   unsigned LSDAEncoding = TLOF.getLSDAEncoding();
   shouldEmitLSDA = shouldEmitPersonality &&
     LSDAEncoding != dwarf::DW_EH_PE_omit;
 
   shouldEmitCFI = shouldEmitPersonality || shouldEmitMoves;
   if (!shouldEmitCFI)
     return;
 
   Asm->OutStreamer->EmitCFIStartProc(/*IsSimple=*/false);
 
   // Indicate personality routine, if any.
   if (!shouldEmitPersonality)
     return;
 
+  // If we are forced to emit this personality, make sure to record
+  // it because it might not appear in any landingpad
+  if (forceEmitPersonality)
+    MMI->addPersonality(Per);
+
   const MCSymbol *Sym =
       TLOF.getCFIPersonalitySymbol(Per, *Asm->Mang, Asm->TM, MMI);
   Asm->OutStreamer->EmitCFIPersonality(Sym, PerEncoding);
 
   // Provide LSDA information.
   if (!shouldEmitLSDA)
     return;
 

llvm/trunk/lib/CodeGen/AsmPrinter/EHStreamer.cpp

for (const auto &MI : MBB) {
         PreviousIsInvoke = true;
       }
     }
   }
 
   // If some instruction between the previous try-range and the end of the
   // function may throw, create a call-site entry with no landing pad for the
   // region following the try-range.
-  if (SawPotentiallyThrowing && !IsSJLJ) {
+  if (SawPotentiallyThrowing && !IsSJLJ && LastLabel != nullptr) {
     CallSiteEntry Site = { LastLabel, nullptr, nullptr, 0 };
     CallSites.push_back(Site);
   }
 }
 
 /// Emit landing pads and actions.
 ///
 /// The general organization of the table is complex, but the basic concepts are

llvm/trunk/lib/CodeGen/AsmPrinter/WinException.cpp

void WinException::beginFunction(const MachineFunction *MF) {
   shouldEmitMoves = Asm->needsSEHMoves();
 
   const TargetLoweringObjectFile &TLOF = Asm->getObjFileLowering();
   unsigned PerEncoding = TLOF.getPersonalityEncoding();
   const Function *Per = nullptr;
   if (F->hasPersonalityFn())
     Per = dyn_cast<Function>(F->getPersonalityFn()->stripPointerCasts());
 
-  shouldEmitPersonality = hasLandingPads &&
-    PerEncoding != dwarf::DW_EH_PE_omit && Per;
+  bool forceEmitPersonality =
+    F->hasPersonalityFn() && !isNoOpWithoutInvoke(classifyEHPersonality(Per)) &&
+    F->needsUnwindTableEntry();
+
+  shouldEmitPersonality = forceEmitPersonality || (hasLandingPads &&
+    PerEncoding != dwarf::DW_EH_PE_omit && Per);
 
   unsigned LSDAEncoding = TLOF.getLSDAEncoding();
   shouldEmitLSDA = shouldEmitPersonality &&
     LSDAEncoding != dwarf::DW_EH_PE_omit;
 
   // If we're not using CFI, we don't want the CFI or the personality. If
   // WinEHPrepare outlined something, we should emit the LSDA.
   if (!Asm->MAI->usesWindowsCFI()) {

llvm/trunk/lib/CodeGen/MachineModuleInfo.cpp

 }
 
 /// addPersonality - Provide the personality function for the exception
 /// information.
 void MachineModuleInfo::addPersonality(MachineBasicBlock *LandingPad,
                                        const Function *Personality) {
   LandingPadInfo &LP = getOrCreateLandingPadInfo(LandingPad);
   LP.Personality = Personality;
+  addPersonality(Personality);
+}
 
+void MachineModuleInfo::addPersonality(const Function *Personality) {
   for (unsigned i = 0; i < Personalities.size(); ++i)
     if (Personalities[i] == Personality)
       return;
 
   // If this is the first personality we're adding go
   // ahead and add it at the beginning.
   if (!Personalities[0])
     Personalities[0] = Personality;

llvm/trunk/test/CodeGen/X86/eh-nolandingpads.ll

+; RUN: llc < %s -mtriple=x86_64-pc-linux | FileCheck %s
+; Test that we emit functions with explicitly specified personality,
+; even if no landing pads are left.
+
+declare i32 @__my_personality_v0(...)
+declare void @might_throw()
+
+define i32 @foo() personality i32 (...)* @__my_personality_v0 {
+; CHECK: .cfi_personality 3, __my_personality_v0
+    call void @might_throw()
+    ret i32 0
+}