[Bug analyzer/108028] Misleading -fanalyzer messages at -O2 and above

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108028

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|--Wanalyzer-null-dereferenc |Misleading -fanalyzer
                   |e false positive with *q =  |messages at -O2 and above
                   |1                           |

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this bug.

There are several things going on here.

(A): the analyzer is considering the function "f" as called standalone, as well
as the case where it's called from "main", rather than merely considering it
when it's called from "main".  There are a few other bug reports about that;
it's not clear to me what we should do for this case; is it expected that such
functions are only ever called from main?

The situation is clearer if we simply delete "main" from the reproducer.  With
that, we see:

  'f': events 1-3
    |
    |    7 |     if (p && (0 == q))
    |      |        ^
    |      |        |
    |      |        (1) following 'true' branch...
    |    8 |     {
    |    9 |         __analyzer_eval(p && (0 == q));
    |      |         ~~~~~~~~~~~~~~~
    |      |         |
    |      |         (2) ...to here
    |......
    |   14 |         *q = 1;
    |      |         ~~~~~~
    |      |            |
    |      |            (3) dereference of NULL '0'
    |


(B) arguably the CFG event (1) to (2) is poorly worded; at (1) we have
"following 'true' branch...", which suggests it always follows the 'true'
branch, whereas it's merely considering what happens *if* we take the 'true'
branch.

If it read: e.g.:

  'f': events 1-3
    |
    |    7 |     if (p && (0 == q))
    |      |        ^
    |      |        |
    |      |        (1) considering following 'true' branch (implying that 'q'
is NULL)...
    |    8 |     {
    |    9 |         __analyzer_eval(p && (0 == q));
    |      |         ~~~~~~~~~~~~~~~
    |      |         |
    |      |         (2) ...to here
    |......
    |   14 |         *q = 1;
    |      |         ~~~~~~
    |      |            |
    |      |            (3) dereference of NULL '0'
    |

the analyzer would be more obviously correct and useful.

Or even "considering when 'q' is NULL; following 'true' branch..."

I've been experimenting with making the wording here clearer
(i): should make a distinction between when the analyzer chooses one of several
paths to consider, versus when the choice of execution path is already
determined by previous choices
(ii): would be nice to capture that q's nullness is the most interesting
property at the "if" statement with respect to the warning, and express that to
the user.


(C) The analyzer runs relatively late compared to most static analyzers, so the
optimization levels affect things.  In particular, consider the gimple IR seen
by -fanalyzer for the assignment:
     *q = 1;
in the block guarded by (0 == q).

At -O1 we have:
     *q_10(D) = 1;
but at -O2 it converts it to:
     MEM[(int *)0B] = 1;

Arguably it's a bug here that we only warn at -O2 and above; analyzing "f"
standalone, we ought to be complaining about the null dereference without
needing -O2.
(specifically, at -O2 -fanalyzer sees a deref of NULL, whereas at -O1 it merely
sees a deref of INIT_VAL(q), whilst knowing the constraint that INIT_VAL(q) is
NULL.
The __analyzer_eval results are also due to gimple IR differences caused by the
optimizer.

Also, perhaps we should run earlier; I probably ought to file a bug about that,
it's a big can of worms.