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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

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.



================
Comment at: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp:113
+    if (CI->hasFnAttr(Attribute::NoUnwind))
+      NewCI->addFnAttr(Attribute::NoUnwind);
 
----------------
sgraenitz wrote:
> efriedma wrote:
> > Are all these functions actually nounwind?  Destroying objects can call into user code.
> 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?
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.


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:969
 
-        // Skip call sites which are nounwind intrinsics or inline asm.
-        auto *CalledFn =
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124680/new/

https://reviews.llvm.org/D124680