[llvm] r273778 - [SimplifyCFG] Stop inserting calls to llvm.trap for UB
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Sun Jun 26 18:06:36 PDT 2016
Thanks!
I was recently playing around with a test case where this insertion of
llvm.trap would inhibit optimization for some pass configurations.
For reference, it was a binary search routine that returned null in the
"not found" case; but the result was unconditionally dereferenced so after
inlining we could remove the `lo <= hi` check every time through the loop
since that leads to "return NULL". Interestingly, this causes the actual
behavior when the UB occurs to be an infloop instead of a segfault.
That's what happened in the original customer test case, but while reducing
this I noticed that simplifycfg could ruin the optimization if run at the
wrong time.
-- Sean Silva
On Sat, Jun 25, 2016 at 1:04 AM, David Majnemer via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: majnemer
> Date: Sat Jun 25 03:04:19 2016
> New Revision: 273778
>
> URL: http://llvm.org/viewvc/llvm-project?rev=273778&view=rev
> Log:
> [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...
>
> Removed:
> llvm/trunk/test/Transforms/SimplifyCFG/trap-debugloc.ll
> Modified:
> llvm/trunk/include/llvm/Transforms/Utils/Local.h
> llvm/trunk/lib/CodeGen/WinEHPrepare.cpp
> llvm/trunk/lib/Transforms/IPO/PruneEH.cpp
> llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp
> llvm/trunk/lib/Transforms/Utils/Local.cpp
> llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp
> llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
> llvm/trunk/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
>
> Modified: llvm/trunk/include/llvm/Transforms/Utils/Local.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/Local.h?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Transforms/Utils/Local.h (original)
> +++ llvm/trunk/include/llvm/Transforms/Utils/Local.h Sat Jun 25 03:04:19
> 2016
> @@ -296,7 +296,7 @@ unsigned removeAllNonTerminatorAndEHPadI
>
> /// 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
>
> Modified: llvm/trunk/lib/CodeGen/WinEHPrepare.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/WinEHPrepare.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/WinEHPrepare.cpp (original)
> +++ llvm/trunk/lib/CodeGen/WinEHPrepare.cpp Sat Jun 25 03:04:19 2016
> @@ -963,9 +963,9 @@ void WinEHPrepare::removeImplausibleInst
> 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::removeImplausibleInst
> 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
>
> Modified: llvm/trunk/lib/Transforms/IPO/PruneEH.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/PruneEH.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/IPO/PruneEH.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/PruneEH.cpp Sat Jun 25 03:04:19 2016
> @@ -258,7 +258,7 @@ void PruneEH::DeleteBasicBlock(BasicBloc
>
> 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));
>
> Modified: llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Sat Jun 25 03:04:19 2016
> @@ -1802,8 +1802,7 @@ static bool runIPSCCP(Module &M, const D
> DEBUG(dbgs() << " BasicBlock Dead:" << *BB);
>
> ++NumDeadBlocks;
> - NumInstRemoved +=
> - changeToUnreachable(BB->getFirstNonPHI(),
> /*UseLLVMTrap=*/false);
> + NumInstRemoved += changeToUnreachable(BB->getFirstNonPHI());
>
> MadeChanges = true;
>
>
> Modified: llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp Sat Jun 25 03:04:19
> 2016
> @@ -1833,7 +1833,7 @@ bool llvm::InlineFunction(CallSite CS, I
> // 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())
>
> Modified: llvm/trunk/lib/Transforms/Utils/Local.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/Local.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/Local.cpp Sat Jun 25 03:04:19 2016
> @@ -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::removeAllNonTerminatorAnd
> 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);
> + for (BasicBlock *Succ : successors(BB))
> + Succ->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());
> - }
> 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;
> }
>
> Modified: llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/LoopSimplify.cpp Sat Jun 25 03:04:19
> 2016
> @@ -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;
> }
> }
>
> Modified: llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll (original)
> +++ llvm/trunk/test/Transforms/SimplifyCFG/invoke.ll Sat Jun 25 03:04:19
> 2016
> @@ -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
>
> Removed: llvm/trunk/test/Transforms/SimplifyCFG/trap-debugloc.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/trap-debugloc.ll?rev=273777&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/SimplifyCFG/trap-debugloc.ll (original)
> +++ llvm/trunk/test/Transforms/SimplifyCFG/trap-debugloc.ll (removed)
> @@ -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}
>
> Modified:
> llvm/trunk/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll?rev=273778&r1=273777&r2=273778&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
> (original)
> +++ llvm/trunk/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
> Sat Jun 25 03:04:19 2016
> @@ -28,7 +28,6 @@ entry:
> ret void
>
> ; CHECK-LABEL: @test2(
> -; CHECK: call void @llvm.trap
> ; CHECK: unreachable
> }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160626/fcd84071/attachment.html>
More information about the llvm-commits
mailing list