Change CallGraphNode to maintain it's Function as an AssertingVH

for sanity.  This didn't turn up any bugs.

Change CallGraphNode to maintain its "callsite" information in the 
call edges list as a WeakVH instead of as an instruction*.  This fixes
a broad class of dangling pointer bugs, and makes CallGraph have a number
of useful invariants again.  This fixes the class of problem indicated
by PR4029 and PR3601.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@80663 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/Analysis/CallGraph.h

@@ -55,6 +55,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CallSite.h"
+#include "llvm/Support/ValueHandle.h"
 #include "llvm/System/IncludeFile.h"
 #include <map>
 
@@ -158,11 +159,16 @@ protected:
 };
 
 //===----------------------------------------------------------------------===//
-// CallGraphNode class definition
+// CallGraphNode class definition.
 //
 class CallGraphNode {
-  Function *F;
-  typedef std::pair<CallSite,CallGraphNode*> CallRecord;
+  AssertingVH<Function> F;
+  
+  // CallRecord - This is a pair of the calling instruction (a call or invoke)
+  // and the callgraph node being called.
+public:
+  typedef std::pair<WeakVH, CallGraphNode*> CallRecord;
+private:
   std::vector<CallRecord> CalledFunctions;
   
   /// NumReferences - This is the number of times that this CallGraphNode occurs
@@ -240,19 +246,22 @@ public:
   /// addCalledFunction - Add a function to the list of functions called by this
   /// one.
   void addCalledFunction(CallSite CS, CallGraphNode *M) {
-    CalledFunctions.push_back(std::make_pair(CS, M));
+    CalledFunctions.push_back(std::make_pair(CS.getInstruction(), M));
     M->AddRef();
   }
 
+  void removeCallEdge(iterator I) {
+    I->second->DropRef();
+    *I = CalledFunctions.back();
+    CalledFunctions.pop_back();
+  }
+  
+  
   /// removeCallEdgeFor - This method removes the edge in the node for the
   /// specified call site.  Note that this method takes linear time, so it
   /// should be used sparingly.
   void removeCallEdgeFor(CallSite CS);
 
-  // FIXME: REMOVE THIS WHEN HACK IS REMOVED FROM CGSCCPASSMGR.
-  void removeCallEdgeFor(Instruction *CS);
-
-  
   /// removeAnyCallEdgeTo - This method removes all call edges from this node
   /// to the specified callee function.  This takes more time to execute than
   /// removeCallEdgeTo, so it should not be used unless necessary.
@@ -278,7 +287,7 @@ public:
 template <> struct GraphTraits<CallGraphNode*> {
   typedef CallGraphNode NodeType;
 
-  typedef std::pair<CallSite, CallGraphNode*> CGNPairTy;
+  typedef CallGraphNode::CallRecord CGNPairTy;
   typedef std::pointer_to_unary_function<CGNPairTy, CallGraphNode*> CGNDerefFun;
 
   static NodeType *getEntryNode(CallGraphNode *CGN) { return CGN; }

lib/Analysis/IPA/CallGraph.cpp

@@ -241,7 +241,7 @@ void CallGraphNode::dump() const { print(errs()); }
 void CallGraphNode::removeCallEdgeFor(CallSite CS) {
   for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) {
     assert(I != CalledFunctions.end() && "Cannot find callsite to remove!");
-    if (I->first == CS) {
+    if (I->first == CS.getInstruction()) {
       I->second->DropRef();
       *I = CalledFunctions.back();
       CalledFunctions.pop_back();
@@ -250,21 +250,6 @@ void CallGraphNode::removeCallEdgeFor(CallSite CS) {
   }
 }
 
-// FIXME: REMOVE THIS WHEN HACK IS REMOVED FROM CGSCCPASSMGR.
-void CallGraphNode::removeCallEdgeFor(Instruction *CS) {
-  for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) {
-    assert(I != CalledFunctions.end() && "Cannot find callsite to remove!");
-    if (I->first.getInstruction() == CS) {
-      I->second->DropRef();
-      *I = CalledFunctions.back();
-      CalledFunctions.pop_back();
-      return;
-    }
-  }
-  
-}
-
-
 
 // removeAnyCallEdgeTo - This method removes any call edges from this node to
 // the specified callee function.  This takes more time to execute than
@@ -285,7 +270,7 @@ void CallGraphNode::removeOneAbstractEdgeTo(CallGraphNode *Callee) {
   for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) {
     assert(I != CalledFunctions.end() && "Cannot find callee to remove!");
     CallRecord &CR = *I;
-    if (CR.second == Callee && CR.first.getInstruction() == 0) {
+    if (CR.second == Callee && CR.first == 0) {
       Callee->DropRef();
       *I = CalledFunctions.back();
       CalledFunctions.pop_back();
@@ -301,9 +286,9 @@ void CallGraphNode::replaceCallSite(CallSite Old, CallSite New,
                                     CallGraphNode *NewCallee) {
   for (CalledFunctionsVector::iterator I = CalledFunctions.begin(); ; ++I) {
     assert(I != CalledFunctions.end() && "Cannot find callsite to replace!");
-    if (I->first != Old) continue;
+    if (I->first != Old.getInstruction()) continue;
     
-    I->first = New;
+    I->first = New.getInstruction();
     
     // If the callee is changing, not just the callsite, then update it as
     // well.

lib/Analysis/IPA/CallGraphSCCPass.cpp

@@ -125,7 +125,7 @@ bool CGPassManager::RunPassOnSCC(Pass *P, std::vector<CallGraphNode*> &CurSCC,
 
 void CGPassManager::RefreshCallGraph(std::vector<CallGraphNode*> &CurSCC,
                                      CallGraph &CG) {
-  DenseMap<Instruction*, CallGraphNode*> CallSites;
+  DenseMap<Value*, CallGraphNode*> CallSites;
   
   DEBUG(errs() << "CGSCCPASSMGR: Refreshing SCC with " << CurSCC.size()
                << " nodes:\n";
@@ -146,12 +146,28 @@ void CGPassManager::RefreshCallGraph(std::vector<CallGraphNode*> &CurSCC,
     
     // Get the set of call sites currently in the function.
     for (CallGraphNode::iterator I = CGN->begin(), E = CGN->end(); I != E; ++I){
-      assert(I->first.getInstruction() &&
-             "Call site record in function should not be abstract");
-      assert(!CallSites.count(I->first.getInstruction()) &&
+      // If this call site is null, then the function pass deleted the call
+      // entirely and the WeakVH nulled it out.  
+      if (I->first == 0 ||
+          // If we've already seen this call site, then the FunctionPass RAUW'd
+          // one call with another, which resulted in two "uses" in the edge
+          // list of the same call.
+          CallSites.count(I->first) ||
+
+          // If the call edge is not from a call or invoke, then the function
+          // pass RAUW'd a call with another value.  This can happen when
+          // constant folding happens of well known functions etc.
+          CallSite::get(I->first).getInstruction() == 0) {
+        // Just remove the edge from the set of callees.
+        CGN->removeCallEdge(I);
+        E = CGN->end();
+        --I;
+        continue;
+      }
+      
+      assert(!CallSites.count(I->first) &&
              "Call site occurs in node multiple times");
-      CallSites.insert(std::make_pair(I->first.getInstruction(),
-                                      I->second));
+      CallSites.insert(std::make_pair(I->first, I->second));
     }
     
     // Loop over all of the instructions in the function, getting the callsites.
@@ -162,7 +178,7 @@ void CGPassManager::RefreshCallGraph(std::vector<CallGraphNode*> &CurSCC,
         
         // If this call site already existed in the callgraph, just verify it
         // matches up to expectations and remove it from CallSites.
-        DenseMap<Instruction*, CallGraphNode*>::iterator ExistingIt =
+        DenseMap<Value*, CallGraphNode*>::iterator ExistingIt =
           CallSites.find(CS.getInstruction());
         if (ExistingIt != CallSites.end()) {
           CallGraphNode *ExistingNode = ExistingIt->second;
@@ -201,18 +217,14 @@ void CGPassManager::RefreshCallGraph(std::vector<CallGraphNode*> &CurSCC,
       }
     
     // After scanning this function, if we still have entries in callsites, then
-    // they are dangling pointers.  Crap.  Well, until we change CallGraph to
-    // use CallbackVH, we'll just zap them here.  When we have that, this should
-    // turn into an assertion.
-    if (CallSites.empty()) continue;
+    // they are dangling pointers.  WeakVH should save us for this, so abort if
+    // this happens.
+    assert(CallSites.empty() && "Dangling pointers found in call sites map");
     
-    for (DenseMap<Instruction*, CallGraphNode*>::iterator I = CallSites.begin(),
-         E = CallSites.end(); I != E; ++I)
-      // FIXME: I had to add a special horrible form of removeCallEdgeFor to
-      // support this.  Remove the Instruction* version of it when we can.
-      CGN->removeCallEdgeFor(I->first);
-    MadeChange = true;
-    CallSites.clear();
+    // Periodically do an explicit clear to remove tombstones when processing
+    // large scc's.
+    if ((sccidx & 15) == 0)
+      CallSites.clear();
   }
 
   DEBUG(if (MadeChange) {

lib/Transforms/IPO/PruneEH.cpp

@@ -165,9 +165,6 @@ bool PruneEH::runOnSCC(std::vector<CallGraphNode *> &SCC) {
 // function if we have invokes to non-unwinding functions or code after calls to
 // no-return functions.
 bool PruneEH::SimplifyFunction(Function *F) {
-  CallGraph &CG = getAnalysis<CallGraph>();
-  CallGraphNode *CGN = CG[F];
-
   bool MadeChange = false;
   for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {
     if (InvokeInst *II = dyn_cast<InvokeInst>(BB->getTerminator()))
@@ -181,14 +178,13 @@ bool PruneEH::SimplifyFunction(Function *F) {
         Call->setAttributes(II->getAttributes());
 
         // Anything that used the value produced by the invoke instruction
-        // now uses the value produced by the call instruction.
+        // now uses the value produced by the call instruction.  Note that we
+        // do this even for void functions and calls with no uses so that the
+        // callgraph edge is updated.
         II->replaceAllUsesWith(Call);
         BasicBlock *UnwindBlock = II->getUnwindDest();
         UnwindBlock->removePredecessor(II->getParent());
 
-        // Fix up the call graph.
-        CGN->replaceCallSite(II, Call, 0/*keep callee*/);
-
         // Insert a branch to the normal destination right before the
         // invoke.
         BranchInst::Create(II->getNormalDest(), II);

lib/Transforms/Utils/InlineFunction.cpp

@@ -212,7 +212,7 @@ static void UpdateCallGraphAfterInlining(CallSite CS,
   }
 
   for (; I != E; ++I) {
-    const Instruction *OrigCall = I->first.getInstruction();
+    const Value *OrigCall = I->first;
 
     DenseMap<const Value*, Value*>::iterator VMI = ValueMap.find(OrigCall);
     // Only copy the edge if the call was inlined!

test/Transforms/ArgumentPromotion/callgraph-update.ll

@@ -0,0 +1,23 @@
+; RUN: llvm-as < %s | opt -argpromotion -simplifycfg -constmerge | llvm-dis
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
+target triple = "i386-apple-darwin10.0"
+
+%struct.VEC2 = type { double, double, double }
+%struct.VERTEX = type { %struct.VEC2, %struct.VERTEX*, %struct.VERTEX* }
+%struct.edge_rec = type { %struct.VERTEX*, %struct.edge_rec*, i32, i8* }
+
+declare %struct.edge_rec* @alloc_edge() nounwind ssp
+
+define i64 @build_delaunay(%struct.VERTEX* %tree, %struct.VERTEX* %extra) nounwind ssp {
+entry:
+  br i1 undef, label %bb11, label %bb12
+
+bb11:                                             ; preds = %bb10
+  %a = call %struct.edge_rec* @alloc_edge() nounwind ; <%struct.edge_rec*> [#uses=0]
+  ret i64 123
+
+bb12:                                             ; preds = %bb10
+  %b = call %struct.edge_rec* @alloc_edge() nounwind ; <%struct.edge_rec*> [#uses=1]
+  %c = ptrtoint %struct.edge_rec* %b to i64
+  ret i64 %c
+}