Fix compiler crashes when instrumenting programs with nested Cilk contexts and nested reducers

clang/test/Cilk/cilk-mixed-unwind-codegen.cpp

@@ -0,0 +1,37 @@
+// Check that Clang may generate functions calls that can throw with or without
+// a landingpad in the same Cilk scope.
+//
+// RUN: %clang_cc1 -fopencilk -fcxx-exceptions -fexceptions -ftapir=none -triple x86_64-unknown-linux-gnu -std=c++11 -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+int bar(int n);
+void foo(int n) {
+    cilk_for (int i = 0; i < n; ++i) {
+        int w = bar(i);
+        throw bar(w);
+    }
+}
+
+// CHECK-LABEL: define {{.*}}void @_Z3fooi(i32 {{.*}}%n)
+
+// Check for detach with an unwind destination
+// CHECK: detach within %[[SYNCREG:.+]], label %[[PFOR_BODY_ENTRY:.+]], label %[[PFOR_INC:.+]] unwind label %[[DETACH_LPAD:.+]]
+
+// CHECK: [[PFOR_BODY_ENTRY]]:
+
+// Check for call to function bar that might throw.
+// CHECK: call {{.*}}i32 @_Z3bari(i32
+
+// Check for invoke of function bar
+// CHECK: invoke noundef i32 @_Z3bari(i32
+// CHECK-NEXT: to label %[[INVOKE_CONT:.+]] unwind label %[[TASK_LPAD:.+]]
+
+// CHECK: [[INVOKE_CONT]]:
+// CHECK: call void @__cxa_throw(ptr
+// CHECK-NEXT: unreachable
+
+// CHECK: [[TASK_LPAD]]:
+// CHECK-NEXT: landingpad
+// CHECK-NEXT: cleanup
+// CHECK: invoke void @llvm.detached.rethrow.sl_p0i32s(token %[[SYNCREG]], { ptr, i32 } %{{.*}})
+// CHECK-NEXT: to label %[[UNREACHABLE:.+]] unwind label %[[DETACH_LPAD]]

llvm/include/llvm/Transforms/Utils/TapirUtils.h

@@ -231,8 +231,12 @@ BasicBlock *CreateSubTaskUnwindEdge(Intrinsic::ID TermFunc, Value *Token,
 
 /// promoteCallsInTasksToInvokes - Traverse the control-flow graph of F to
 /// convert calls to invokes, recursively traversing tasks and taskframes to
-/// insert appropriate detached.rethrow and taskframe.resume terminators.
-void promoteCallsInTasksToInvokes(Function &F, const Twine Name = "cleanup");
+/// insert appropriate detached.rethrow and taskframe.resume terminators.  The
+/// optional \p IgnoreFunctionCheck parameter allows the caller to handle some
+/// call sites in a custom manner.
+void promoteCallsInTasksToInvokes(
+    Function &F, const Twine Name = "cleanup",
+    std::function<bool(CallBase *)> IgnoreFunctionCheck = nullptr);
 
 /// eraseTaskFrame - Remove the specified taskframe and all uses of it.  The
 /// given \p TaskFrame should correspond to a taskframe.create call.  The

llvm/lib/Transforms/Instrumentation/ComprehensiveStaticInstrumentation.cpp

@@ -24,12 +24,14 @@
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/IR/CFG.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/EHPersonalities.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
@@ -746,7 +748,15 @@ void CSIImpl::setupCalls(Function &F) {
   if (F.doesNotThrow())
     return;
 
-  promoteCallsInTasksToInvokes(F, "csi.cleanup");
+  promoteCallsInTasksToInvokes(F, "csi.cleanup", [](CallBase *CB) {
+    if (const Function *F = CB->getCalledFunction()) {
+      if (F->getName().starts_with("__asan")) {
+        CB->setDoesNotThrow();
+        return true;
+      }
+    }
+    return false;
+  });
 }
 
 static BasicBlock *splitOffPreds(BasicBlock *BB,
@@ -1130,10 +1140,19 @@ bool CSIImpl::instrumentMemIntrinsic(Instruction *I) {
 }
 
 void CSIImpl::instrumentBasicBlock(BasicBlock &BB, const TaskInfo &TI) {
-  IRBuilder<> IRB(&*BB.getFirstInsertionPt());
+  Instruction *InsertPt = &*BB.getFirstInsertionPt();
   bool IsEntry = isEntryBlock(BB, TI);
   if (IsEntry)
-    IRB.SetInsertPoint(getEntryBBInsertPt(BB));
+    InsertPt = getEntryBBInsertPt(BB);
+  // Skip any sync.unwind intrinsics, which need to remain paired with
+  // corresponding syncs.
+  if (isSyncUnwind(InsertPt))
+    InsertPt = InsertPt->getNextNode();
+  // Skip any taskframe.end intrinsics, to keep the basic-block instrumentation
+  // in the same basic block.
+  if (isTapirIntrinsic(Intrinsic::taskframe_end, InsertPt))
+    InsertPt = InsertPt->getNextNode();
+  IRBuilder<> IRB(InsertPt);
   uint64_t LocalId = BasicBlockFED.add(BB);
   uint64_t BBSizeId = BBSize.add(BB, GetTTI ?
                                  &(*GetTTI)(*BB.getParent()) : nullptr);
@@ -1227,8 +1246,24 @@ void CSIImpl::instrumentLoop(Loop &L, TaskInfo &TI, ScalarEvolution *SE) {
   insertHookCall(&*IRB.GetInsertPoint(), CsiLoopBodyEntry, {LoopCsiId,
                                                             LoopPropVal});
 
+  SmallPtrSet<BasicBlock *, 4> ExitingBlocksVisited;
   // Insert hooks at the ends of the exiting blocks.
-  for (BasicBlock *BB : ExitingBlocks) {
+  while (!ExitingBlocks.empty()) {
+    BasicBlock *BB = ExitingBlocks.pop_back_val();
+    if (!ExitingBlocksVisited.insert(BB).second)
+      continue;
+    if (isSyncUnwind(BB->getTerminator())) {
+      // Insert the loopbody_exit hook before the sync instruction, rather than
+      // the sync.unwind.
+      // TODO: I don't think there's anything preventing a sync.unwind from
+      // having multiple sync-instruction predecessors, so all such predecessors
+      // need to be addressed.  This logic should become simpler if sync itself
+      // is modified to have an unwind destination.
+      for (BasicBlock *Pred : predecessors(BB))
+        ExitingBlocks.push_back(Pred);
+      continue;
+    }
+
     // Record properties of this loop exit
     CsiLoopExitProperty LoopExitProp;
     LoopExitProp.setIsLatch(L.isLoopLatch(BB));
@@ -1798,13 +1833,16 @@ CallInst *CSIImpl::insertHookCallInSuccessorBB(BasicBlock *Succ, BasicBlock *BB,
                                                ArrayRef<Value *> HookArgs,
                                                ArrayRef<Value *> DefaultArgs) {
   assert(HookFunction && "No hook function given.");
+  Instruction *InsertPt = &*Succ->getFirstInsertionPt();
+  if (isSyncUnwind(InsertPt))
+    InsertPt = InsertPt->getNextNode();
+
   // If this successor block has a unique predecessor, just insert the hook call
   // as normal.
   if (Succ->getUniquePredecessor()) {
     assert(Succ->getUniquePredecessor() == BB &&
            "BB is not unique predecessor of successor block");
-    return insertHookCall(&*Succ->getFirstInsertionPt(), HookFunction,
-                          HookArgs);
+    return insertHookCall(InsertPt, HookFunction, HookArgs);
   }
 
   if (updateArgPHIs(Succ, BB, HookFunction, HookArgs, DefaultArgs))
@@ -1815,7 +1853,7 @@ CallInst *CSIImpl::insertHookCallInSuccessorBB(BasicBlock *Succ, BasicBlock *BB,
   for (PHINode *ArgPHI : ArgPHIs[Key])
     SuccessorHookArgs.push_back(ArgPHI);
 
-  IRBuilder<> IRB(&*Succ->getFirstInsertionPt());
+  IRBuilder<> IRB(InsertPt);
   // Insert the hook call, using the PHI as the CSI ID.
   CallInst *Call = IRB.CreateCall(HookFunction, SuccessorHookArgs);
   setInstrumentationDebugLoc(*Succ, (Instruction *)Call);
@@ -2739,6 +2777,11 @@ void CSIImpl::instrumentFunction(Function &F) {
     for (BasicBlock *BB : BasicBlocks)
       instrumentBasicBlock(*BB, TI);
 
+  if (Options.InstrumentLoops)
+    // Recursively instrument all loops
+    for (Loop *L : LI)
+      instrumentLoop(*L, TI, SE);
+
   // Instrument Tapir constructs.
   if (Options.InstrumentTapir) {
     if (Config->DoesFunctionRequireInstrumentationForPoint(
@@ -2760,11 +2803,6 @@ void CSIImpl::instrumentFunction(Function &F) {
     for (Instruction *I : Allocas)
       instrumentAlloca(I, TI);
 
-  if (Options.InstrumentLoops)
-    // Recursively instrument all loops
-    for (Loop *L : LI)
-      instrumentLoop(*L, TI, SE);
-
   // Do this work in a separate loop after copying the iterators so that we
   // aren't modifying the list as we're iterating.
   if (Options.InstrumentMemoryAccesses)

llvm/lib/Transforms/Tapir/LoopSpawningTI.cpp

@@ -688,23 +688,10 @@ void DACSpawning::implementDACIterSpawnOnHelper(
         SplitBlock(Preheader, &Preheader->front(), (DomTreeUpdater *)nullptr,
                    nullptr, nullptr, Preheader->getName() + ".dac.head");
 
-    // Move any syncregion_start's in DACHead into Preheader.
-    BasicBlock::iterator InsertPoint = Preheader->begin();
-    for (BasicBlock::iterator I = DACHead->begin(), E = DACHead->end();
-         I != E;) {
-      IntrinsicInst *II = dyn_cast<IntrinsicInst>(I++);
-      if (!II)
-        continue;
-      if (Intrinsic::syncregion_start != II->getIntrinsicID())
-        continue;
-
-      while (isa<IntrinsicInst>(I) &&
-             Intrinsic::syncregion_start ==
-                 cast<IntrinsicInst>(I)->getIntrinsicID())
-        ++I;
-
-      Preheader->splice(InsertPoint, &*DACHead, II->getIterator(), I);
-    }
+    // Move the syncregion corresponding with the original loop into Preheader,
+    // so the new detach can use it.
+    if (Instruction *SyncRegionI = dyn_cast<Instruction>(SyncRegion))
+      SyncRegionI->moveBefore(&*Preheader->getFirstInsertionPt());
 
     if (!Preheader->getTerminator()->getDebugLoc())
       Preheader->getTerminator()->setDebugLoc(

llvm/lib/Transforms/Utils/TapirUtils.cpp

@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/TapirUtils.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
 #include "llvm/Analysis/LoopInfo.h"
@@ -85,16 +86,13 @@ bool llvm::isSkippableTapirIntrinsic(const Instruction *I) {
 /// Returns true if the given basic block \p B is a placeholder successor of a
 /// taskframe.resume or detached.rethrow.
 bool llvm::isTapirPlaceholderSuccessor(const BasicBlock *B) {
-  for (const BasicBlock *Pred : predecessors(B)) {
+  return llvm::any_of(predecessors(B), [&](const BasicBlock *Pred) {
     if (!isDetachedRethrow(Pred->getTerminator()) &&
         !isTaskFrameResume(Pred->getTerminator()))
       return false;
-
     const InvokeInst *II = dyn_cast<InvokeInst>(Pred->getTerminator());
-    if (B != II->getNormalDest())
-      return false;
-  }
-  return true;
+    return B == II->getNormalDest();
+  });
 }
 
 /// Returns a taskframe.resume that uses the given taskframe, or nullptr if no
@@ -1816,6 +1814,8 @@ void llvm::fixupTaskFrameExternalUses(Spindle *TF, const TaskInfo &TI,
 
       // Examine all users of this instruction.
       for (Use &U : I.uses()) {
+        if (!DT.isReachableFromEntry(U))
+          continue;
         // If we find a live use outside of the task, it's an output.
         if (Instruction *UI = dyn_cast<Instruction>(U.getUser())) {
           if (!taskFrameEncloses(TF, UI->getParent(), TI)) {
@@ -2005,9 +2005,9 @@ BasicBlock *llvm::CreateSubTaskUnwindEdge(Intrinsic::ID TermFunc, Value *Token,
   return NewUnwindEdge;
 }
 
-static BasicBlock *maybePromoteCallInBlock(BasicBlock *BB,
-                                           BasicBlock *UnwindEdge,
-                                           const Value *TaskFrame) {
+static BasicBlock *maybePromoteCallInBlock(
+    BasicBlock *BB, BasicBlock *UnwindEdge, const Value *TaskFrame,
+    std::function<bool(CallBase *)> IgnoreFunctionCheck) {
   for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E;) {
     Instruction *I = &*BBI++;
 
@@ -2031,6 +2031,8 @@ static BasicBlock *maybePromoteCallInBlock(BasicBlock *BB,
     // We cannot transform calls with musttail tag.
     if (CI->isMustTailCall())
       continue;
+    if (IgnoreFunctionCheck && IgnoreFunctionCheck(CI))
+      continue;
 
     // We do not need to (and in fact, cannot) convert possibly throwing calls
     // to @llvm.experimental_deoptimize (resp. @llvm.experimental.guard) into
@@ -2073,10 +2075,10 @@ static Instruction *getTaskFrameInstructionInBlock(BasicBlock *BB,
 
 // Recursively handle inlined tasks.
 static void promoteCallsInTasksHelper(
-    BasicBlock *EntryBlock, BasicBlock *UnwindEdge,
-    BasicBlock *Unreachable, Value *CurrentTaskFrame,
-    SmallVectorImpl<BasicBlock *> *ParentWorklist,
-    SmallPtrSetImpl<BasicBlock *> &Processed) {
+    BasicBlock *EntryBlock, BasicBlock *UnwindEdge, BasicBlock *Unreachable,
+    Value *CurrentTaskFrame, SmallVectorImpl<BasicBlock *> *ParentWorklist,
+    SmallPtrSetImpl<BasicBlock *> &Processed,
+    std::function<bool(CallBase *)> IgnoreFunctionCheck) {
   SmallVector<DetachInst *, 8> DetachesToReplace;
   SmallVector<BasicBlock *, 32> Worklist;
   // TODO: See if we need a global Visited set over all recursive calls, i.e.,
@@ -2090,8 +2092,8 @@ static void promoteCallsInTasksHelper(
       continue;
 
     // Promote any calls in the block to invokes.
-    while (BasicBlock *NewBB =
-           maybePromoteCallInBlock(BB, UnwindEdge, CurrentTaskFrame))
+    while (BasicBlock *NewBB = maybePromoteCallInBlock(
+               BB, UnwindEdge, CurrentTaskFrame, IgnoreFunctionCheck))
       BB = cast<InvokeInst>(NewBB->getTerminator())->getNormalDest();
 
     Instruction *TFI = getTaskFrameInstructionInBlock(BB, CurrentTaskFrame);
@@ -2113,7 +2115,8 @@ static void promoteCallsInTasksHelper(
 
         // Recursively check all blocks
         promoteCallsInTasksHelper(NewBB, TaskFrameUnwindEdge, Unreachable,
-                                  TFCreate, &Worklist, Processed);
+                                  TFCreate, &Worklist, Processed,
+                                  IgnoreFunctionCheck);
 
         // Remove the unwind edge for the taskframe if it is not needed.
         if (pred_empty(TaskFrameUnwindEdge))
@@ -2161,30 +2164,28 @@ static void promoteCallsInTasksHelper(
     // spawned task recursively.
     if (DetachInst *DI = dyn_cast<DetachInst>(BB->getTerminator())) {
       Processed.insert(BB);
-      if (!DI->hasUnwindDest()) {
-        // Create an unwind edge for the subtask, which is terminated with a
-        // detached-rethrow.
-        BasicBlock *SubTaskUnwindEdge = CreateSubTaskUnwindEdge(
-            Intrinsic::detached_rethrow, DI->getSyncRegion(), UnwindEdge,
-            Unreachable, DI);
-        // Recursively check all blocks in the detached task.
-        promoteCallsInTasksHelper(DI->getDetached(), SubTaskUnwindEdge,
-                                  Unreachable, CurrentTaskFrame, &Worklist,
-                                  Processed);
-        // If the new unwind edge is not used, remove it.
-        if (pred_empty(SubTaskUnwindEdge))
-          SubTaskUnwindEdge->eraseFromParent();
-        else
-          DetachesToReplace.push_back(DI);
 
-      } else {
-        // Because this detach has an unwind destination, Any calls in the
-        // spawned task that may throw should already be invokes.  Hence there
-        // is no need to promote calls in this task.
-        if (Visited.insert(DI->getUnwindDest()).second)
-          // If the detach-unwind isn't dead, add it to the worklist.
-          Worklist.push_back(DI->getUnwindDest());
-      }
+      // Create an unwind edge for the subtask, which is terminated with a
+      // detached-rethrow.
+      BasicBlock *SubTaskUnwindEdge = CreateSubTaskUnwindEdge(
+          Intrinsic::detached_rethrow, DI->getSyncRegion(),
+          DI->hasUnwindDest() ? DI->getUnwindDest() : UnwindEdge, Unreachable,
+          DI);
+      // Recursively check all blocks in the detached task.
+      promoteCallsInTasksHelper(DI->getDetached(), SubTaskUnwindEdge,
+                                Unreachable, CurrentTaskFrame, &Worklist,
+                                Processed, IgnoreFunctionCheck);
+
+      // If the new unwind edge is not used, remove it.
+      if (pred_empty(SubTaskUnwindEdge))
+        SubTaskUnwindEdge->eraseFromParent();
+      else if (!DI->hasUnwindDest())
+        DetachesToReplace.push_back(DI);
+
+      if (DI->hasUnwindDest() && Visited.insert(DI->getUnwindDest()).second)
+        // If the detach-unwind isn't dead, add it to the worklist.
+        Worklist.push_back(DI->getUnwindDest());
+
       // Add the continuation to the worklist.
       if (isTaskFrameResume(UnwindEdge->getTerminator()) &&
           (CurrentTaskFrame == getTaskFrameUsed(DI->getDetached()))) {
@@ -2223,7 +2224,9 @@ static FunctionCallee getDefaultPersonalityFn(Module *M) {
                                 FunctionType::get(Type::getInt32Ty(C), true));
 }
 
-void llvm::promoteCallsInTasksToInvokes(Function &F, const Twine Name) {
+void llvm::promoteCallsInTasksToInvokes(
+    Function &F, const Twine Name,
+    std::function<bool(CallBase *)> IgnoreFunctionCheck) {
   // Collect blocks to process, in order to handle unreachable blocks.
   SmallVector<BasicBlock *, 8> ToProcess;
   ToProcess.push_back(&F.getEntryBlock());
@@ -2254,7 +2257,7 @@ void llvm::promoteCallsInTasksToInvokes(Function &F, const Twine Name) {
   for (BasicBlock *BB : ToProcess) {
     if (!Processed.contains(BB))
       promoteCallsInTasksHelper(BB, CleanupBB, UnreachableBlk, nullptr, nullptr,
-                                Processed);
+                                Processed, IgnoreFunctionCheck);
   }
 
   // Either finish inserting the cleanup block (and associated data) or remove