[PATCH] Disable tail merge of call sites when building with any sanitizer

[Evgeniy Stepanov]

Merging call sites leads to very poor and misleading stack traces. They have wrong line numbers for some of the frames, but in a very non-obvious way, which makes reasoning about such reports very difficult.


http://llvm-reviews.chandlerc.com/D2215

Files:
  lib/CodeGen/BranchFolding.h
  lib/CodeGen/BranchFolding.cpp
  test/CodeGen/X86/tail-merge-msan.ll

Index: lib/CodeGen/BranchFolding.h
===================================================================
--- lib/CodeGen/BranchFolding.h
+++ lib/CodeGen/BranchFolding.h
@@ -86,6 +86,7 @@
     std::vector<SameTailElt> SameTails;
 
     bool EnableTailMerge;
+    bool EnableTailMergeCalls;
     bool EnableHoistCommonCode;
     const TargetInstrInfo *TII;
     const TargetRegisterInfo *TRI;
Index: lib/CodeGen/BranchFolding.cpp
===================================================================
--- lib/CodeGen/BranchFolding.cpp
+++ lib/CodeGen/BranchFolding.cpp
@@ -90,7 +90,6 @@
                                  getAnalysisIfAvailable<MachineModuleInfo>());
 }
 
-
 BranchFolder::BranchFolder(bool defaultEnableTailMerge, bool CommonHoist) {
   switch (FlagEnableTailMerge) {
   case cl::BOU_UNSET: EnableTailMerge = defaultEnableTailMerge; break;
@@ -169,7 +168,7 @@
   return true;
 }
 
-/// OptimizeFunction - Perhaps branch folding, tail merging and other
+/// OptimizeFunction - Perform branch folding, tail merging and other
 /// CFG optimizations on the given function.
 bool BranchFolder::OptimizeFunction(MachineFunction &MF,
                                     const TargetInstrInfo *tii,
@@ -184,6 +183,13 @@
   MMI = mmi;
   RS = NULL;
 
+  // Tail merging code that contains function calls breaks symbolization of
+  // stack traces.
+  EnableTailMergeCalls =
+      !MF.getFunction()->hasFnAttribute(Attribute::SanitizeAddress) &&
+      !MF.getFunction()->hasFnAttribute(Attribute::SanitizeMemory) &&
+      !MF.getFunction()->hasFnAttribute(Attribute::SanitizeThread);
+
   // Use a RegScavenger to help update liveness when required.
   MachineRegisterInfo &MRI = MF.getRegInfo();
   if (MRI.tracksLiveness() && TRI->trackLivenessAfterRegAlloc(MF))
@@ -305,7 +311,8 @@
 static unsigned ComputeCommonTailLength(MachineBasicBlock *MBB1,
                                         MachineBasicBlock *MBB2,
                                         MachineBasicBlock::iterator &I1,
-                                        MachineBasicBlock::iterator &I2) {
+                                        MachineBasicBlock::iterator &I2,
+                                        bool EnableTailMergeCalls) {
   I1 = MBB1->end();
   I2 = MBB2->end();
 
@@ -337,7 +344,7 @@
       --I2;
     }
     // I1, I2==first (untested) non-DBGs preceding known match
-    if (!I1->isIdenticalTo(I2) ||
+    if (!I1->isIdenticalTo(I2) || (!EnableTailMergeCalls && I1->isCall()) ||
         // FIXME: This check is dubious. It's used to get around a problem where
         // people incorrectly expect inline asm directives to remain in the same
         // relative order. This is untenable because normal compiler
@@ -519,15 +526,16 @@
 /// and decide if it would be profitable to merge those tails.  Return the
 /// length of the common tail and iterators to the first common instruction
 /// in each block.
-static bool ProfitableToMerge(MachineBasicBlock *MBB1,
-                              MachineBasicBlock *MBB2,
+static bool ProfitableToMerge(MachineBasicBlock *MBB1, MachineBasicBlock *MBB2,
                               unsigned minCommonTailLength,
                               unsigned &CommonTailLen,
                               MachineBasicBlock::iterator &I1,
                               MachineBasicBlock::iterator &I2,
                               MachineBasicBlock *SuccBB,
-                              MachineBasicBlock *PredBB) {
-  CommonTailLen = ComputeCommonTailLength(MBB1, MBB2, I1, I2);
+                              MachineBasicBlock *PredBB,
+                              bool EnableTailMergeCalls) {
+  CommonTailLen =
+      ComputeCommonTailLength(MBB1, MBB2, I1, I2, EnableTailMergeCalls);
   if (CommonTailLen == 0)
     return false;
   DEBUG(dbgs() << "Common tail length of BB#" << MBB1->getNumber()
@@ -604,9 +612,8 @@
     for (MPIterator I = prior(CurMPIter); I->getHash() == CurHash ; --I) {
       unsigned CommonTailLen;
       if (ProfitableToMerge(CurMPIter->getBlock(), I->getBlock(),
-                            minCommonTailLength,
-                            CommonTailLen, TrialBBI1, TrialBBI2,
-                            SuccBB, PredBB)) {
+                            minCommonTailLength, CommonTailLen, TrialBBI1,
+                            TrialBBI2, SuccBB, PredBB, EnableTailMergeCalls)) {
         if (CommonTailLen > maxCommonTailLength) {
           SameTails.clear();
           maxCommonTailLength = CommonTailLen;
Index: test/CodeGen/X86/tail-merge-msan.ll
===================================================================
--- test/CodeGen/X86/tail-merge-msan.ll
+++ test/CodeGen/X86/tail-merge-msan.ll
@@ -0,0 +1,80 @@
+; RUN: llc < %s -march=x86-64 -mtriple=x86_64-unknown-linux-gnu -asm-verbose=false | FileCheck %s
+
+declare void @callee()
+
+; Test that call sites are not tail merged with sanitize_address
+
+define i32 @caller1(i32 %x) sanitize_address {
+; CHECK: caller1:
+; CHECK:   callq callee
+; CHECK:   callq callee
+; CHECK:   ret
+entry:
+  switch i32 %x, label %if.end3 [
+    i32 1, label %if.then
+    i32 2, label %if.then2
+  ]
+
+if.then:                                          ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.then2:                                         ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.end3:                                          ; preds = %entry, %if.then2, %if.then
+  ret i32 0
+}
+
+
+; Test that call sites are not tail merged with sanitize_memory
+
+define i32 @caller2(i32 %x) sanitize_memory {
+; CHECK: caller2:
+; CHECK:   callq callee
+; CHECK:   callq callee
+; CHECK:   ret
+entry:
+  switch i32 %x, label %if.end3 [
+    i32 1, label %if.then
+    i32 2, label %if.then2
+  ]
+
+if.then:                                          ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.then2:                                         ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.end3:                                          ; preds = %entry, %if.then2, %if.then
+  ret i32 0
+}
+
+
+; Test that call sites are not tail merged with sanitize_thread
+
+define i32 @caller3(i32 %x) sanitize_thread {
+; CHECK: caller3:
+; CHECK:   callq callee
+; CHECK:   callq callee
+; CHECK:   ret
+entry:
+  switch i32 %x, label %if.end3 [
+    i32 1, label %if.then
+    i32 2, label %if.then2
+  ]
+
+if.then:                                          ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.then2:                                         ; preds = %entry
+  tail call void @callee()
+  br label %if.end3
+
+if.end3:                                          ; preds = %entry, %if.then2, %if.then
+  ret i32 0
+}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2215.1.patch
Type: text/x-patch
Size: 6781 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131118/de35fafe/attachment.bin>