[llvm] r355947 - [SanitizerCoverage] Avoid splitting critical edges when destination is a basic block containing unreachable

[Craig Topper via llvm-commits]

Author: ctopper
Date: Tue Mar 12 11:20:25 2019
New Revision: 355947

URL: http://llvm.org/viewvc/llvm-project?rev=355947&view=rev
Log:
[SanitizerCoverage] Avoid splitting critical edges when destination is a basic block containing unreachable

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

Added:
    llvm/trunk/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll
Modified:
    llvm/trunk/include/llvm/Transforms/Utils/BasicBlockUtils.h
    llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
    llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp

Modified: llvm/trunk/include/llvm/Transforms/Utils/BasicBlockUtils.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/BasicBlockUtils.h?rev=355947&r1=355946&r2=355947&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Utils/BasicBlockUtils.h (original)
+++ llvm/trunk/include/llvm/Transforms/Utils/BasicBlockUtils.h Tue Mar 12 11:20:25 2019
@@ -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

Modified: llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp?rev=355947&r1=355946&r2=355947&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp Tue Mar 12 11:20:25 2019
@@ -535,7 +535,7 @@ bool SanitizerCoverageModule::runOnFunct
       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;

Modified: llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp?rev=355947&r1=355946&r2=355947&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp (original)
+++ llvm/trunk/lib/Transforms/Utils/BreakCriticalEdges.cpp Tue Mar 12 11:20:25 2019
@@ -153,6 +153,10 @@ llvm::SplitCriticalEdge(Instruction *TI,
   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");

Added: llvm/trunk/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll?rev=355947&view=auto
==============================================================================
--- llvm/trunk/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll (added)
+++ llvm/trunk/test/Instrumentation/SanitizerCoverage/unreachable-critedge.ll Tue Mar 12 11:20:25 2019
@@ -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
+}