Ownership of declarations in clang?

Currently, clang leaks a lot of stuff. I was looking at this using
valgrind, and then I was trying to figure out what code is supposed to
be responsible for cleaning up the declarations generated by Sema
after the file is finished being parsed. I wasn't able to find any
such code.

What exactly is the expected ownership model for the declarations
generated by Sema? They obviously need to be around until we are
finished parsing the file, but by then, it appears only the parser
(specifically, the root scope) is holding onto pointers to
declarations, and it doesn't know how to delete them.

-Eli

Currently, clang leaks a lot of stuff.

Yes :(, this is an open area that is very important.

I was looking at this using
valgrind, and then I was trying to figure out what code is supposed to
be responsible for cleaning up the declarations generated by Sema
after the file is finished being parsed. I wasn't able to find any
such code.

Ted may know better than me at this point, but I think the idea is that the TranslationUnit object should own (and thus destroy) the decls when it is torn down.

What exactly is the expected ownership model for the declarations
generated by Sema? They obviously need to be around until we are
finished parsing the file, but by then, it appears only the parser
(specifically, the root scope) is holding onto pointers to
declarations, and it doesn't know how to delete them.

TranslationUnit should own them. I don't know what its current state is, Ted?

In addition to decls, sema leaks memory in many cases when an error occurs. I implemented what I think is the best solution in ActOnCallExpr:

1) on entry, the CallExpr AST node is created, which takes ownership of all the operands.
2) the ast node is owned by an OwningPtr<> object.
3) various checks are done, if the code is invalid and an early exit is taken, the Call, and all its operands, is deleted.
4) if the expr checks out, the callexpr is returned by using the .take() method on the OwningPtr.

I think this leads to relatively simple code and uses RAII to prevent obvious leaks. Most of sema isn't following this approach yet though.

Also, I'm not sure if all the virtual dtors for the various AST nodes are deleting their children right now.

-Chris

I was looking at this using
valgrind, and then I was trying to figure out what code is supposed to
be responsible for cleaning up the declarations generated by Sema
after the file is finished being parsed. I wasn't able to find any
such code.

Ted may know better than me at this point, but I think the idea is
that the TranslationUnit object should own (and thus destroy) the
decls when it is torn down.

The TranslationUnit object is currently only used when serializing/deserializing ASTs from Bitcode. When building ASTs from source, lines 1252-1296 of clang.cpp construct the SourceManager object, Preprocessor, and then calls ProcessInputFile (which then just streams ASTs to an ASTConsumer). In this scenario there is now ownership of the top-level decls.

The TranslationUnit object can be easily modified to have its dstor delete the top-level decls that it owns. That's not the big problem. The major issue is that when we parse source we aren't using TranslationUnit or some similar abstraction to encapsulate ownership of the ASTs.

What exactly is the expected ownership model for the declarations
generated by Sema? They obviously need to be around until we are
finished parsing the file, but by then, it appears only the parser
(specifically, the root scope) is holding onto pointers to
declarations, and it doesn't know how to delete them.

TranslationUnit should own them. I don't know what its current state
is, Ted?

This is where I think things should go. It was cludged together enough to get serialization working, but the driver needs to be reworked to integrate TranslationUnit into the default parsing process.

The other issue is whether or not TranslationUnit and ASTContext should just be merged. Over time, while I was working on the serializer, I think it was decided to have one ASTContext/SourceManager per translation unit. If this still is the case, perhaps having both entities is redundant.

Also, I'm not sure if all the virtual dtors for the various AST nodes
are deleting their children right now.

We don't do any cleanup of this kind at all right now. The virtual dstors for AST nodes have never been implemented. We talked about doing this, but it remains a TODO.