[cfe-commits] r153010 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc-plist.c

[Jordan Rose]

On Mar 18, 2012, at 15:14, Anna Zaks wrote:

> On Mar 18, 2012, at 11:33 AM, Jordan Rose wrote:
> 
>> Possibilities:
>> - Store the generic visitors (added by BugReporter) separately from the visitors added by the checkers, and run them second.
>> - Add the generic visitors unconditionally when BugReports are created. Since ConditionBRVisitor and NilReceiverVisitor don't have state, we could just have a cached instance of each, saving allocation costs.
>> - Change the visitor data structure to not be an ImmutableList, and require that visitors not add other visitors once the report has started. NilReceiverBRVisitor relies on this, though.
>> 
>> I'm leaning towards the second one right now. Thoughts?
> 
> I also like the idea of visitors adding to the list of interesting symbols. Further, you could have a bunch of generic visitors and allow the checkers to choose which (of the generic ones) to use. You can also have a default list which gets generated with each BugReport by default.

Right now this is done by the bug reporter simply adding ConditionBRVisitor and NilReceiverVisitor to every report just before emitting path diagnostics. (Presumably this is so that if we /don't/ emit path diagnostics, we don't incur the hit of constructing two heap-allocated visitors. Even though they're empty objects.)

As for choosing which generic visitors to use, wouldn't this just mean taking away the generic ones entirely, or only adding them if there were no custom-specified reports? I feel like that's just perpetuating the hack I committed.

> Before you go down the road of changing the order, try to experiment with it (using the simplest solution) to see if there are any unknown issues that reordering might lead to in terms of desired output. For example, what happens if you have default and custom visitors generating a note for the same pair of nodes? The order might change the quality of output. 

Good point. But it seems right now there are NO custom visits besides reallocation that generate notes on the same nodes as the default visitors, at least not in any of our plist checks. (Or in retain-release.m, which I checked as well because there aren't many plist tests.) Looking at the existing custom visitors, none of them seem like they would fire on the same nodes.

So, with the hack in place:

Assuming 'tmp' is NULL
Reallocation failed
Taking true branch

And changed:

Reallocation failed (perhaps "Testing reallocation"?)
Assuming 'tmp' is NULL
Taking true branch

That is, letting custom visitors emit messages first means that the messages themselves come second. ("Taking X branch" will always come last, cause it's part of the BugReporter itself.) In this case, it makes sense, because once you decide the reallocation failed, 'tmp' being NULL is no longer a choice.

Since we don't have any existing behavior here, we can decide what standard to set. I think saying "custom notes will follow standard notes" is okay -- notes that come /before/ standard notes would describe what we're /about/ to do, but the convention of all the standalone notes is that they describe results. ("Object allocated here", "Taking true branch", etc.)

Of course, if we decide we want /flexibility/ here...then the problem gets a lot harder.

Thoughts?
Jordy