[PATCH] Disable tail merge of call sites when building with any sanitizer
David Blaikie
dblaikie at gmail.com
Mon Nov 18 13:29:22 PST 2013
Same as:
Do we have precedence for this kind of change (where sanitizers affect
optimizations in arbitrary internal ways - not simply by
enabling/disabling
certain passes)? If not, does this need some deeper discussion about
alternatives (is it important that we be able to produce equivalent code
without the sanitizers enabled?)?
http://llvm-reviews.chandlerc.com/D2214
On Mon, Nov 18, 2013 at 7:31 AM, Evgeniy Stepanov <eugenis at google.com>wrote:
> 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
> +}
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131118/a99315f8/attachment.html>
More information about the llvm-commits
mailing list