[WinEHPrepare] Fix accidental truncation of EH funclets with GNUstep ObjC runtime

[sgraenitz]

Summary

Unwinding ObjC code involves calls to intrinsics like llvm.obj.retain and llvm.objc.destroyWeak. Exception handling on Windows is based on funclets and WinEHPrepare only allows calls to nounwind intrinsics. This works just fine, except for ObjC nounwind intrinsics, because these are lowered into regular function calls in an earlier stage already (i.e. PreISelIntrinsicLoweringPass). Thus, WinEHPrepare accidentally drops them as implausible instructions and silently truncates certain funclets. Eventually, this causes crashes during unwinding on Windows with the GNUstep ObjC runtime [1].

This patch is a first attempt to fix the issue: PreISelIntrinsicLoweringPass passes on nounwind attributes to lowered call sites and WinEHPrepare gets a little less conservative by allowing any nounwind calls. It fixes the issue in GNUstep libobjc2 [1] and related projects. I will work on a test case during the next weeks and I understand if this is not immediately ready to land upstream. For the moment, I am looking for feedback to avoid unintended side effects.

Thanks for all input!

[1] https://github.com/gnustep/libobjc2/issues/222

[sgraenitz]

Added some more details here on Github:
https://github.com/weliveindetail/libobjc2/commit/cfd3ede57901fa1d294268d8ef2ab992fefa3fb1#r72508265

[efriedma]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

113

Are all these functions actually nounwind? Destroying objects can call into user code.

[triplef]

I think the original issue and these changes might also be related to these frontend crashes when using @try/catch with @finally in Objective-C code on Windows:

https://github.com/llvm/llvm-project/issues/43828
https://github.com/llvm/llvm-project/issues/51899

I will try to verify this next week.

[sgraenitz]

There is a second attempt here, that I am more uncertain about: https://github.com/weliveindetail/llvm-project/commit/93e830eb1b9280d08e3f082b746a8d1343d413ac

However, the behavior of CodeGenFunction::getBundlesForFunclet() appears related. It works, because all the relevant ObjC ARC calls will have the "funclet" entry in their bundle list. I had to extend the respective WinEHPrepare condition, because sometimes funclet pads had disappeared (via optimizations I assume?).

I don't fully understand the intended behavior of the existing code and so I can't say which one might be closer to a correct solution (or whether they are both far away).

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

113

You mean all intrinsics lowered pre-ISel? Not sure, so far we observed the following ones to trigger truncation and from the IR I looked at they all were all declared nounwind:
objc_destroyWeak, objc_release, objc_retain, objc_retainAutoreleasedReturnValue, objc_storeStrong

Yes destruction likely calls back into user code. In C++ throwing from destructors is undefined behavior. Is that the case in ObjC as well?

[rnk]

Which pass inserts the calls to these intrinsics? Can that pass be responsible for calculating funclet colors and adding the operand bundles to the intrinsics? That would match what we do for other instrumentation passes (ASan, PGO). The pre isel lowering pass can simply carry over the funclet operand bundle if present. I think there is a utility for carrying over that and similar operand bundles.

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

113

I think if the original intrinsic call is nounwind, it is safe to transfer that onto the lowered replacement call regardless of how it interacts with WinEH, other transforms will have already optimized assuming this instruction doesn't throw exceptions.

llvm/lib/CodeGen/WinEHPrepare.cpp

969

I'd rather keep this restricted to intrinsics if possible. The funclet bundle operands exist to make sure that we don't have to clone common blocks with any interesting EH state. They prevent mid level passes from doing things like tail merging, or merging identical regions of IR.

The design is somewhat complicated because it tries to defend against all possible legal IR transforms, not just the ones in tree. Unfortunately, the design makes it really hard for any pass after the frontend IR generation to correctly insert call instructions, and that probably includes ARC.

[sgraenitz]

Thanks for your comments Reid, this was really helpful!

In D124680#3483821, @rnk wrote:

Which pass inserts the calls to these intrinsics? Can that pass be responsible for calculating funclet colors and adding the operand bundles to the intrinsics?

Yes, my second attempt was a basic sketch for that, but I wasn't sure it's going in the right direction. With your explanation it makes a lot more sense now.

So, I did some polishing and set up a new review here: D124762

llvm/lib/CodeGen/WinEHPrepare.cpp

969

Ok, my patch in the new review avoids it and uses bundle operands instead.

[ahatanak]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

113

Retain and release methods under ARC aren't allowed to raise exceptions, so I assume llvm.objc.retain and llvm.objc.release don't raise exceptions either.

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#retain-count-semantics

I guess the other methods are marked as nounwind for similar reasons.

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

for (Use &U : llvm::make_early_inc_range(F.uses())) {
 
     auto *CI = cast<CallInst>(CB);
     assert(CI->getCalledFunction() && "Cannot lower an indirect call!");
 
     IRBuilder<> Builder(CI->getParent(), CI->getIterator());
     SmallVector<Value *, 8> Args(CI->args());
     CallInst *NewCI = Builder.CreateCall(FCache, Args);
     NewCI->setName(CI->getName());
+    if (CI->hasFnAttr(Attribute::NoUnwind))
+      NewCI->addFnAttr(Attribute::NoUnwind);
 
     // Try to set the most appropriate TailCallKind based on both the current
     // attributes and the ones that we could get from ObjCARC's special
     // knowledge of the runtime functions.
     //
     // std::max respects both requirements of notail and tail here:
     // * notail on either the call or from ObjCARC becomes notail
     // * tail on either side is stronger than none, but not notail

llvm/lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
 
         Value *FuncletBundleOperand = nullptr;
         if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
           FuncletBundleOperand = BU->Inputs.front();
 
         if (FuncletBundleOperand == FuncletPad)
           continue;
 
-        // Skip call sites which are nounwind intrinsics or inline asm.
-        auto *CalledFn =
-            dyn_cast<Function>(CB->getCalledOperand()->stripPointerCasts());
-        if (CalledFn && ((CalledFn->isIntrinsic() && CB->doesNotThrow()) ||
-                         CB->isInlineAsm()))
+        // Skip call sites which are nounwind or inline asm.
+        if (isa<Function>(CB->getCalledOperand()->stripPointerCasts()))
+          if (CB->doesNotThrow() || CB->isInlineAsm())
             continue;
 
         // This call site was not part of this funclet, remove it.
         if (isa<InvokeInst>(CB)) {
           // Remove the unwind edge if it was an invoke.
           removeUnwindEdge(BB);
           // Get a pointer to the new call.
           BasicBlock::iterator CallI =
               std::prev(BB->getTerminator()->getIterator());