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

[Jordan Rose]

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


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