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

[Anna Zaks]

On Mar 19, 2012, at 12:38 AM, Jordan Rose wrote:

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

I don't think setting which generic visitors should be used when creating a BugReport is a hack. (I don't suggest that we add a generic visitor while visiting the path with a custom visitor.) Anyway, this probably goes into the future work category. For example, we could add another generic visitor that adds a note whenever underflow or overflow occurs or another one that shows the ranges of interesting symbols. These additional diagnostics would be valuable for buffer overflow checking but not for null dereference.

>> 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
> 
(The first sequence is what we have now, and the second one is what will happen after reordering?)

I think it's important to say "Reallocation failed". The error only occurs when reallocation failed, not if we just test for failure. And it's best to have temporal order here. We assume 'tmp' is NULL, which implies that reallocation failed. (On the other hand, "Reallocation failed" is a more important message to display, but we could use other means to specify that.)

> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120319/c8cbb12d/attachment.html>