[Patch] Make ParseAST() take ASTContext and TU params

Hi,

Attached is a patch to make ParseAST() take ASTContext and
TranslationUnit as parameters, as suggested by Steve Naroff.

This makes the FreeMemory parameter obsolete, so it is removed.

The motivation is that clients are now responsible for owning those
objects, and are also able to free them themselves, rather than
leaking memory like before (when FreeMemory was false).

I've made the new parameters default to 0, and if they are null then
ParseAST will allocate them on its own and dispose of them after (the
same functionality as when FreeMemory was false previously).

-Alexei

ParseAST.diff (3.38 KB)

Alexei Svitkine wrote:

Hi,

Attached is a patch to make ParseAST() take ASTContext and
TranslationUnit as parameters, as suggested by Steve Naroff.
  
The double negative in the code confuses me somewhat, but isn't this
condition the wrong way round?

+ if (!DisableFree) {
+ Context = new ASTContext(PP.getLangOptions(),
PP.getSourceManager(),
+ PP.getTargetInfo(),
+ PP.getIdentifierTable(),
PP.getSelectorTable());
+ TU = new TranslationUnit(*Context);
+ }
+ ParseAST(PP, Consumer.get(), Context, TU, Stats);

Previously, if DisableFree was true, FreeMemory was false, and ParseAST
didn't free the objects.
Now, if DisableFree is true, the driver doesn't allocate objects, so
ParseAST does and will free them.

Sebastian

You are right. Here's a revised patch.

-Alexei

ParseAST2.diff (3.38 KB)

Here's an updated patch that also removes the comment about the
FreeMemory param which no longer exists.

-Alexei

ParseAST3.diff (3.49 KB)

If no one objects to these changes, can someone commit this to the repository?

-Alexei

Hi Alex,

Sorry for not looking at this sooner. The patch looks good, but I have two additional nits.

First, could you possibly add a comment to the prototype and definition of ParseAST indicating what is the behavior of TranslationUnit and ASTContext are null? The postcondition for this situation is that once ParseAST completes then any subsequent accesses to the ASTs it provided is no longer valid (because they are reclaimed). This should be explicitly stated, since the ASTConsumer (which lives beyond the call to ParseAST) could potentially have dangling references to the ASTs.

Second, does it make sense to pass in both a TranslationUnit and an ASTContext pointer? Since TranslationUnit holds a reference to ASTContext, I think we only need a single argument. If the TranslationUnit* is null then the ASTContext* is null as well.

Ted

Hi Alexei,

I apologize, I left the "ei" off your name.

Thanks for the comments!

Here's a revised patch.

-Alexei

ParseAST4.diff (4.42 KB)

Oops some tabs made it into the previous one.

Here's the same thing with correct whitespace.

-Alexei

ParseAST5.diff (4.42 KB)

Hi Alexei,

Looks great!

I'm actually having difficulty applying the patch. Did you generate it against top-of-tree clang?

Ted

Oops - it looks like there was a change to ParseAST() earlier that I
wasn't up to date with. This one:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090126/011412.html

That change involves propagating the FreeMemory param to the
ASTContext constructor... which is problematic with my change which
gets rid of that param entirely. I'm not sure how to handle this.

What is the purpose of -disable-free in the context of the
command-line clang tool?

-Alexei

Oops - it looks like there was a change to ParseAST() earlier that I
wasn’t up to date with. This one:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090126/011412.html

That change involves propagating the FreeMemory param to the
ASTContext constructor… which is problematic with my change which
gets rid of that param entirely. I’m not sure how to handle this.

The “FreeMemory” argument to ASTContext is a little of a misnomer. It’s more a selector of which Allocator that ASTContext uses to allocate ASTs (i.e., MallocAllocator or BumpPtrAllocator). Calling ~ASTContext() will always release memory. Feel free to rename that parameter to something more appropriate.

Much of your patch is orthogonal to these changes. Your patch is about creating a new interface for ParseAST that allows clients to hold on the TranslationUnit/ASTContext after ParseAST has completed.

What is the purpose of -disable-free in the context of the
command-line clang tool?

Our timings show that a significant amount of time is spent in clang freeing ASTs.

When invoking ‘clang’ for compilation from the command-line, freeing ASTs is a waste of time since the OS will automatically reclaim that memory. Providing -disable-free from the command line allows us to experiment with the performance of just having the OS free memory versus freeing the memory directly.

Moreover, having it as a command-line option (rather than hard-coded) allows us to free memory ASTs manually for debugging (to make sure all the ownership issues are correct) but still have the --disable-free option for production releases.

Ah, thanks for the clarification.

Here's an updated patch that passes the correct parameters to the
ASTContext() constructor under each case. I didn't rename the
parameter in ASTContext however - since it actually does not call
Deallocate when FreeMemory is false.

-Alexei

ParseAST6.diff (4.52 KB)

Applied!

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090126/011422.html