[llvm-commits] [llvm] r103425 - in /llvm/trunk: include/llvm/PassManager.h lib/VMCore/PassManager.cpp test/Other/2010-05-60-Printer.ll

[Chris Lattner]

On May 10, 2010, at 1:24 PM, David Greene wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=103425&view=rev
> Log:
> 
> Fix PR6875:
> 
> This includes a patch by Roman Divacky to fix the initial crash.
> 
> Move the actual addition of passes from *PassManager::add to
> *PassManager::addImpl.  That way, when adding printer passes we won't
> recurse infinitely.
> 
> Finally, check to make sure that we are actually adding a FunctionPass
> to a FunctionPassManager before doing a print before or after it.
> Immutable passes are strange in this way because they aren't
> FunctionPasses yet they can be and are added to the FunctionPassManager.

This patch broke the buildbot.  Please test all patches before submitting or committing them.

> Added:
>    llvm/trunk/test/Other/2010-05-60-Printer.ll

You typo'd the name of the test, please svn mv it to the right name.


> /// add - Add a pass to the queue of passes to run.  This passes
> /// ownership of the Pass to the PassManager.  When the
> /// PassManager_X is destroyed, the pass will be destroyed as well, so
> /// there is no need to delete the pass. (TODO delete passes.)
> /// This implies that all passes MUST be allocated with 'new'.
> void FunctionPassManager::add(Pass *P) { 
> -  if (ShouldPrintBeforePass(P))
> -    add(P->createPrinterPass(dbgs(), std::string("*** IR Dump Before ")
> -                             + P->getPassName() + " ***"));
> -  FPM->add(P);
> -
> -  if (ShouldPrintAfterPass(P))
> -    add(P->createPrinterPass(dbgs(), std::string("*** IR Dump After ")
> -                             + P->getPassName() + " ***"));
> +  // If this is a not a function pass, don't add a printer for it.
> +  if (P->getPassKind() == PT_Function)
> +    if (ShouldPrintBeforePass(P))
> +      addImpl(P->createPrinterPass(dbgs(), std::string("*** IR Dump Before ")
> +                                   + P->getPassName() + " ***"));

This doesn't seem like a great fix here.  You're seriously violating object orientation by doing this.  Can't you sink the 'ShouldPrintAfterPass' logic down into the pass manager to the point where it actually adds the passes to the pass managers?  You didn't really explain what the actual bug is that you're fixing, but I assume that it has to do with adding printers multiple times?

-Chris