[PATCH][LegacyPassManager] If a ModulePass uses a pass that is not listed in its

[jasonk]

Summary

getAnalysisUsage(), instead of triggering an appropriate
error message, a double delete of the pass happens.

This patch prevents that double free by moving the delete point around.
This also ensures that a helpful assert (which already exists) is triggered
instead.

Unfortunately, there are other issues exposed by this patch.

Because the Legacy Pass Manager's ModulePass is itself not heavily exercised in LLVM, and the required ImmutablePass's (such as DataLayout) are not properly propagated over to the other Analysis passes (e.g. ScalarEvolution gets a NULL DataLayout)

Even though the ModulePass gets the "proper" DataLayout, when it does a getAnalysis<ScalarEvolution>(), the DataLayout instance returned to ScalarEvolution::runOnFunctiion() is NULL.

This problem persists onto the other analysis passes (such as DependenceAnalysis - i.e. it always results in MayAlias being produced)..

Apologies for the delay in posting this patch.

[atrick]

PassManagerImpl::add(P) has a contract that states that it takes ownership of P and deletes it. So you can't change the implementation of schedulePass(P) without changing that contract and updating all users of PassManager::add.

I don't particularly like that interface and have been bitten by it, but that's the way the API works.

I'm not sure how this double-delete is happening. Is there some other way to avoid it. Callers of PassManager::add() or schedulePas() should no longer refer to the pass object that they send as the argument.

[jasonk]

{F52644}

{F52645}

I am having a bit of churn getting back to vanilla llvm 32-bit land so I can't readily reproduce the error (double free) but here are the two patches that you can use to reproduce:

The first patch adds a dummy ModulePass called foo (call by opt -foo <x.ll)
it does NOT list DataLayout as a dependency, but it is using it "on-the-fly" in runOnModule()

Without the second patch, you should see a doublefree occur.
With the second patch applied, you should see the proper assert happening instead.
In the meantime, as soon as I get my gcc-4.8 upgrade hassle straightened out, I'll try to produce the stacktrace of the doublefree

[atrick]

I spent quite some time debugging this. Once I figured out the issue, a fix was easy so I committed r205753.

I don't think it's essential to have more unit tests for the LegacyPassManager, but it would be nice to create more tests for the new PassManager. Chandler can tell you how to do it and whether it's worthwhile in this case.

[jasonk]

Thanks!! :-)

lib/IR/LegacyPassManager.cpp

   // available at this point.
   const PassInfo *PI =
     PassRegistry::getPassRegistry()->getPassInfo(P->getPassID());
-  if (PI && PI->isAnalysis() && findAnalysisPass(P->getPassID())) {
-    delete P;
+  // don't delete the pass here. Instead delete it elsewhere
+  if (PI && PI->isAnalysis() && findAnalysisPass(P->getPassID()))
     return;
-  }
+
 
   AnalysisUsage *AnUsage = findAnalysisUsage(P);
 

   assert((P->getPotentialPassManagerType() <
           RequiredPass->getPotentialPassManagerType()) &&
          "Unable to handle Pass that requires lower level Analysis pass");
-
+  if (RequiredPass == NULL)
+    return;
   FunctionPassManagerImpl *FPP = OnTheFlyManagers[P];
   if (!FPP) {
     FPP = new FunctionPassManagerImpl();

 
     OnTheFlyManagers[P] = FPP;
   }
-  FPP->add(RequiredPass);
+  const PassInfo * RequiredPassPI =
+    PassRegistry::getPassRegistry()->getPassInfo(RequiredPass->getPassID());
+
+  if (RequiredPassPI && RequiredPassPI->isAnalysis()
+      && ((PMTopLevelManager*)FPP)->findAnalysisPass(P->getPassID())) {
+    delete RequiredPass;
+    RequiredPass = NULL;
+  } else
+    FPP->add(RequiredPass);
 
   // Register P as the last user of RequiredPass.
   if (RequiredPass) {