Bug 3888: cleanup attribute

[+cfe-dev list]

You’re very welcome to e-mail me directly! You should also feel free to CC the cfe-dev mailing list, so that other people can both see the results of our conversation and possibly answer questions faster than me. (I’m only spending a bit of time reviewing patches each day, I’m afraid.)

Take as long as you need, ask questions along the way, and thanks for looking into this!
Jordan

Hi Jordan,

I had a few moments to look at this again in the weekend. I guess I am
making progress but I feel that whenever I figure something out I also
find 3 new things I have to understand. I hope that you can help to
make a few things clearer for me.

I have been working on a few different approaches but I do not feel
sure about what the correct way forward is. I have created the
CFGGNUCleanup subclass of CFGElement and the misc methods to add it to
the CFG. But now I am unsure of whether to extend the logic in
addAutomaticObjDtors to also deal with VarDecls with cleanup
attribute? The method name would have to change but the alternative is
to add another method that would be called in all the same places as
addAutomaticObjDtors (the Visit* methods where vars go out of scope)
or even a new method to call both?.

I think extending the method is the right way to go. I wouldn’t even worry about changing the name; attribute((cleanup)) is very clearly a destructor-like thing already. (If you can think of a better name, you’re welcome to suggest it, though.)

I am not sure I even understand
how addAutomaticObjDtors is supposed to work. If I understand
correctly it will iterate over all VarDecl’s within the scope we are
going to leave. It seems to add the automaticObjDtor to all of them.
Is there some kind of filtering I just don’t understand or have I
misunderstood it completly?

It looks like only variables with non-trivial destructors are added to the local scope to begin with; see CFGBuilder::addLocalScopeForVarDecl (or just line 1337). We’d want to add variables with attribute((cleanup)) as well, and then be careful to handle them correctly in addAutomaticObjDtors. A brute-force way to handle this would be to replace LocalScope’s list of VarDecls with a PointerIntPair<VarDecl, 1, bool>, where the boolean value indicates whether this is being added for its attribute-provided cleanups or its destructor.

What does Clang do when compiling a variable with both attribute((cleanup)) and a non-trivial destructor? Because we should make sure we do the same thing in the CFG. (Also, do we accept multiple cleanup attributes on the same variable?)

I also had a look at ExprEngine to process the new CFGNUCleanup there.
Do I understand correctly that this would be the place where I should
add a VisitGNUCleanup method that will take the CFGGNUCleanup and
there create the statement to call the function function given in the
cleanup attribute with the variable as argument?

I would take a look at how ProcessImplicitDtor gets invoked, and mirror that.

Feel free to also send partial patches to the cfe-commits list, or post them at http://reviews.llvm.org with me as a reviewer and cfe-commits as a subscriber.

Thanks for looking at this!
Jordan