<div dir="ltr">Same as:<br><br><div class="im" style="font-family:monospace"> Do we have precedence for this kind of change (where sanitizers affect<br> optimizations in arbitrary internal ways - not simply by enabling/disabling<br>
certain passes)? If not, does this need some deeper discussion about<br> alternatives (is it important that we be able to produce equivalent code<br> without the sanitizers enabled?)?<br><br></div><a href="http://llvm-reviews.chandlerc.com/D2214" target="_blank" style="font-family:monospace">http://llvm-reviews.chandlerc.com/D2214</a><br>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Nov 18, 2013 at 7:31 AM, Evgeniy Stepanov <span dir="ltr"><<a href="mailto:eugenis@google.com" target="_blank">eugenis@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2215" target="_blank">http://llvm-reviews.chandlerc.com/D2215</a><br>
<br>
Files:<br>
lib/CodeGen/BranchFolding.h<br>
lib/CodeGen/BranchFolding.cpp<br>
test/CodeGen/X86/tail-merge-msan.ll<br>
<br>
Index: lib/CodeGen/BranchFolding.h<br>
===================================================================<br>
--- lib/CodeGen/BranchFolding.h<br>
+++ lib/CodeGen/BranchFolding.h<br>
@@ -86,6 +86,7 @@<br>
std::vector<SameTailElt> SameTails;<br>
<br>
bool EnableTailMerge;<br>
+ bool EnableTailMergeCalls;<br>
bool EnableHoistCommonCode;<br>
const TargetInstrInfo *TII;<br>
const TargetRegisterInfo *TRI;<br>
Index: lib/CodeGen/BranchFolding.cpp<br>
===================================================================<br>
--- lib/CodeGen/BranchFolding.cpp<br>
+++ lib/CodeGen/BranchFolding.cpp<br>
@@ -90,7 +90,6 @@<br>
getAnalysisIfAvailable<MachineModuleInfo>());<br>
}<br>
<br>
-<br>
BranchFolder::BranchFolder(bool defaultEnableTailMerge, bool CommonHoist) {<br>
switch (FlagEnableTailMerge) {<br>
case cl::BOU_UNSET: EnableTailMerge = defaultEnableTailMerge; break;<br>
@@ -169,7 +168,7 @@<br>
return true;<br>
}<br>
<br>
-/// OptimizeFunction - Perhaps branch folding, tail merging and other<br>
+/// OptimizeFunction - Perform branch folding, tail merging and other<br>
/// CFG optimizations on the given function.<br>
bool BranchFolder::OptimizeFunction(MachineFunction &MF,<br>
const TargetInstrInfo *tii,<br>
@@ -184,6 +183,13 @@<br>
MMI = mmi;<br>
RS = NULL;<br>
<br>
+ // Tail merging code that contains function calls breaks symbolization of<br>
+ // stack traces.<br>
+ EnableTailMergeCalls =<br>
+ !MF.getFunction()->hasFnAttribute(Attribute::SanitizeAddress) &&<br>
+ !MF.getFunction()->hasFnAttribute(Attribute::SanitizeMemory) &&<br>
+ !MF.getFunction()->hasFnAttribute(Attribute::SanitizeThread);<br>
+<br>
// Use a RegScavenger to help update liveness when required.<br>
MachineRegisterInfo &MRI = MF.getRegInfo();<br>
if (MRI.tracksLiveness() && TRI->trackLivenessAfterRegAlloc(MF))<br>
@@ -305,7 +311,8 @@<br>
static unsigned ComputeCommonTailLength(MachineBasicBlock *MBB1,<br>
MachineBasicBlock *MBB2,<br>
MachineBasicBlock::iterator &I1,<br>
- MachineBasicBlock::iterator &I2) {<br>
+ MachineBasicBlock::iterator &I2,<br>
+ bool EnableTailMergeCalls) {<br>
I1 = MBB1->end();<br>
I2 = MBB2->end();<br>
<br>
@@ -337,7 +344,7 @@<br>
--I2;<br>
}<br>
// I1, I2==first (untested) non-DBGs preceding known match<br>
- if (!I1->isIdenticalTo(I2) ||<br>
+ if (!I1->isIdenticalTo(I2) || (!EnableTailMergeCalls && I1->isCall()) ||<br>
// FIXME: This check is dubious. It's used to get around a problem where<br>
// people incorrectly expect inline asm directives to remain in the same<br>
// relative order. This is untenable because normal compiler<br>
@@ -519,15 +526,16 @@<br>
/// and decide if it would be profitable to merge those tails. Return the<br>
/// length of the common tail and iterators to the first common instruction<br>
/// in each block.<br>
-static bool ProfitableToMerge(MachineBasicBlock *MBB1,<br>
- MachineBasicBlock *MBB2,<br>
+static bool ProfitableToMerge(MachineBasicBlock *MBB1, MachineBasicBlock *MBB2,<br>
unsigned minCommonTailLength,<br>
unsigned &CommonTailLen,<br>
MachineBasicBlock::iterator &I1,<br>
MachineBasicBlock::iterator &I2,<br>
MachineBasicBlock *SuccBB,<br>
- MachineBasicBlock *PredBB) {<br>
- CommonTailLen = ComputeCommonTailLength(MBB1, MBB2, I1, I2);<br>
+ MachineBasicBlock *PredBB,<br>
+ bool EnableTailMergeCalls) {<br>
+ CommonTailLen =<br>
+ ComputeCommonTailLength(MBB1, MBB2, I1, I2, EnableTailMergeCalls);<br>
if (CommonTailLen == 0)<br>
return false;<br>
DEBUG(dbgs() << "Common tail length of BB#" << MBB1->getNumber()<br>
@@ -604,9 +612,8 @@<br>
for (MPIterator I = prior(CurMPIter); I->getHash() == CurHash ; --I) {<br>
unsigned CommonTailLen;<br>
if (ProfitableToMerge(CurMPIter->getBlock(), I->getBlock(),<br>
- minCommonTailLength,<br>
- CommonTailLen, TrialBBI1, TrialBBI2,<br>
- SuccBB, PredBB)) {<br>
+ minCommonTailLength, CommonTailLen, TrialBBI1,<br>
+ TrialBBI2, SuccBB, PredBB, EnableTailMergeCalls)) {<br>
if (CommonTailLen > maxCommonTailLength) {<br>
SameTails.clear();<br>
maxCommonTailLength = CommonTailLen;<br>
Index: test/CodeGen/X86/tail-merge-msan.ll<br>
===================================================================<br>
--- test/CodeGen/X86/tail-merge-msan.ll<br>
+++ test/CodeGen/X86/tail-merge-msan.ll<br>
@@ -0,0 +1,80 @@<br>
+; RUN: llc < %s -march=x86-64 -mtriple=x86_64-unknown-linux-gnu -asm-verbose=false | FileCheck %s<br>
+<br>
+declare void @callee()<br>
+<br>
+; Test that call sites are not tail merged with sanitize_address<br>
+<br>
+define i32 @caller1(i32 %x) sanitize_address {<br>
+; CHECK: caller1:<br>
+; CHECK: callq callee<br>
+; CHECK: callq callee<br>
+; CHECK: ret<br>
+entry:<br>
+ switch i32 %x, label %if.end3 [<br>
+ i32 1, label %if.then<br>
+ i32 2, label %if.then2<br>
+ ]<br>
+<br>
+if.then: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.then2: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.end3: ; preds = %entry, %if.then2, %if.then<br>
+ ret i32 0<br>
+}<br>
+<br>
+<br>
+; Test that call sites are not tail merged with sanitize_memory<br>
+<br>
+define i32 @caller2(i32 %x) sanitize_memory {<br>
+; CHECK: caller2:<br>
+; CHECK: callq callee<br>
+; CHECK: callq callee<br>
+; CHECK: ret<br>
+entry:<br>
+ switch i32 %x, label %if.end3 [<br>
+ i32 1, label %if.then<br>
+ i32 2, label %if.then2<br>
+ ]<br>
+<br>
+if.then: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.then2: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.end3: ; preds = %entry, %if.then2, %if.then<br>
+ ret i32 0<br>
+}<br>
+<br>
+<br>
+; Test that call sites are not tail merged with sanitize_thread<br>
+<br>
+define i32 @caller3(i32 %x) sanitize_thread {<br>
+; CHECK: caller3:<br>
+; CHECK: callq callee<br>
+; CHECK: callq callee<br>
+; CHECK: ret<br>
+entry:<br>
+ switch i32 %x, label %if.end3 [<br>
+ i32 1, label %if.then<br>
+ i32 2, label %if.then2<br>
+ ]<br>
+<br>
+if.then: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.then2: ; preds = %entry<br>
+ tail call void @callee()<br>
+ br label %if.end3<br>
+<br>
+if.end3: ; preds = %entry, %if.then2, %if.then<br>
+ ret i32 0<br>
+}<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>