[SimplifyCFG] Stop inserting calls to llvm.trap for UB

SimplifyCFG had logic to insert calls to llvm.trap for two very
particular IR patterns: stores and invokes of undef/null.

While InstCombine canonicalizes certain undefined behavior IR patterns
to stores of undef, phase ordering means that this cannot be relied upon
in general.

There are much better tools than llvm.trap: UBSan and ASan.

N.B. I could be argued into reverting this change if a clear argument as
to why it is important that we synthesize llvm.trap for stores, I'd be
hard pressed to see why it'd be useful for invokes...

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@273778 91177308-0d34-0410-b5e6-96231b3b80d8

10 files changed, 31 insertions, 73 deletions

diff --git a/include/llvm/Transforms/Utils/Local.h b/include/llvm/Transforms/Utils/Local.h
index 43b376cf806..0decd511aae 100644
--- a/include/llvm/Transforms/Utils/Local.h
+++ b/include/llvm/Transforms/Utils/Local.h
@@ -296,7 +296,7 @@ unsigned removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB);
 
 /// Insert an unreachable instruction before the specified
 /// instruction, making it and the rest of the code in the block dead.
-unsigned changeToUnreachable(Instruction *I, bool UseLLVMTrap);
+unsigned changeToUnreachable(Instruction *I);
 
 /// Replace 'BB's terminator with one that does not have an unwind successor
 /// block. Rewrites `invoke` to `call`, etc. Updates any PHIs in unwind
diff --git a/lib/CodeGen/WinEHPrepare.cpp b/lib/CodeGen/WinEHPrepare.cpp
index 041fb7b912b..a8e089a23d2 100644
--- a/lib/CodeGen/WinEHPrepare.cpp
+++ b/lib/CodeGen/WinEHPrepare.cpp
@@ -963,9 +963,9 @@ void WinEHPrepare::removeImplausibleInstructions(Function &F) {
           BasicBlock::iterator CallI =
               std::prev(BB->getTerminator()->getIterator());
           auto *CI = cast<CallInst>(&*CallI);
-          changeToUnreachable(CI, /*UseLLVMTrap=*/false);
+          changeToUnreachable(CI);
         } else {
-          changeToUnreachable(&I, /*UseLLVMTrap=*/false);
+          changeToUnreachable(&I);
         }
 
         // There are no more instructions in the block (except for unreachable),
@@ -986,7 +986,7 @@ void WinEHPrepare::removeImplausibleInstructions(Function &F) {
         IsUnreachableCleanupret = CRI->getCleanupPad() != CleanupPad;
       if (IsUnreachableRet || IsUnreachableCatchret ||
           IsUnreachableCleanupret) {
-        changeToUnreachable(TI, /*UseLLVMTrap=*/false);
+        changeToUnreachable(TI);
       } else if (isa<InvokeInst>(TI)) {
         if (Personality == EHPersonality::MSVC_CXX && CleanupPad) {
           // Invokes within a cleanuppad for the MSVC++ personality never
diff --git a/lib/Transforms/IPO/PruneEH.cpp b/lib/Transforms/IPO/PruneEH.cpp
index 04383f4f3fa..abde3d3609a 100644
--- a/lib/Transforms/IPO/PruneEH.cpp
+++ b/lib/Transforms/IPO/PruneEH.cpp
@@ -258,7 +258,7 @@ void PruneEH::DeleteBasicBlock(BasicBlock *BB) {
 
   if (TokenInst) {
     if (!isa<TerminatorInst>(TokenInst))
-      changeToUnreachable(TokenInst->getNextNode(), /*UseLLVMTrap=*/false);
+      changeToUnreachable(TokenInst->getNextNode());
   } else {
     // Get the list of successors of this block.
     std::vector<BasicBlock *> Succs(succ_begin(BB), succ_end(BB));
diff --git a/lib/Transforms/Scalar/SCCP.cpp b/lib/Transforms/Scalar/SCCP.cpp
index 91e625b7aac..264bdb6f353 100644
--- a/lib/Transforms/Scalar/SCCP.cpp
+++ b/lib/Transforms/Scalar/SCCP.cpp
@@ -1802,8 +1802,7 @@ static bool runIPSCCP(Module &M, const DataLayout &DL,
         DEBUG(dbgs() << "  BasicBlock Dead:" << *BB);
 
         ++NumDeadBlocks;
-        NumInstRemoved +=
-            changeToUnreachable(BB->getFirstNonPHI(), /*UseLLVMTrap=*/false);
+        NumInstRemoved += changeToUnreachable(BB->getFirstNonPHI());
 
         MadeChanges = true;
 
diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp
index a06c8499e19..e4a4da8f0ed 100644
--- a/lib/Transforms/Utils/InlineFunction.cpp
+++ b/lib/Transforms/Utils/InlineFunction.cpp
@@ -1833,7 +1833,7 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
       // As such, we replace the cleanupret with unreachable.
       if (auto *CleanupRet = dyn_cast<CleanupReturnInst>(BB->getTerminator()))
         if (CleanupRet->unwindsToCaller() && EHPadForCallUnwindsLocally)
-          changeToUnreachable(CleanupRet, /*UseLLVMTrap=*/false);
+          changeToUnreachable(CleanupRet);
 
       Instruction *I = BB->getFirstNonPHI();
       if (!I->isEHPad())
diff --git a/lib/Transforms/Utils/Local.cpp b/lib/Transforms/Utils/Local.cpp
index edacfd20f99..a37e4911d8d 100644
--- a/lib/Transforms/Utils/Local.cpp
+++ b/lib/Transforms/Utils/Local.cpp
@@ -42,11 +42,13 @@
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Operator.h"
+#include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
+using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "local"
 
@@ -1310,21 +1312,13 @@ unsigned llvm::removeAllNonTerminatorAndEHPadInstructions(BasicBlock *BB) {
   return NumDeadInst;
 }
 
-unsigned llvm::changeToUnreachable(Instruction *I, bool UseLLVMTrap) {
+unsigned llvm::changeToUnreachable(Instruction *I) {
   BasicBlock *BB = I->getParent();
   // Loop over all of the successors, removing BB's entry from any PHI
   // nodes.
-  for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE; ++SI)
-    (*SI)->removePredecessor(BB);
-
-  // Insert a call to llvm.trap right before this.  This turns the undefined
-  // behavior into a hard fail instead of falling through into random code.
-  if (UseLLVMTrap) {
-    Function *TrapFn =
-      Intrinsic::getDeclaration(BB->getParent()->getParent(), Intrinsic::trap);
-    CallInst *CallTrap = CallInst::Create(TrapFn, "", I);
-    CallTrap->setDebugLoc(I->getDebugLoc());
-  }
+  for (BasicBlock *Succ : successors(BB))
+    Succ->removePredecessor(BB);
+
   new UnreachableInst(I->getContext(), I);
 
   // All instructions after this are dead.
@@ -1374,22 +1368,14 @@ static bool markAliveBlocks(Function &F,
     // Do a quick scan of the basic block, turning any obviously unreachable
     // instructions into LLVM unreachable insts.  The instruction combining pass
     // canonicalizes unreachable insts into stores to null or undef.
-    for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E;++BBI){
+    for (Instruction &I : *BB) {
       // Assumptions that are known to be false are equivalent to unreachable.
       // Also, if the condition is undefined, then we make the choice most
       // beneficial to the optimizer, and choose that to also be unreachable.
-      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BBI)) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I)) {
         if (II->getIntrinsicID() == Intrinsic::assume) {
-          bool MakeUnreachable = false;
-          if (isa<UndefValue>(II->getArgOperand(0)))
-            MakeUnreachable = true;
-          else if (ConstantInt *Cond =
-                   dyn_cast<ConstantInt>(II->getArgOperand(0)))
-            MakeUnreachable = Cond->isZero();
-
-          if (MakeUnreachable) {
-            // Don't insert a call to llvm.trap right before the unreachable.
-            changeToUnreachable(&*BBI, false);
+          if (match(II->getArgOperand(0), m_CombineOr(m_Zero(), m_Undef()))) {
+            changeToUnreachable(II);
             Changed = true;
             break;
           }
@@ -1404,19 +1390,19 @@ static bool markAliveBlocks(Function &F,
           // Note: unlike in llvm.assume, it is not "obviously profitable" for
           // guards to treat `undef` as `false` since a guard on `undef` can
           // still be useful for widening.
-          if (auto *CI = dyn_cast<ConstantInt>(II->getArgOperand(0)))
-            if (CI->isZero() && !isa<UnreachableInst>(II->getNextNode())) {
-              changeToUnreachable(II->getNextNode(), /*UseLLVMTrap=*/ false);
+          if (match(II->getArgOperand(0), m_Zero()))
+            if (!isa<UnreachableInst>(II->getNextNode())) {
+              changeToUnreachable(II->getNextNode());
               Changed = true;
               break;
             }
         }
       }
 
-      if (CallInst *CI = dyn_cast<CallInst>(BBI)) {
+      if (auto *CI = dyn_cast<CallInst>(&I)) {
         Value *Callee = CI->getCalledValue();
         if (isa<ConstantPointerNull>(Callee) || isa<UndefValue>(Callee)) {
-          changeToUnreachable(CI, /*UseLLVMTrap=*/false);
+          changeToUnreachable(CI);
           Changed = true;
           break;
         }
@@ -1424,10 +1410,8 @@ static bool markAliveBlocks(Function &F,
           // If we found a call to a no-return function, insert an unreachable
           // instruction after it.  Make sure there isn't *already* one there
           // though.
-          ++BBI;
-          if (!isa<UnreachableInst>(BBI)) {
-            // Don't insert a call to llvm.trap right before the unreachable.
-            changeToUnreachable(&*BBI, false);
+          if (!isa<UnreachableInst>(CI->getNextNode())) {
+            changeToUnreachable(CI->getNextNode());
             Changed = true;
           }
           break;
@@ -1437,7 +1421,7 @@ static bool markAliveBlocks(Function &F,
       // Store to undef and store to null are undefined and used to signal that
       // they should be changed to unreachable by passes that can't modify the
       // CFG.
-      if (StoreInst *SI = dyn_cast<StoreInst>(BBI)) {
+      if (auto *SI = dyn_cast<StoreInst>(&I)) {
         // Don't touch volatile stores.
         if (SI->isVolatile()) continue;
 
@@ -1446,7 +1430,7 @@ static bool markAliveBlocks(Function &F,
         if (isa<UndefValue>(Ptr) ||
             (isa<ConstantPointerNull>(Ptr) &&
              SI->getPointerAddressSpace() == 0)) {
-          changeToUnreachable(SI, true);
+          changeToUnreachable(SI);
           Changed = true;
           break;
         }
@@ -1458,7 +1442,7 @@ static bool markAliveBlocks(Function &F,
       // Turn invokes that call 'nounwind' functions into ordinary calls.
       Value *Callee = II->getCalledValue();
       if (isa<ConstantPointerNull>(Callee) || isa<UndefValue>(Callee)) {
-        changeToUnreachable(II, true);
+        changeToUnreachable(II);
         Changed = true;
       } else if (II->doesNotThrow() && canSimplifyInvokeNoUnwind(&F)) {
         if (II->use_empty() && II->onlyReadsMemory()) {
@@ -1511,9 +1495,9 @@ static bool markAliveBlocks(Function &F,
     }
 
     Changed |= ConstantFoldTerminator(BB, true);
-    for (succ_iterator SI = succ_begin(BB), SE = succ_end(BB); SI != SE; ++SI)
-      if (Reachable.insert(*SI).second)
-        Worklist.push_back(*SI);
+    for (BasicBlock *Succ : successors(BB))
+      if (Reachable.insert(Succ).second)
+        Worklist.push_back(Succ);
   } while (!Worklist.empty());
   return Changed;
 }
diff --git a/lib/Transforms/Utils/LoopSimplify.cpp b/lib/Transforms/Utils/LoopSimplify.cpp
index 2509b0a058f..a2dd581f45c 100644
--- a/lib/Transforms/Utils/LoopSimplify.cpp
+++ b/lib/Transforms/Utils/LoopSimplify.cpp
@@ -491,7 +491,7 @@ ReprocessLoop:
 
       // Zap the dead pred's terminator and replace it with unreachable.
       TerminatorInst *TI = P->getTerminator();
-      changeToUnreachable(TI, /*UseLLVMTrap=*/false);
+      changeToUnreachable(TI);
       Changed = true;
     }
   }
diff --git a/test/Transforms/SimplifyCFG/invoke.ll b/test/Transforms/SimplifyCFG/invoke.ll
index b7fd7d62ccf..506fef0ef92 100644
--- a/test/Transforms/SimplifyCFG/invoke.ll
+++ b/test/Transforms/SimplifyCFG/invoke.ll
@@ -12,7 +12,6 @@ declare i32 @fn()
 ; CHECK-LABEL: @f1(
 define i8* @f1() nounwind uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
-; CHECK: call void @llvm.trap()
 ; CHECK: unreachable
   %call = invoke noalias i8* undef()
           to label %invoke.cont unwind label %lpad
@@ -31,7 +30,6 @@ lpad:
 ; CHECK-LABEL: @f2(
 define i8* @f2() nounwind uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
-; CHECK: call void @llvm.trap()
 ; CHECK: unreachable
   %call = invoke noalias i8* null()
           to label %invoke.cont unwind label %lpad
diff --git a/test/Transforms/SimplifyCFG/trap-debugloc.ll b/test/Transforms/SimplifyCFG/trap-debugloc.ll
deleted file mode 100644
index a912dc561a4..00000000000
--- a/test/Transforms/SimplifyCFG/trap-debugloc.ll
+++ /dev/null
@@ -1,22 +0,0 @@
-; RUN: opt -S -simplifycfg < %s | FileCheck %s
-; Radar 9342286
-; Assign DebugLoc to trap instruction.
-define void @foo() nounwind ssp !dbg !0 {
-; CHECK: call void @llvm.trap(), !dbg
-  store i32 42, i32* null, !dbg !5
-  ret void, !dbg !7
-}
-
-!llvm.dbg.cu = !{!2}
-!llvm.module.flags = !{!10}
-
-!0 = distinct !DISubprogram(name: "foo", line: 3, isLocal: false, isDefinition: true, virtualIndex: 6, isOptimized: false, unit: !2, file: !8, scope: !1, type: !3)
-!1 = !DIFile(filename: "foo.c", directory: "/private/tmp")
-!2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "Apple clang version 3.0 (tags/Apple/clang-206.1) (based on LLVM 3.0svn)", isOptimized: true, emissionKind: FullDebug, file: !8, enums: !{}, retainedTypes: !{})
-!3 = !DISubroutineType(types: !4)
-!4 = !{null}
-!5 = !DILocation(line: 4, column: 2, scope: !6)
-!6 = distinct !DILexicalBlock(line: 3, column: 12, file: !8, scope: !0)
-!7 = !DILocation(line: 5, column: 1, scope: !6)
-!8 = !DIFile(filename: "foo.c", directory: "/private/tmp")
-!10 = !{i32 1, !"Debug Info Version", i32 3}
diff --git a/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll b/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
index 5881367d961..818c454398b 100644
--- a/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
+++ b/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
@@ -28,7 +28,6 @@ entry:
         ret void
         
 ; CHECK-LABEL: @test2(
-; CHECK: call void @llvm.trap
 ; CHECK: unreachable
 }