[ObjCARC] Prevent code motion into a catchswitch

[smeenai]

Summary

A catchswitch must be the only non-phi instruction in its basic block;
attempting to move a retain or release into a catchswitch basic block
will result in invalid IR. Explicitly mark a CFG hazard in this case to
prevent the code motion.

Fixes PR37332.

[rjmccall]

I'm going to have to leave this one to Akira or Michael, sorry.

[smeenai]

No worries; thanks for all the other reviews, and for adding Michael as a reviewer to this one :)

[smeenai]

Ping.

[ahatanak]

lib/Transforms/ObjCARC/ObjCARCOpts.cpp

1577 ↗

(On Diff #145323)

If you want to set CFGHazardAfflicted inside BottomUpPtrState::HandlePotentialUse, perhaps you can read NewRetainRRI's CFGHazardAfflicted bit and set CFGHazardAfflicted here, just as it's done in line 1649?

1699 ↗

(On Diff #145323)

This is preventing releases from being inserted before a CatchSwitchInst. Is that correct? Do you have to prevent retains from being inserted before CatchSwitchInsts too somewhere, or we don't have to worry about it because it never happens?

[rnk]

lib/Transforms/ObjCARC/ObjCARCOpts.cpp

1699 ↗

(On Diff #145323)

Yes, a catchswitch must be the first and last non-phi instruction in the BB. It's BB must be empty. It only exists to multiplex unwind edges from invokes.

[smeenai]

lib/Transforms/ObjCARC/ObjCARCOpts.cpp

1577 ↗

(On Diff #145323)

Let me try that, thanks!

1699 ↗

(On Diff #145323)

I haven't seen retains be inserted before a catchswitch. I think that's because only the bottom-up analysis does the special treatment of invoke instructions, where they're analyzed as part of the successor basic blocks instead.

[smeenai]

@ahatanak's suggestion

[ahatanak]

LGTM

llvm/trunk/lib/Transforms/ObjCARC/ObjCARCOpts.cpp

bool ObjCARCOpt::PairUpRetainsAndReleases(
   bool FirstRelease = true;
   for (SmallVector<Instruction *, 4> NewRetains{Retain};;) {
     SmallVector<Instruction *, 4> NewReleases;
     for (Instruction *NewRetain : NewRetains) {
       auto It = Retains.find(NewRetain);
       assert(It != Retains.end());
       const RRInfo &NewRetainRRI = It->second;
       KnownSafeTD &= NewRetainRRI.KnownSafe;
+      CFGHazardAfflicted |= NewRetainRRI.CFGHazardAfflicted;
       for (Instruction *NewRetainRelease : NewRetainRRI.Calls) {
         auto Jt = Releases.find(NewRetainRelease);
         if (Jt == Releases.end())
           return false;
         const RRInfo &NewRetainReleaseRRI = Jt->second;
 
         // If the release does not have a reference to the retain as well,
         // something happened which is unaccounted for. Do not do anything.

llvm/trunk/lib/Transforms/ObjCARC/PtrState.cpp

auto SetSeqAndInsertReverseInsertPt = [&](Sequence NewSeq){
     SetSeq(NewSeq);
     // If this is an invoke instruction, we're scanning it as part of
     // one of its successor blocks, since we can't insert code after it
     // in its own block, and we don't want to split critical edges.
     BasicBlock::iterator InsertAfter;
     if (isa<InvokeInst>(Inst)) {
       const auto IP = BB->getFirstInsertionPt();
       InsertAfter = IP == BB->end() ? std::prev(BB->end()) : IP;
+      if (isa<CatchSwitchInst>(InsertAfter))
+        // A catchswitch must be the only non-phi instruction in its basic
+        // block, so attempting to insert an instruction into such a block would
+        // produce invalid IR.
+        SetCFGHazardAfflicted(true);
     } else {
       InsertAfter = std::next(Inst->getIterator());
     }
     InsertReverseInsertPt(&*InsertAfter);
   };
 
   // Check for possible direct uses.
   switch (GetSeq()) {

llvm/trunk/test/Transforms/ObjCARC/opt-catchswitch.ll

+; RUN: opt -S -objc-arc < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686--windows-msvc"
+
+declare i8* @f(i8*, i8*)
+
+declare i32 @__CxxFrameHandler3(...)
+
+declare dllimport i8* @objc_autoreleaseReturnValue(i8* returned)
+declare dllimport i8* @objc_retain(i8* returned)
+declare dllimport i8* @objc_retainAutoreleasedReturnValue(i8* returned)
+declare dllimport void @objc_release(i8*)
+
+define i8* @g(i8* %p, i8* %q) local_unnamed_addr personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %0 = tail call i8* @objc_retain(i8* %p) #0
+  %1 = tail call i8* @objc_retain(i8* %q) #0
+  %call = invoke i8* @f(i8* %p, i8* %q)
+          to label %invoke.cont unwind label %catch.dispatch, !clang.arc.no_objc_arc_exceptions !0
+
+catch.dispatch:
+  %2 = catchswitch within none [label %catch] unwind to caller
+
+catch:
+  %3 = catchpad within %2 [i8* null, i32 64, i8* null]
+  catchret from %3 to label %cleanup
+
+invoke.cont:
+  %4 = tail call i8* @objc_retainAutoreleasedReturnValue(i8* %call) #0
+  br label %cleanup
+
+cleanup:
+  %retval.0 = phi i8* [ %call, %invoke.cont ], [ null, %catch ]
+  tail call void @objc_release(i8* %q) #0, !clang.imprecise_release !0
+  tail call void @objc_release(i8* %p) #0, !clang.imprecise_release !0
+  %5 = tail call i8* @objc_autoreleaseReturnValue(i8* %retval.0) #0
+  ret i8* %retval.0
+}
+
+; CHECK-LABEL: entry:
+; CHECK-NEXT:    %0 = tail call i8* @objc_retain(i8* %p) #0
+; CHECK-NEXT:    %call = invoke i8* @f(i8* %p, i8* %q)
+; CHECK-NEXT:            to label %invoke.cont unwind label %catch.dispatch
+
+; CHECK-LABEL: catch.dispatch:
+; CHECK-NEXT:    %1 = catchswitch within none [label %catch] unwind to caller
+
+; CHECK-LABEL: cleanup:
+; CHECK:         tail call void @objc_release(i8* %p) #0
+
+attributes #0 = { nounwind }
+
+!0 = !{}