[CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

[sgraenitz]

Summary

Pushing the CatchRetScope early causes cleanups for catch parameters to be emitted in the basic block of the catch handler instead of the catchret.dest block. This is important because the latter is not part of the catchpad and this caused code truncations due to ARC PreISel intrinsics in WinEHPrepare.

[sgraenitz]

The approach follows the C++ try/catch implementation in clang/lib/CodeGen/CGException.cpp. I think this is the correct solution for what I attempted to fix with D134866 in the backend before.

[rnk]

Thanks, I can see the bug here:
https://gcc.godbolt.org/z/1xjMYarT9
You can see how storeStrong cleanup uses the catchpad funclet when it should not.

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

36

Don't we need an exceptional cleanup here to release the exception if do_something or may_throw throw?

[sgraenitz]

Run test with -fobjc-arc-exceptions and check cleanup pads as well

[sgraenitz]

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

36

Thanks, that's a very good point! -fobjc-arc-exceptions tells clang to generate invokes here instead of calls and then we can check funclet tokens in cleanup pads too.

[rnk]

lgtm, thanks!

clang/lib/CodeGen/CGObjCRuntime.cpp

 #include "CGCleanup.h"
 #include "CGRecordLayout.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/CodeGenABITypes.h"
+#include "llvm/IR/Instruction.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;
 using namespace CodeGen;
 
 uint64_t CGObjCRuntime::ComputeIvarBaseOffset(CodeGen::CodeGenModule &CGM,
                                               const ObjCInterfaceDecl *OID,
                                               const ObjCIvarDecl *Ivar) {
void CGObjCRuntime::EmitTryCatchStmt(CodeGenFunction &CGF,
   // Remember where we were.
   CGBuilderTy::InsertPoint SavedIP = CGF.Builder.saveAndClearIP();
 
   // Emit the handlers.
   for (unsigned I = 0, E = Handlers.size(); I != E; ++I) {
     CatchHandler &Handler = Handlers[I];
 
     CGF.EmitBlock(Handler.Block);
-    llvm::CatchPadInst *CPI = nullptr;
-    SaveAndRestore<llvm::Instruction *> RestoreCurrentFuncletPad(CGF.CurrentFuncletPad);
-    if (useFunclets)
-      if ((CPI = dyn_cast_or_null<llvm::CatchPadInst>(Handler.Block->getFirstNonPHI()))) {
+
+    CodeGenFunction::LexicalScope Cleanups(CGF, Handler.Body->getSourceRange());
+    SaveAndRestore<llvm::Instruction *> RevertAfterScope(CGF.CurrentFuncletPad);
+    if (useFunclets) {
+      llvm::Instruction *CPICandidate = Handler.Block->getFirstNonPHI();
+      if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
         CGF.CurrentFuncletPad = CPI;
         CPI->setOperand(2, CGF.getExceptionSlot().getPointer());
-      }
+        CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
+      }
+
     llvm::Value *RawExn = CGF.getExceptionFromSlot();
 
     // Enter the catch.
     llvm::Value *Exn = RawExn;
     if (beginCatchFn)
       Exn = CGF.EmitNounwindRuntimeCall(beginCatchFn, RawExn, "exn.adjusted");
 
-    CodeGenFunction::LexicalScope cleanups(CGF, Handler.Body->getSourceRange());
-
     if (endCatchFn) {
       // Add a cleanup to leave the catch.
       bool EndCatchMightThrow = (Handler.Variable == nullptr);
 
       CGF.EHStack.pushCleanup<CallObjCEndCatch>(NormalAndEHCleanup,
                                                 EndCatchMightThrow,
                                                 endCatchFn);
     }
 
     // Bind the catch parameter if it exists.
     if (const VarDecl *CatchParam = Handler.Variable) {
       llvm::Type *CatchType = CGF.ConvertType(CatchParam->getType());
       llvm::Value *CastExn = CGF.Builder.CreateBitCast(Exn, CatchType);
 
       CGF.EmitAutoVarDecl(*CatchParam);
       EmitInitOfCatchParam(CGF, CastExn, CatchParam);
     }
-    if (CPI)
-        CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
 
     CGF.ObjCEHValueStack.push_back(Exn);
     CGF.EmitStmt(Handler.Body);
     CGF.ObjCEHValueStack.pop_back();
 
     // Leave any cleanups associated with the catch.
-    cleanups.ForceCleanup();
+    Cleanups.ForceCleanup();
 
     CGF.EmitBranchThroughCleanup(Cont);
   }
 
   // Go back to the try-statement fallthrough.
   CGF.Builder.restoreIP(SavedIP);
 
   // Pop out of the finally.

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

-// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-arc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
 
 // WinEH requires funclet tokens on nounwind intrinsics if they can lower to
 // regular function calls in the course of IR transformations.
 //
 // This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
-// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
-// refer to their catchpad's SSA value.
+// funclet tokens for llvm.objc.* calls inside catch- and cleanup-pads and that
+// they refer to their pad's SSA value.
 
-@class Ety;
-void opaque(void);
-void test_catch_with_objc_intrinsic(void) {
+void do_something();
+void may_throw(id);
+
+void try_catch_with_objc_intrinsic() {
+  id ex;
   @try {
-    opaque();
-  } @catch (Ety *ex) {
-    // Destroy ex when leaving catchpad. This emits calls to intrinsic functions
-    // llvm.objc.retain and llvm.objc.storeStrong
+    may_throw(ex);
+  } @catch (id ex_caught) {
+    do_something();
+    may_throw(ex_caught);
   }
 }
 
-// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_with_objc_intrinsic
-//                ...
-// CHECK:       catch.dispatch:
-// CHECK-NEXT:    [[CATCHSWITCH:%[0-9]+]] = catchswitch within none
-//                ...
-// CHECK:       catch:
-// CHECK-NEXT:    [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
-// CHECK:         {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
-// CHECK:         call {{.*}} @llvm.objc.storeStrong{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK-LABEL:   try_catch_with_objc_intrinsic
+//
+// CHECK:         catch.dispatch:
+// CHECK-NEXT:      [[CATCHSWITCH:%[0-9]+]] = catchswitch within none [label %catch] unwind label %[[CLEANUP1:.*]]
+//
+// All calls within a catchpad must have funclet tokens that refer to it:
+// CHECK:         catch:
+// CHECK-NEXT:      [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK:           call
+// CHECK:             @llvm.objc.retain
+// CHECK:             [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:           invoke
+// CHECK:             do_something
+// CHECK:             [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:             unwind label %[[CLEANUP2:.*]]
+// CHECK:           invoke
+// CHECK:             may_throw
+// CHECK:             [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:             unwind label %[[CLEANUP2]]
+// CHECK:           call
+// CHECK:             @llvm.objc.storeStrong
+// CHECK:             [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:           catchret from [[CATCHPAD]] to label %catchret.dest
+//
+// This block exists and it's empty:
+// CHECK:         catchret.dest:
+// CHECK-NEXT:      br label %eh.cont
+//
+// CHECK:         [[CLEANUP2]]:
+// CHECK-NEXT:      [[CLEANUPPAD2:%[0-9]+]] = cleanuppad within [[CATCHPAD]]
+// CHECK:           call
+// CHECK:             @llvm.objc.storeStrong
+// CHECK:             [ "funclet"(token [[CLEANUPPAD2]]) ]
+// CHECK:           cleanupret from [[CLEANUPPAD2]]
+// CHECK:             unwind label %[[CLEANUP1]]
+//
+// CHECK:         [[CLEANUP1]]:
+// CHECK-NEXT:      [[CLEANUPPAD1:%[0-9]+]] = cleanuppad within none
+// CHECK:           call
+// CHECK:             @llvm.objc.storeStrong
+// CHECK:             [ "funclet"(token [[CLEANUPPAD1]]) ]
+// CHECK:           cleanupret from [[CLEANUPPAD1]] unwind to caller