[cfe-dev] Replacing the analyzer's CallOrObjCMessage with something more general

[Ted Kremenek]

Hi Jordan,

I think this is great.  It's a great infrastructural improvement that will serve as well.  Comments inline.

On Jun 21, 2012, at 11:04 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> Hi, Ted, Anna. In order to pave the way for proper handling of destructors and new-expressions, I'm working on replacing CallOrObjCMessage with a new class tree, CallAction. Like CallOrObjCMessage, CallAction bundles together a particular function/method/block invocation and a snapshot of where it occurred; unlike CallOrObjCMessage, it is done using "proper" OO and is a lot more maintainable.

Using proper OO is fine, and it remains lightweight.

> 
> The idea is that in many cases, we don't really care /what/ is being called as long as we can do something useful with it. Currently there are a lot of places that either switch based on what's being called (ExprEngine::invalidateArguments), or funnel a bunch of other checks into a common method (CallAndMessageChecker, RetainCountChecker -- currently using CallOrObjCMessage). Other checks like AttrNonNullChecker would probably be useful for constructors and possibly Objective-C methods, but currently just check CallExprs.

Makes sense.  As you well know, many checkers *do* care what is being called, but I don't see why this model doesn't allow more specialized behavior when necessary.

> 
> The other flaw with CallOrObjCMessage is that a lot of the checkers that use it assume that there is an expression in the source that corresponds to the call or for each argument, which is not true for destructors or operator new size arguments. CallAction will at least make it safe to ask for the SVal corresponding to an argument that isn't in the AST, and it should provide a source range for each argument that can be used for diagnostics.

Sounds great to me.  It abstracts away this detail, which is something checker author's shouldn't be worrying about anyway.

> 
> Currently I have it set up like this:
> 
> CallAction
> |-AnyFunctionCall (things that might have FunctionDecls) 
> | |-SimpleCall (things that are written as CallExprs)
> | | |-FunctionCall (simple C functions, and anything unknown)
> | | |-CXXMemberCall (calls to C++ non-static member functions)
> | | \-BlockCall (calls to blocks)***
> | |-CXXConstructorCall (calls to constructors)
> | |-*CXXDestructorCall
> | |-*CXXAllocator
> | |-*CXXDeallocator
> | \-*AttributeCleanup (__attribute__((cleanup))-style destructors)
> \-ObjCMessageInvocation (explicit sends, property accesses, and eventually subscripting, via ObjCMessage)
> 
> The ones with * are not implemented yet. BlockCall is annotated with *** because while blocks are written as CallExprs, they do not have an associated FunctionDecl, meaning that SimpleCall's access to the arguments can be inherited by AnyFunctionCall's type-checking and such must be overridden. (Lambdas do not have this problem because they are actually objects with an operator(); they will show up in CXXMemberCall.)

Sounds good.  Note that many of these will eventually have proper CFG-support as well, including __attribute((cleanup)) style destructors.  Having CallAction wrap the appropriate CFG elements might be the way to go, but we can implement those as the needed functionality comes up in libAnalysis.

> 
> Eventually I'd like to completely eliminate CallOrObjCMessage. ObjCMessage might still be good to keep around, since it's a little lighter (it doesn't wrap state and location context in with the expressions). We can then have new checker callbacks preCall and postCall, which don't care how a function or method is being invoked. (evalCall would probably also use CallAction, but still not be called for Objective-C messages.)

Is there really any benefit to keeping ObjCMessage around and just use ObjCMethodInvocation?  The cost you are talking about is insignificant compare to where the real performance issues are with the analyzer.  These objects are created far too infrequently to matter in practice.

> 
> This first draft does use virtual methods. I haven't done any performance testing yet, but if it turns out to be an issue I can change to using "static dynamic" dispatch using a switch.

I think virtual methods are fine.  Again, this logic is called for too infrequently to really matter in practice.  The cost of the dynamic dispatch is dwarfed by the work in the checkers themselves.

> 
> Finally, the names are definitely tentative. Any better suggestions?

CallAction seems like the name of a callback object.  What about something like "Call".  That's what you name the header file anyway.  It's succinct, and is in a completely different namespace then the rest of the compiler.

My other suggestion is to make ObjCMessageInvocation be ObjCMethodCall.  The "Call" part keeps the name matching with the rest of the subclasses of CallAction.  While the language construct is a message expression, the semantics we are trying to model is a call.  This seems natural to me since CallAction deals with the semantics, not the syntax.