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

[aaronpuchert]

Bugzilla Link	46991
Version	trunk
OS	All
CC	@devincoughlin,@Szelethus,@haoNoQ

Extended Description

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.

[aaronpuchert]

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

[haoNoQ]

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.

[aaronpuchert]

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.

[aaronpuchert]

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.

[haoNoQ]

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.

[aaronpuchert]

assigned to @haoNoQ