[cfe-commits] r145575 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngine.cpp test/Analysis/misc-ps-region-store.cpp

[Ted Kremenek]

The idea is not that we assume that 'this' can never be null, but I wanted to treat that as a baseline assumption (for avoiding false positives) when we are just analyzing a method in isolation.  We were getting weird false positives otherwise because we're being overly pessimistic about code.

Good point about RTTI.  I'm fine with changing the test, although this test reflects more the actual false positive we saw.  Maybe two tests? (one that does what it does now, and the other as you suggest?)

On Dec 10, 2011, at 9:05 PM, Jordy Rose wrote:

> Hi, Ted. Just wanted to note that it's technically legal for 'this' to be null if the method is non-virtual, which I'm sure you know already. Maybe someday we can make this a setting?
> 
> I'm also confused about the test. I thought that testing 'p' was supposed to bifurcate the state for 'this', but only the "non-null" state would be vaild. That already feels hacky to me, relying on the analyzer treating the test of a member as a bifurcation point for the base.
> 
> Then I glanced at the C++ standard and realized that &field may not be defined. It's short for &(this->field), right? &(this->field) could be compiled to (this+offsetof(field)), and if offsetof(field) is nonzero (say, because of RTTI info being inserted), the test could actually come out true even if 'this' is null.
> 
> I'm not saying Clang /will/ do this, but it'd be nice to have a test that depended less on the compiler settings. I'd prefer making this a deadcode test: "if (!this) doSomethingStupid();" or something. 
> 
> (I realize this is at odds with what I pointed out first about 'this' possibly being 'null', but this will notice if we ever put that behavior under a flag, anyway. I feel like negative tests can silently break more easily than positive tests.)
> 
> Sorry to get to this one a week later,
> Jordy
> 
> 
> On Dec 1, 2011, at 12:29, Ted Kremenek wrote:
> 
>> Modified: cfe/trunk/test/Analysis/misc-ps-region-store.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/misc-ps-region-store.cpp?rev=145575&r1=145574&r2=145575&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/misc-ps-region-store.cpp (original)
>> +++ cfe/trunk/test/Analysis/misc-ps-region-store.cpp Wed Nov 30 23:29:42 2011
>> @@ -492,3 +492,17 @@
>>  return NaN;
>> }
>> 
>> +// Test that 'this' is assumed null upon analyzing the entry to a "top-level"
>> +// function (i.e., when not analyzing from a specific caller).
>> +struct TestNullThis {
>> +  int field;
>> +  void test();
>> +};
>> +
>> +void TestNullThis::test() {
>> +  int *p = &field;
>> +  if (p)
>> +    return;
>> +  field = 2; // no-warning
>> +}
>> +
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>