From 39e357fafe074babbd9661f3e78239f5ebe7c429 Mon Sep 17 00:00:00 2001 From: Nick Lewycky Date: Thu, 19 Jul 2012 22:35:28 +0000 Subject: [PATCH] Don't wipe out global variables that are probably storing pointers to heap memory. This makes clang play nice with leak checkers. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@160529 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/IPO/GlobalOpt.cpp | 179 +++++++++++++++++- .../2009-11-16-BrokenPerformHeapAllocSRoA.ll | 2 +- .../GlobalOpt/cleanup-pointer-root-users.ll | 20 ++ 3 files changed, 192 insertions(+), 9 deletions(-) create mode 100644 test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll diff --git a/lib/Transforms/IPO/GlobalOpt.cpp b/lib/Transforms/IPO/GlobalOpt.cpp index 4e1c23c1980..d8edffcd70d 100644 --- a/lib/Transforms/IPO/GlobalOpt.cpp +++ b/lib/Transforms/IPO/GlobalOpt.cpp @@ -296,6 +296,159 @@ static bool AnalyzeGlobal(const Value *V, GlobalStatus &GS, return false; } +/// isLeakCheckerRoot - Is this global variable possibly used by a leak checker +/// as a root? If so, we might not really want to eliminate the stores to it. +static bool isLeakCheckerRoot(GlobalVariable *GV) { + // A global variable is a root if it is a pointer, or could plausibly contain + // a pointer. There are two challenges; one is that we could have a struct + // the has an inner member which is a pointer. We recurse through the type to + // detect these (up to a point). The other is that we may actually be a union + // of a pointer and another type, and so our LLVM type is an integer which + // gets converted into a pointer, or our type is an [i8 x #] with a pointer + // potentially contained here. + + if (GV->hasPrivateLinkage()) + return false; + + SmallVector Types; + Types.push_back(cast(GV->getType())->getElementType()); + + unsigned Limit = 20; + do { + Type *Ty = Types.pop_back_val(); + switch (Ty->getTypeID()) { + default: break; + case Type::PointerTyID: return true; + case Type::ArrayTyID: + case Type::VectorTyID: { + SequentialType *STy = cast(Ty); + Types.push_back(STy->getElementType()); + break; + } + case Type::StructTyID: { + StructType *STy = cast(Ty); + if (STy->isOpaque()) return true; + for (StructType::element_iterator I = STy->element_begin(), + E = STy->element_end(); I != E; ++I) { + Type *InnerTy = *I; + if (isa(InnerTy)) return true; + if (isa(InnerTy)) + Types.push_back(InnerTy); + } + break; + } + } + if (--Limit == 0) return true; + } while (!Types.empty()); + return false; +} + +/// Given a value that is stored to a global but never read, determine whether +/// it's safe to remove the store and the chain of computation that feeds the +/// store. +static bool IsSafeComputationToRemove(Value *V) { + do { + if (!V->hasOneUse()) + return false; + if (isa(V) || isa(V) || isa(V)) + return false; + if (isAllocationFn(V) || isa(V)) + return true; + + Instruction *I = cast(V); + if (I->mayHaveSideEffects()) + return false; + if (GetElementPtrInst *GEP = dyn_cast(I)) { + if (!GEP->hasAllConstantIndices()) + return false; + } else if (I->getNumOperands() != 1) { + return false; + } + + V = I->getOperand(0); + } while (1); +} + +/// CleanupPointerRootUsers - This GV is a pointer root. Loop over all users +/// of the global and clean up any that obviously don't assign the global a +/// value that isn't dynamically allocated. +static bool CleanupPointerRootUsers(GlobalVariable *GV) { + bool Changed = false; + + // If Dead[n].first is the only use of a malloc result, we can delete its + // chain of computation and the store to the global in Dead[n].second. + SmallVector, 32> Dead; + + // Constants can't be pointers to dynamically allocated memory. + for (Value::use_iterator UI = GV->use_begin(), E = GV->use_end(); + UI != E;) { + User *U = *UI++; + if (StoreInst *SI = dyn_cast(U)) { + Value *V = SI->getValueOperand(); + if (isa(V)) { + Changed = true; + SI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(V)) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, SI)); + } + } else if (MemSetInst *MSI = dyn_cast(U)) { + if (isa(MSI->getValue())) { + Changed = true; + MSI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(MSI->getValue())) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, MSI)); + } + } else if (MemTransferInst *MTI = dyn_cast(U)) { + GlobalVariable *MemSrc = dyn_cast(MTI->getSource()); + if (MemSrc && MemSrc->isConstant()) { + Changed = true; + MTI->eraseFromParent(); + } else if (Instruction *I = dyn_cast(MemSrc)) { + if (I->hasOneUse()) + Dead.push_back(std::make_pair(I, MTI)); + } + } else if (ConstantExpr *CE = dyn_cast(U)) { + if (CE->use_empty()) { + CE->destroyConstant(); + Changed = true; + } + } else if (Constant *C = dyn_cast(U)) { + if (SafeToDestroyConstant(C)) { + C->destroyConstant(); + // This could have invalidated UI, start over from scratch. + Dead.clear(); + CleanupPointerRootUsers(GV); + return true; + } + } + } + + for (int i = 0, e = Dead.size(); i != e; ++i) { + if (IsSafeComputationToRemove(Dead[i].first)) { + Dead[i].second->eraseFromParent(); + Instruction *I = Dead[i].first; + do { + Instruction *J = dyn_cast(I->getOperand(0)); + if (!J) + break; + I->eraseFromParent(); + I = J; + } while (!isAllocationFn(I)); + I->eraseFromParent(); + } + } + + if (GV->use_empty()) { + GV->eraseFromParent(); + ++NumDeleted; + Changed = true; + } + + return Changed; +} + /// CleanupConstantGlobalUsers - We just marked GV constant. Loop over all /// users of the global, cleaning up the obvious ones. This is largely just a /// quick scan over the use list to clean up the easy and obvious cruft. This @@ -812,13 +965,18 @@ static bool OptimizeAwayTrappingUsesOfLoads(GlobalVariable *GV, Constant *LV, // If we nuked all of the loads, then none of the stores are needed either, // nor is the global. if (AllNonStoreUsesGone) { - DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n"); - CleanupConstantGlobalUsers(GV, 0, TD, TLI); + if (isLeakCheckerRoot(GV)) { + Changed |= CleanupPointerRootUsers(GV); + } else { + Changed = true; + CleanupConstantGlobalUsers(GV, 0, TD, TLI); + } if (GV->use_empty()) { + DEBUG(dbgs() << " *** GLOBAL NOW DEAD!\n"); + Changed = true; GV->eraseFromParent(); ++NumDeleted; } - Changed = true; } return Changed; } @@ -1794,10 +1952,15 @@ bool GlobalOpt::ProcessInternalGlobal(GlobalVariable *GV, if (!GS.isLoaded) { DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV); - // Delete any stores we can find to the global. We may not be able to - // make it completely dead though. - bool Changed = CleanupConstantGlobalUsers(GV, GV->getInitializer(), - TD, TLI); + bool Changed; + if (isLeakCheckerRoot(GV)) { + // Delete any constant stores to the global. + Changed = CleanupPointerRootUsers(GV); + } else { + // Delete any stores we can find to the global. We may not be able to + // make it completely dead though. + Changed = CleanupConstantGlobalUsers(GV, GV->getInitializer(), TD, TLI); + } // If the global is dead now, delete it. if (GV->use_empty()) { @@ -1845,7 +2008,7 @@ bool GlobalOpt::ProcessInternalGlobal(GlobalVariable *GV, if (GV->use_empty()) { DEBUG(dbgs() << " *** Substituting initializer allowed us to " - << "simplify all users and delete global!\n"); + << "simplify all users and delete global!\n"); GV->eraseFromParent(); ++NumDeleted; } else { diff --git a/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll b/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll index 54e8f909790..40862bd038e 100644 --- a/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll +++ b/test/Transforms/GlobalOpt/2009-11-16-BrokenPerformHeapAllocSRoA.ll @@ -17,7 +17,7 @@ define void @test() nounwind ssp { %2 = sext i32 %1 to i64 ; [#uses=1] %3 = mul i64 %2, ptrtoint (%struct.strchartype* getelementptr (%struct.strchartype* null, i64 1) to i64) ; [#uses=1] %4 = tail call i8* @malloc(i64 %3) ; [#uses=1] -; CHECK: call i8* @malloc(i64 +; CHECK-NOT: call i8* @malloc(i64 %5 = bitcast i8* %4 to %struct.strchartype* ; <%struct.strchartype*> [#uses=1] store %struct.strchartype* %5, %struct.strchartype** @chartypes, align 8 ret void diff --git a/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll b/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll new file mode 100644 index 00000000000..0d5cbf53330 --- /dev/null +++ b/test/Transforms/GlobalOpt/cleanup-pointer-root-users.ll @@ -0,0 +1,20 @@ +; RUN: opt -globalopt -S -o - < %s | FileCheck %s + +@test1 = internal global i8* null + +define void @test1a() { +; CHECK: @test1a +; CHECK-NOT: store +; CHECK-NEXT: ret void + store i8* null, i8** @test1 + ret void +} + +define void @test1b(i8* %p) { +; CHECK: @test1b +; CHECK-NEXT: store +; CHECK-NEXT: ret void + store i8* %p, i8** @test1 + ret void +} +