>> +void check(int *p) {
>> + if (p) {
>> + // expected-note@-1 + {{Assuming 'p' is null}}
>> + // expected-note@-2 + {{Assuming pointer value is null}}
>> + // expected-note@-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@-1 {{Calling 'check'}}
>> + // expected-note@-2 {{Returning from 'check'}}
>> + *a = 1; // expected-warning{{Dereference of null pointer}}
>> + // expected-note@-1 {{Dereference of null pointer (loaded from variable
>> 'a')}}
>> +}
>> +
>> +
>> +int *getPointer();
>> +
>> +void testInitCheck() {
>> + int *a = getPointer();
>> + // expected-note@-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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
