r179736 - [analyzer] Tweak getDerefExpr more to track DeclRefExprs to references.

[Jordan Rose]

On Apr 17, 2013, at 18:35 , David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, Apr 17, 2013 at 6:14 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>> On Apr 17, 2013, at 17:15 , Anna Zaks <ganna at apple.com> wrote:
>> 
>> +  // This is not valid C++; dynamic_cast with a reference type will throw
>> an
>> +  // exception if the pointer does not match the expected type. However,
>> our
>> +  // implementation of dynamic_cast will pass through a null pointer...or a
>> +  // "null reference"! So this branch is actually possible.
>> +  if (&val == 0) { //expected-note {{Assuming pointer value is null}}
>> 
>> 
>> I know this is just a test, but the comment is bogus. Instead it's something
>> like this:
>> 
>> // This is not valid C++; if 'ptr' were null, creating 'ref' would be
>> illegal.
>> // However, this is not checked at runtime, so this branch is actually
>> possible.
> 
> Shouldn't the SA just be diagnosing the initialization of the
> reference 'val' as problematic, rather than the later use? The code is
> UB because of that reference initialization.

Yes and no. We actually tried forbidding the creation of null lvalues for a while, but it turned out that not only does that seem to be legal, people actually use it to implement a homegrown 'offsetof'.

The next problem here is that we don't actually find out that the pointer is null until the if-case. At this point the reference has already been defined, and whatever path we've taken to get to the purported null dereference might have relied on using the reference. Even if we realized there was a problem, it could be difficult to report it in a sensible way.

What we could do is check the reference at initialization time in UndefinedAssignmentChecker, like we do for return values (ReturnUndefChecker) and function parameters (CallAndMessageChecker). At this point, if we can't prove a pointer is null (to warn about it), we'd just assume it's non-null.

We've discussed this before, and I think we decided against it because people do make this mistake, and strictly following the C++ standard here would cause us not to warn about it at all. (Most compilers do not insert runtime checks for this undefined behavior.) The ideal case would be to make the assumption at bind time, and then show that the branch is dead, but the analyzer doesn't currently have a good way to assert that something is true on all paths, rather than just some path...which is critical for dead-code checking.

Anyway, yes, it's less than ideal, but until we reach the better solution we should do a good job with what we have.
Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130417/e381f12a/attachment.html>