False positives in core.NullDereference on non_odr_use_constant DeclRefExpr to non-capture

[Quuxplusone]

Bugzilla Link	PR46991
Status	CONFIRMED
Importance	P normal
Reported by	Aaron Puchert (aaronpuchert@alice-dsl.net)
Reported on	2020-08-04 10:24:39 -0700
Last modified on	2020-08-08 05:15:02 -0700
Version	trunk
Hardware	All All
CC	dcoughlin@apple.com, dkszelethus@gmail.com, llvm-bugs@lists.llvm.org, noqnoqneo@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

On the following code:

void foo(int v);
constexpr int VAL = 1;
void bar()
{
    const int& val = VAL;
    []() {
        foo(val);
    }();
}

the static analyzer warns

<source>:7:13: warning: Dereference of undefined pointer value
[core.NullDereference]
        foo(val);
            ^
<source>:6:5: note: Uninitialized value stored to 'val'
    []() {
    ^~~~~~
<source>:6:5: note: Calling 'operator()'
    []() {
    ^~~~~~
<source>:7:13: note: Dereference of undefined pointer value
        foo(val);
            ^~~

I think that's wrong, val has an initializer expression. Note that the
DeclRefExpr referring to val is marked as non_odr_use_constant, and val doesn't
need to be captured. Adding an explicit capture fixes the Static Analyzer issue
but raises a regular Clang warning:

<source>:6:6: warning: lambda capture 'val' is not required to be captured for
this use [-Wunused-lambda-capture]
    [val]() {
     ^~~

or

<source>:6:7: warning: lambda capture 'val' is not required to be captured for
this use [-Wunused-lambda-capture]
    [&val]() {
     ~^~~

Using a capture-default expression, i.e. [=] or [&], does not fix the issue.

[Quuxplusone]

Interesting! This isn't a modeling problem, there's indeed no capture
necessary. It's a liveness problem: variable 'val' is marked as dead too early
and the binding gets cleaned up and that's why the variable appears
uninitialized when used (it was initialized but we forgot it by that point).

Aaron, if this reduced example is indeed representative of your original
problem, you should be able to suppress all warnings by adding a //use// of the
variable below the lambda:

    const int& val = VAL;
    []() {
        foo(val);
    }();
    (void)val;   // <== Silence a static analyzer false positive.

I'll see what i can do. Also +Kristof who seems to enjoy liveness problems
these days.

[Quuxplusone]

(In reply to Artem Dergachev from comment #1)
> Aaron, if this reduced example is indeed representative of your original
> problem, you should be able to suppress all warnings by adding a //use// of
> the variable below the lambda:
>
>     const int& val = VAL;
>     []() {
>         foo(val);
>     }();
>     (void)val;   // <== Silence a static analyzer false positive.
Thanks, that does it.

[Quuxplusone]

(In reply to Artem Dergachev from comment #1)
> It's a liveness problem: variable 'val' is marked as dead too
> early and the binding gets cleaned up and that's why the variable appears
> uninitialized when used (it was initialized but we forgot it by that point).
How would one debug such an issue? The docs mention a check debug.DumpLiveVars,
but if I try it on Godbolt, I only get this:

[ B0 (live variables at block exit) ]

[ B1 (live variables at block exit) ]

[ B2 (live variables at block exit) ]

even with (void)val, while the output for debug.DumpCFG looks reasonable.

I'm asking because I get another report now for the same check, where it's
saying "Access to field 'x' results in a dereference of a null pointer" in the
midst of a member function, x being an integer member being referenced through
implicit this (if this is null I'd expect a complaint on the call already, or
earlier member accesses). On top of that, the execution path doesn't tell me
where the nullness is coming from, so I'm somewhat suspecting a similar issue.
There is no lambda though.

[Quuxplusone]

The docs mention a check debug.DumpLiveVars, but if I try it on Godbolt, I only get this:

In your case every variable is indeed dead by the end of the block; but it's what's going on within the block that's of interest to you. Additionally, live variables analysis isn't sufficient for your cause; memory region of a variable may have different lifetime (say, a pointer to the variable may still be present even after the last use of the variable, as long as the variable is still on the stack).

I'm somewhat suspecting a similar issue. There is no lambda though

Nah, if there's no lambda that's definitely a different issue.

the execution path doesn't tell me where the nullness is coming from

That alone is bug-report-worthy.

[Quuxplusone]

(In reply to Artem Dergachev from comment #4)
> In your case every variable is indeed dead by the end of the block; but it's
> what's going on within the block that's of interest to you.

You're right, if I add a fake branch between initialization and lambda I can
observe the difference between original code an workaround.

> Nah, if there's no lambda that's definitely a different issue.

Now that I have a reproducer it's pretty clear that this is different.

> > the execution path doesn't tell me where the nullness is coming from
>
> That alone is bug-report-worthy.

Done in bug 47054.