From 167ede898a6105e05fcd9d2ae5679fbf1744018f Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Thu, 17 Jan 2013 21:28:46 +0000 Subject: [PATCH] Reverting r171325 & r172363. This was causing a mis-compile on the self-hosted LTO build bots. Okay, here's how to reproduce the problem: 1) Build a Release (or Release+Asserts) version of clang in the normal way. 2) Using the clang & clang++ binaries from (1), build a Release (or Release+Asserts) version of the same sources, but this time enable LTO --- specify the `-flto' flag on the command line. 3) Run the ARC migrator tests: $ arcmt-test --args -triple x86_64-apple-darwin10 -fsyntax-only -x objective-c++ ./src/tools/clang/test/ARCMT/cxx-rewrite.mm You'll see that the output isn't correct (the whitespace is off). The mis-compile is in the function `RewriteBuffer::RemoveText' in the clang/lib/Rewrite/Core/Rewriter.cpp file. When that function and RewriteRope.cpp are compiled with LTO and the `arcmt-test' executable is regenerated, you'll see the error. When those files are not LTO'ed, then the output of the `arcmt-test' is fine. It is *really* hard to get a testcase out of this. I'll file a PR with what I have currently. --- Reverse-merging r172363 into '.': U include/llvm/Analysis/MemoryBuiltins.h U lib/Analysis/MemoryBuiltins.cpp --- Reverse-merging r171325 into '.': U test/Transforms/InstCombine/objsize.ll G include/llvm/Analysis/MemoryBuiltins.h G lib/Analysis/MemoryBuiltins.cpp git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@172756 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Analysis/MemoryBuiltins.h | 4 +- lib/Analysis/MemoryBuiltins.cpp | 41 +++----- test/Transforms/InstCombine/objsize.ll | 128 ------------------------- 3 files changed, 13 insertions(+), 160 deletions(-) diff --git a/include/llvm/Analysis/MemoryBuiltins.h b/include/llvm/Analysis/MemoryBuiltins.h index 2a9cd75ee78..f07658fbc6d 100644 --- a/include/llvm/Analysis/MemoryBuiltins.h +++ b/include/llvm/Analysis/MemoryBuiltins.h @@ -153,14 +153,12 @@ typedef std::pair SizeOffsetType; class ObjectSizeOffsetVisitor : public InstVisitor { - typedef DenseMap CacheMapTy; - const DataLayout *TD; const TargetLibraryInfo *TLI; bool RoundToAlign; unsigned IntTyBits; APInt Zero; - CacheMapTy CacheMap; + SmallPtrSet SeenInsts; APInt align(APInt Size, uint64_t Align); diff --git a/lib/Analysis/MemoryBuiltins.cpp b/lib/Analysis/MemoryBuiltins.cpp index 1d27a83d935..0fc05505ddb 100644 --- a/lib/Analysis/MemoryBuiltins.cpp +++ b/lib/Analysis/MemoryBuiltins.cpp @@ -385,23 +385,16 @@ ObjectSizeOffsetVisitor::ObjectSizeOffsetVisitor(const DataLayout *TD, SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) { V = V->stripPointerCasts(); + if (Instruction *I = dyn_cast(V)) { + // If we have already seen this instruction, bail out. Cycles can happen in + // unreachable code after constant propagation. + if (!SeenInsts.insert(I)) + return unknown(); - if (isa(V) || isa(V)) { - // return cached value or insert unknown in cache if size of V was not - // computed yet in order to avoid recursions in PHis - std::pair CacheVal = - CacheMap.insert(std::make_pair(V, unknown())); - if (!CacheVal.second) - return CacheVal.first->second; - - SizeOffsetType Result; if (GEPOperator *GEP = dyn_cast(V)) - Result = visitGEPOperator(*GEP); - else - Result = visit(cast(*V)); - return CacheMap[V] = Result; + return visitGEPOperator(*GEP); + return visit(*I); } - if (Argument *A = dyn_cast(V)) return visitArgument(*A); if (ConstantPointerNull *P = dyn_cast(V)) @@ -415,6 +408,8 @@ SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) { if (ConstantExpr *CE = dyn_cast(V)) { if (CE->getOpcode() == Instruction::IntToPtr) return unknown(); // clueless + if (CE->getOpcode() == Instruction::GetElementPtr) + return visitGEPOperator(cast(*CE)); } DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: " << *V @@ -548,21 +543,9 @@ SizeOffsetType ObjectSizeOffsetVisitor::visitLoadInst(LoadInst&) { return unknown(); } -SizeOffsetType ObjectSizeOffsetVisitor::visitPHINode(PHINode &PHI) { - if (PHI.getNumIncomingValues() == 0) - return unknown(); - - SizeOffsetType Ret = compute(PHI.getIncomingValue(0)); - if (!bothKnown(Ret)) - return unknown(); - - // verify that all PHI incoming pointers have the same size and offset - for (unsigned i = 1, e = PHI.getNumIncomingValues(); i != e; ++i) { - SizeOffsetType EdgeData = compute(PHI.getIncomingValue(i)); - if (!bothKnown(EdgeData) || EdgeData != Ret) - return unknown(); - } - return Ret; +SizeOffsetType ObjectSizeOffsetVisitor::visitPHINode(PHINode&) { + // too complex to analyze statically. + return unknown(); } SizeOffsetType ObjectSizeOffsetVisitor::visitSelectInst(SelectInst &I) { diff --git a/test/Transforms/InstCombine/objsize.ll b/test/Transforms/InstCombine/objsize.ll index 0ead9d12374..31a3cb46e45 100644 --- a/test/Transforms/InstCombine/objsize.ll +++ b/test/Transforms/InstCombine/objsize.ll @@ -256,131 +256,3 @@ xpto: return: ret i32 7 } - -declare noalias i8* @valloc(i32) nounwind - -; CHECK: @test14 -; CHECK: ret i32 6 -define i32 @test14(i32 %a) nounwind { - switch i32 %a, label %sw.default [ - i32 1, label %sw.bb - i32 2, label %sw.bb1 - ] - -sw.bb: - %call = tail call noalias i8* @malloc(i32 6) nounwind - br label %sw.epilog - -sw.bb1: - %call2 = tail call noalias i8* @calloc(i32 3, i32 2) nounwind - br label %sw.epilog - -sw.default: - %call3 = tail call noalias i8* @valloc(i32 6) nounwind - br label %sw.epilog - -sw.epilog: - %b.0 = phi i8* [ %call3, %sw.default ], [ %call2, %sw.bb1 ], [ %call, %sw.bb ] - %1 = tail call i32 @llvm.objectsize.i32(i8* %b.0, i1 false) - ret i32 %1 -} - -; CHECK: @test15 -; CHECK: llvm.objectsize -define i32 @test15(i32 %a) nounwind { - switch i32 %a, label %sw.default [ - i32 1, label %sw.bb - i32 2, label %sw.bb1 - ] - -sw.bb: - %call = tail call noalias i8* @malloc(i32 3) nounwind - br label %sw.epilog - -sw.bb1: - %call2 = tail call noalias i8* @calloc(i32 2, i32 1) nounwind - br label %sw.epilog - -sw.default: - %call3 = tail call noalias i8* @valloc(i32 3) nounwind - br label %sw.epilog - -sw.epilog: - %b.0 = phi i8* [ %call3, %sw.default ], [ %call2, %sw.bb1 ], [ %call, %sw.bb ] - %1 = tail call i32 @llvm.objectsize.i32(i8* %b.0, i1 false) - ret i32 %1 -} - -; CHECK: @test16 -; CHECK: llvm.objectsize -define i32 @test16(i8* %a, i32 %n) nounwind { - %b = alloca [5 x i8], align 1 - %c = alloca [5 x i8], align 1 - switch i32 %n, label %sw.default [ - i32 1, label %sw.bb - i32 2, label %sw.bb1 - ] - -sw.bb: - %bp = bitcast [5 x i8]* %b to i8* - br label %sw.epilog - -sw.bb1: - %cp = bitcast [5 x i8]* %c to i8* - br label %sw.epilog - -sw.default: - br label %sw.epilog - -sw.epilog: - %phi = phi i8* [ %a, %sw.default ], [ %cp, %sw.bb1 ], [ %bp, %sw.bb ] - %sz = call i32 @llvm.objectsize.i32(i8* %phi, i1 false) - ret i32 %sz -} - -; CHECK: @test17 -; CHECK: ret i32 5 -define i32 @test17(i32 %n) nounwind { - %b = alloca [5 x i8], align 1 - %c = alloca [5 x i8], align 1 - %bp = bitcast [5 x i8]* %b to i8* - switch i32 %n, label %sw.default [ - i32 1, label %sw.bb - i32 2, label %sw.bb1 - ] - -sw.bb: - br label %sw.epilog - -sw.bb1: - %cp = bitcast [5 x i8]* %c to i8* - br label %sw.epilog - -sw.default: - br label %sw.epilog - -sw.epilog: - %phi = phi i8* [ %bp, %sw.default ], [ %cp, %sw.bb1 ], [ %bp, %sw.bb ] - %sz = call i32 @llvm.objectsize.i32(i8* %phi, i1 false) - ret i32 %sz -} - -@globalalias = alias internal [60 x i8]* @a - -; CHECK: @test18 -; CHECK-NEXT: ret i32 60 -define i32 @test18() { - %bc = bitcast [60 x i8]* @globalalias to i8* - %1 = call i32 @llvm.objectsize.i32(i8* %bc, i1 false) - ret i32 %1 -} - -@globalalias2 = alias weak [60 x i8]* @a - -; CHECK: @test19 -; CHECK: llvm.objectsize -define i32 @test19() { - %bc = bitcast [60 x i8]* @globalalias2 to i8* - %1 = call i32 @llvm.objectsize.i32(i8* %bc, i1 false) - ret i32 %1 -}