[llvm-commits] Patch to update available analysis correctly

[Matthijs Kooijman]

Hi All,

please find a patch attached, which fixes the removeDeadPasses method of
PMDataManager. This method removes any passes that are no longer used from
the AvailableAnalysis map. This map maps PassInfo* to Pass*.

In this map, a pass can appear multiple times, one time for its own PassInfo,
and one for each analysis interface it implements. This is handled by
recordAvailableAnalysis, which calls the getInterfacesImplemented method on
the pass being added, and adds the pass to the map for each interface
implemented.

The removeDeadPasses method, however, does not do this iteration. It only
removes the entry for the Pass itself from the map, leaving the entries for
the interfaces the pass implements. This results in a pass that is no longer
available (in particular, it has its releaseMemory method called) to be listed
as the available pass for all the interfaces it implements.

I can't find anything that caused this recently, it seems that this bug has
been present for quite some time. Since this seems like quite a fundamental
bug and I find it unlikely that is was not noticed before, I slightly doubt my
analysis. However, the attached patch, which teaches removeDeadPasses to also
remove the entries for interfaces a Pass implements, fixes my problems.

As to those problems. I've noticed these problems first in our custom
frontend. Fortunately, they turn out to be easily reproduced by pulling the
following program through opt -inline -internalize:

	define void @foo() nounwind {
					ret void
	}

	define void @main(...) nounwind {
					call void @foo()
					ret void
	}


The -inline option causes a callgraph to be constructed, and released
immediately after inlining. The internalize pass knows how to update a call
graph, but assumes that the callgraph is consistent with the code, so it
crashes with the following assertion:
	opt: /home/kooijman/src/llvm-trunk/include/llvm/Analysis/CallGraph.h:103:
	llvm::CallGraphNode* llvm::CallGraph::operator[](const llvm::Function*):
	Assertion `I != FunctionMap.end() && "Function not in callgraph!"' failed.

It seems that internalize updating CallGraph was only added in r56996 three
days ago, which would explain this particular case from breaking now. Possibly
all other analysis updating code is more robust and does not assume that the
analysises they update are consistent (though this robustness is probably
accidental), or there are no cases where analysises are updated after they got
freed in the standard pass orderings.

In any case, I think this is good to fix, and probably to get into 2.4 as
well. I won't be able to commit this until next monday, so please commit the
patch if you think it is correct.

Gr.

Matthijs
-------------- next part --------------
Index: lib/VMCore/PassManager.cpp
===================================================================
--- lib/VMCore/PassManager.cpp	(revision 57166)
+++ lib/VMCore/PassManager.cpp	(working copy)
@@ -779,13 +779,23 @@
     if (TheTimeInfo) TheTimeInfo->passStarted(*I);
     (*I)->releaseMemory();
     if (TheTimeInfo) TheTimeInfo->passEnded(*I);
+    if (const PassInfo *PI = (*I)->getPassInfo()) {
+      std::map<AnalysisID, Pass*>::iterator Pos =
+        AvailableAnalysis.find(PI);
 
-    std::map<AnalysisID, Pass*>::iterator Pos = 
-      AvailableAnalysis.find((*I)->getPassInfo());
-    
-    // It is possible that pass is already removed from the AvailableAnalysis
-    if (Pos != AvailableAnalysis.end())
-      AvailableAnalysis.erase(Pos);
+      // It is possible that pass is already removed from the AvailableAnalysis
+      if (Pos != AvailableAnalysis.end())
+        AvailableAnalysis.erase(Pos);
+
+      // Remove all interfaces this pass implements, for which it is also
+      // listed as the available implementation.
+      const std::vector<const PassInfo*> &II = PI->getInterfacesImplemented();
+      for (unsigned i = 0, e = II.size(); i != e; ++i) {
+        Pos = AvailableAnalysis.find(II[i]);
+        if (Pos != AvailableAnalysis.end() && Pos->second == *I)
+          AvailableAnalysis.erase(Pos);
+      }
+    }
   }
 }
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20081006/d85c2d59/attachment.sig>