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

[David Blaikie]

On Wed, Apr 17, 2013 at 6:43 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> 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,

The specifics are a bit tricky here, but my understand is the
dereference itself is not UB, but initializing the reference is. (at
least that's how UBSan has been implemented & I'll take Richard's word
for it - though I think the wording got better/more clear here in
C++11, too)

> people
> actually use it to implement a homegrown 'offsetof'.

I don't know if that falls afoul of the above UB - I suspect it
doesn't, but just by a narrow margin. So a correct checker would not
fail on this while still failing on the reference binding.

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

Really? That seems strange. There's no constant condition evaluation?
(even things as simple as "if (false)" - which this should be
equivalent to)

> Anyway, yes, it's less than ideal, but until we reach the better solution we
> should do a good job with what we have.

Okey dokey.