[CodeGen] Teach SimplifyPersonality about the updated LandingPadInst

[vsk]

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).

[rjmccall]

lib/CodeGen/CGException.cpp

276

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]

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]

LGTM, thanks.

[vsk]

Committed r247421

[rnk]

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

9

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]

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.

[vsk]

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

[rnk]

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]

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?

[rjmccall]

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.

lib/CodeGen/CGException.cpp

 }
 
 static llvm::Constant *getOpaquePersonalityFn(CodeGenModule &CGM,
                                         const EHPersonality &Personality) {
   llvm::Constant *Fn = getPersonalityFn(CGM, Personality);
   return llvm::ConstantExpr::getBitCast(Fn, CGM.Int8PtrTy);
 }
 
-/// Check whether a personality function could reasonably be swapped
-/// for a C++ personality function.
-static bool PersonalityHasOnlyCXXUses(llvm::Constant *Fn) {
-  for (llvm::User *U : Fn->users()) {
-    // Conditionally white-list bitcasts.
-    if (llvm::ConstantExpr *CE = dyn_cast<llvm::ConstantExpr>(U)) {
-      if (CE->getOpcode() != llvm::Instruction::BitCast) return false;
-      if (!PersonalityHasOnlyCXXUses(CE))
-        return false;
-      continue;
-    }
-
-    // Otherwise, it has to be a landingpad instruction.
-    llvm::LandingPadInst *LPI = dyn_cast<llvm::LandingPadInst>(U);
-    if (!LPI) return false;
-
-    for (unsigned I = 0, E = LPI->getNumClauses(); I != E; ++I) {
-      // Look for something that would've been returned by the ObjC
-      // runtime's GetEHType() method.
-      llvm::Value *Val = LPI->getClause(I)->stripPointerCasts();
-      if (LPI->isCatch(I)) {
-        // Check if the catch value has the ObjC prefix.
-        if (llvm::GlobalVariable *GV = dyn_cast<llvm::GlobalVariable>(Val))
-          // ObjC EH selector entries are always global variables with
-          // names starting like this.
-          if (GV->getName().startswith("OBJC_EHTYPE"))
-            return false;
-      } else {
-        // Check if any of the filter values have the ObjC prefix.
-        llvm::Constant *CVal = cast<llvm::Constant>(Val);
-        for (llvm::User::op_iterator
-               II = CVal->op_begin(), IE = CVal->op_end(); II != IE; ++II) {
-          if (llvm::GlobalVariable *GV =
-              cast<llvm::GlobalVariable>((*II)->stripPointerCasts()))
-            // ObjC EH selector entries are always global variables with
-            // names starting like this.
-            if (GV->getName().startswith("OBJC_EHTYPE"))
-              return false;
-        }
-      }
-    }
+/// Check whether a landingpad instruction uses any non-C++ features.
+static bool LandingPadHasOnlyCXXUses(llvm::LandingPadInst *LPI) {
+  return true;
+}
+
+/// Check whether a personality function could reasonably be swapped
+/// for a C++ personality function.
+static bool PersonalityHasOnlyCXXUses(llvm::Constant *Fn) {
+  for (llvm::User *U : Fn->users()) {
+    // Conditionally white-list bitcasts.
+    if (llvm::ConstantExpr *CE = dyn_cast<llvm::ConstantExpr>(U)) {
+      if (CE->getOpcode() != llvm::Instruction::BitCast) return false;
+      if (!PersonalityHasOnlyCXXUses(CE))
+        return false;
+      continue;
+    }
+
+    // Otherwise it must be a function.
+    llvm::Function *F = dyn_cast<llvm::Function>(U);
+    if (!U) return false;
+
+    for (auto BB = F->begin(), E = F->end(); BB != E; ++BB) {
+      if (BB->isLandingPad())
+        if (!LandingPadHasOnlyCXXUses(BB->getLandingPadInst()))
+          return false;
+    }
   }
 
   return true;
 }
 
 /// Try to use the C++ personality function in ObjC++.  Not doing this
 /// can cause some incompatibilities with gcc, which is more
 /// aggressive about only using the ObjC++ personality in a function

test/CodeGenObjCXX/exception-cxx.mm

+// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions -fobjc-exceptions -o - %s | FileCheck %s
+
+// rdar://problem/22155434
+namespace test0 {
+  void foo() {
+    try {
+      throw 0;
+    } catch (...) {
+      return;
+    }
+  }
+// CHECK: define void @_ZN5test03fooEv() #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*)
+}