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

[Jordan Rose]

On Jun 21, 2012, at 2:54 PM, Ted Kremenek wrote:

> 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.
>> 
>> 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.

Oops, this was supposed to be that most checkers don't care /how/ something is being called. This should giwe us a double-free warning, for example:

void *x __attribute__((cleanup(free)));
x = malloc(1);
free(x);

But yes, it should be easy to take apart this abstraction. And this is probably the right place for a few of the helper functions that we've been putting on CheckerContext.


>> 
>> 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.

I was thinking about our checker callbacks for Objective-C messages, and trying to keep a distinction between preObjCMessage and preGenericCall. The actual issue that arises there is that other checkers may have updated the state inside the Call. But I'm very carefully making the state and location context an internal detail of Call objects, so the only thing being exposed is values bound to expressions...which shouldn't be changed by callbacks… As long as we're careful about what accessors are actually state-dependent, it should be okay, and then it's a nice unification.


>> 
>> 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.

I don't like "Call" because it's the natural variable name for these things. "C" is usually already a CheckerContext, and we're trying not to use lowercase variable names anymore. "Invocation"? "AbstractCall"? "AnyCall"?

Alternately, what's the natural variable name for a "Call" argument?


> 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.

Fair enough.