[SanitizerCoverage] Avoid splitting critical edges when destination is a basic block...

This patch adds a new option to SplitAllCriticalEdges and uses it to avoid splitting critical edges when the destination basic block ends with unreachable. Otherwise if we split the critical edge, sanitizer coverage will instrument the new block that gets inserted for the split. But since this block itself shouldn't be reachable this is pointless. These basic blocks will stick around and generate assembly, but they don't end in sane control flow and might get placed at the end of the function. This makes it look like one function has code that flows into the next function.

This showed up while compiling the linux kernel with clang. The kernel has a tool called objtool that detected the code that appeared to flow from one function to the next. https://github.com/ClangBuiltLinux/linux/issues/351#issuecomment-461698884

Differential Revision: https://reviews.llvm.org/D57982

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

diff --git a/include/llvm/Transforms/Utils/BasicBlockUtils.h b/include/llvm/Transforms/Utils/BasicBlockUtils.h
index 6ee649daa3f8a091cc41c56e06d926c55514c575..4d861ffe9a312765cee7a1d4c78a6ae6115ffea0 100644 (file)
--- a/include/llvm/Transforms/Utils/BasicBlockUtils.h
+++ b/include/llvm/Transforms/Utils/BasicBlockUtils.h
@@ -116,6 +116,7 @@ struct CriticalEdgeSplittingOptions {
   bool MergeIdenticalEdges = false;
   bool KeepOneInputPHIs = false;
   bool PreserveLCSSA = false;
+  bool IgnoreUnreachableDests = false;
 
   CriticalEdgeSplittingOptions(DominatorTree *DT = nullptr,
                                LoopInfo *LI = nullptr,
@@ -137,6 +138,11 @@ struct CriticalEdgeSplittingOptions {
     PreserveLCSSA = true;
     return *this;
   }
+
+  CriticalEdgeSplittingOptions &setIgnoreUnreachableDests() {
+    IgnoreUnreachableDests = true;
+    return *this;
+  }
 };
 
 /// If this edge is a critical edge, insert a new node to split the critical

diff --git a/lib/Transforms/Instrumentation/SanitizerCoverage.cpp b/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
index 40151bc5d784fa8a5c923dfbe16832b7948d955e..01e89d3c253c5a8c8b6376a096d99aad5c6f8f1b 100644 (file)
--- a/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
+++ b/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
@@ -535,7 +535,7 @@ bool SanitizerCoverageModule::runOnFunction(Function &F) {
       isAsynchronousEHPersonality(classifyEHPersonality(F.getPersonalityFn())))
     return false;
   if (Options.CoverageType >= SanitizerCoverageOptions::SCK_Edge)
-    SplitAllCriticalEdges(F);
+    SplitAllCriticalEdges(F, CriticalEdgeSplittingOptions().setIgnoreUnreachableDests());
   SmallVector<Instruction *, 8> IndirCalls;
   SmallVector<BasicBlock *, 16> BlocksToInstrument;
   SmallVector<Instruction *, 8> CmpTraceTargets;

diff --git a/lib/Transforms/Utils/BreakCriticalEdges.cpp b/lib/Transforms/Utils/BreakCriticalEdges.cpp
index 3b4b0b5de07302fe64ffdcad0d2235ea11a20c82..f5e4b53f6d97d1a76bf8fb41b4f0f3f168febff2 100644 (file)
--- a/lib/Transforms/Utils/BreakCriticalEdges.cpp
+++ b/lib/Transforms/Utils/BreakCriticalEdges.cpp
@@ -153,6 +153,10 @@ llvm::SplitCriticalEdge(Instruction *TI, unsigned SuccNum,
   if (isa<CallBrInst>(TI) && SuccNum > 0)
     return nullptr;
 
+  if (Options.IgnoreUnreachableDests &&
+      isa<UnreachableInst>(DestBB->getFirstNonPHIOrDbgOrLifetime()))
+    return nullptr;
+
   // Create a new basic block, linking it into the CFG.
   BasicBlock *NewBB = BasicBlock::Create(TI->getContext(),
                       TIBB->getName() + "." + DestBB->getName() + "_crit_edge");

diff --git a/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll b/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll
new file mode 100644 (file)
index 0000000..ad6cd57
--- /dev/null
+++ b/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll
@@ -0,0 +1,46 @@
+; RUN: opt < %s -S -sancov -sanitizer-coverage-level=3 | FileCheck %s
+
+; The critical edges to unreachable_bb should not be split.
+define i32 @foo(i32 %c, i32 %d) {
+; CHECK-LABEL: @foo(
+; CHECK:         switch i32 [[C:%.*]], label [[UNREACHABLE_BB:%.*]] [
+; CHECK-NEXT:    i32 0, label %exit0
+; CHECK-NEXT:    i32 1, label %exit1
+; CHECK-NEXT:    i32 2, label %cont
+; CHECK-NEXT:    ]
+; CHECK:       cont:
+; CHECK:         switch i32 [[D:%.*]], label [[UNREACHABLE_BB]] [
+; CHECK-NEXT:    i32 0, label %exit2
+; CHECK-NEXT:    i32 1, label %exit3
+; CHECK-NEXT:    i32 2, label %exit4
+; CHECK-NEXT:    ]
+; CHECK:       unreachable_bb:
+; CHECK-NEXT:    unreachable
+;
+  switch i32 %c, label %unreachable_bb [i32 0, label %exit0
+  i32 1, label %exit1
+  i32 2, label %cont]
+
+cont:
+  switch i32 %d, label %unreachable_bb [i32 0, label %exit2
+  i32 1, label %exit3
+  i32 2, label %exit4]
+
+exit0:
+  ret i32 0
+
+exit1:
+  ret i32 1
+
+exit2:
+  ret i32 2
+
+exit3:
+  ret i32 3
+
+exit4:
+  ret i32 4
+
+unreachable_bb:
+  unreachable
+}