Crash with -serialize

Hi,

`clang -serialize` does crash for all input files. This is because of the following commit:

     URL: http://llvm.org/viewvc/llvm-project?rev=54502&view=rev
     Log:
     ParseAST now never releases the passed ASTConsumer. This is the responsibility of the client.

While I understand why not deleting the Consumer is nice, it's not as simple as this patch pretends: The TranslationUnit is a local variable of ParseAST() and is destroyed as soon as the function exits. Most Consumers keep a reference to the TU and do work in their destructor, which will now be always run when the TU is already dead (e.g. the ASTSerializers).

So, either that commit is undone, or the TranslationUnit has to be allocated on the stack somehow.

Nico

Nico Weber wrote:

Hi,

`clang -serialize` does crash for all input files. This is because of the following commit:

     URL: http://llvm.org/viewvc/llvm-project?rev=54502&view=rev
     Log:
     ParseAST now never releases the passed ASTConsumer. This is the responsibility of the client.

While I understand why not deleting the Consumer is nice, it's not as simple as this patch pretends: The TranslationUnit is a local variable of ParseAST() and is destroyed as soon as the function exits. Most Consumers keep a reference to the TU and do work in their destructor, which will now be always run when the TU is already dead (e.g. the ASTSerializers).

So, either that commit is undone, or the TranslationUnit has to be allocated on the stack somehow.
  
How about having all Consumers do work in the HandleTranslationUnit method, instead of their destructor, wouldn't this fix the problem ?

-Argiris

So, either that commit is undone, or the TranslationUnit has to be
allocated on the stack somehow.

That was supposed to say "heap" :stuck_out_tongue:

That sounds reasonable. Somewhat unrelated: What about adding a new method FinalizeTranslationUnit(TranslationUnit* TU) that is only called in ParseAST if !PP.getDiags().hasErrorOccurred()? Nearly all the consumers check for that manually at the moment.

Nico