[cfe-commits] r161280 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/BugReporter/ lib/StaticAnalyzer/Checkers/ lib/StaticAnalyzer/Core/ test/Analysis/ test/Analysis/inlining/

[Anna Zaks]

On Aug 4, 2012, at 8:31 PM, Jordan Rose wrote:

> 
>>> +void check(int *p) {
>>> +  if (p) {
>>> +    // expected-note at -1 + {{Assuming 'p' is null}}
>>> +    // expected-note at -2 + {{Assuming pointer value is null}}
>>> +    // expected-note at -3 + {{Taking false branch}}
>> 
>> A lot of redundant diagnostics here.
> 
> Yep; that's what the FIXME in r161279 is about, though it looks like VisitTrueTest is really where the problem lies. I split the commits incorrectly, sorry.
> 
> Stepping back, the question is which of these is necessary. The first and third come from the generic condition visitor that shows branches taken based on the AST. The second comes from the constraint tracker for the 'p' region, which might want to make a better effort to use a real region name (rather than "pointer value").
> 
> You get weird results mixing these, not just because the messages are duplicated, but because the name of the region is probably the name from the caller, and the DeclRefExpr refers to the name in the callee.
> 
> We've always printed two diagnostics here whenever we consider the condition variable to be "interesting", so maybe we can ease off on the constraint tracker rather than the condition visitor. I'll look into it on Monday.
> 
> 
>>> +    return;
>>> +  }
>>> +  return;
>>> +}
>>> +
>>> +void testCheck(int *a) {
>>> +  check(a);
>>> +  // expected-note at -1 {{Calling 'check'}}
>>> +  // expected-note at -2 {{Returning from 'check'}}
>>> +  *a = 1; // expected-warning{{Dereference of null pointer}}
>>> +  // expected-note at -1 {{Dereference of null pointer (loaded from variable 'a')}}
>>> +}
>>> +
>>> +
>>> +int *getPointer();
>>> +
>>> +void testInitCheck() {
>>> +  int *a = getPointer();
>>> +  // expected-note at -1 {{Variable 'a' initialized here}}
>> 
>> I am not convinced that the extra info is necessary for explaining this bug:
>> 1) a is initialized, 
>> 2) it's assigned to 0
>> 3) a is dereferenced
>> 
>> #1 seems to be not important in this example (and the next).
>> 
>> I general, we want to keep the output to the minimum and only include diagnostics needed to understand the bug.
> 
> In this case 'a' is not /assigned/ 0, but /constrained/ to 0. I think that makes it very valuable to see where the value came from, in case the constraint was accidentally violated earlier. But I'm not tied to this.

Sounds good. This would help users to identify false positives in case 'a' cannot be 0 based on the way it was initialized. I recall that someone on the mailing list once said that the null check warnings could be annoying since they often come from the fact that someone added a redundant check for null, and not because the pointer can actually be null.

> 
> 
>> We would love to show where a variable was NOT initialized, but that's not easy/possible :) 
> 
> We do, in fact, do this, at least for local variables. I'm not sure if we do anything for things like malloc regions, but we should. It wouldn't be that hard because for cases /other/ than local variables, we have an actual binding of Undefined.

> 
>>> Modified: cfe/trunk/test/Analysis/method-call-path-notes.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/method-call-path-notes.cpp?rev=161280&r1=161279&r2=161280&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/test/Analysis/method-call-path-notes.cpp (original)
>>> +++ cfe/trunk/test/Analysis/method-call-path-notes.cpp Fri Aug  3 18:09:01 2012
>>> @@ -24,7 +24,7 @@
>>> }
>>> 
>>> void test_ic_null(TestInstanceCall *p) {
>>> -  if (!p) // expected-note {{Taking true branch}}
>>> +  if (!p) // expected-note {{Assuming pointer value is null}} expected-note {{Taking true branch}}
>> Is the goal to show both notes in the output? The new one is more useful, but it makes the second one redundant. Maybe we could suppress the other one if the better diagnostic is available?
> 
> This is arguable; see above.

I see. This is consistent with what we had before.

Here, one of the diagnostics corresponds to an arrow in the plist output, so it will not be as verbose in the GUI which does support arrows. Also, even though in this particular case 2 diagnostics are too much, if the condition is more complex (consists of several assumptions), we would want the "Taking true branch" diagnostic. Which means that if we wanted to limit the number of diagnostics here, we would have to come up with heuristics on when to keep both and when to drop one. Based on all this, I am guessing it's not a priority for now.