[Bug analyzer/107565] New: -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[Bug analyzer/107565] New: -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[eggert at cs dot ucla.edu]

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

            Bug ID: 107565
           Summary: -Wanalyzer-use-of-uninitialized-value false positive
                    with rdrand
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: eggert at cs dot ucla.edu
  Target Milestone: ---

Created attachment 53846
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53846&action=edit
test program illustrating the -fanalyzer false positive

This is gcc (GCC) 12.2.1 20220819 (Red Hat 12.2.1-2) on x86-64. Compile the
attached program with the command:

gcc -O2 -mrdrnd -fanalyzer -S t.c

GCC says "warning: use of uninitialized value ‘x’ [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]". This is a false positive, as x cannot
possibly be uninitialized. Apparently -fanalyzer is confused about how
__builtin_ia32_rdrand64_step works.

GCC 11.3.0 does not issue this diagnostic, so this is a regression.

[Bug target/107565] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|UNCONFIRMED                 |NEW
             Target|                            |x86_64
   Last reconfirmed|                            |2022-11-08
          Component|analyzer                    |target
           Assignee|dmalcolm at gcc dot gnu.org        |unassigned at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.
      else if (!fndecl_has_gimple_body_p (callee_fndecl)
               && (!(callee_fndecl_flags & (ECF_CONST | ECF_PURE)))
               && !fndecl_built_in_p (callee_fndecl))
        unknown_side_effects = true;

The last part is part of the problem I think. At least here.
Yes maybe we should have another builtin which returns a "_Complex unsigned
long long" here which is folded into for __builtin_ia32_rdrand*_step to remove
the need to the address too.

I am going to declare this one as a target issue but there might be other
builtins which are harder to do the "_Complex" trick.

[Bug target/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> Confirmed.
>       else if (!fndecl_has_gimple_body_p (callee_fndecl)
>                && (!(callee_fndecl_flags & (ECF_CONST | ECF_PURE)))
>                && !fndecl_built_in_p (callee_fndecl))
>         unknown_side_effects = true;
> 
> The last part is part of the problem I think. At least here.

I think the problem is here, in the analyzer: I think the analyzer is here
making the assumption that builtins that haven't been explicitly handled don't
have side effects (such as writing through the input pointers), which is
clearly wrong for this builtin.


> Yes maybe we should have another builtin which returns a "_Complex unsigned
> long long" here which is folded into for __builtin_ia32_rdrand*_step to
> remove the need to the address too.
> 
> I am going to declare this one as a target issue but there might be other
> builtins which are harder to do the "_Complex" trick.

Andrew: I see you've marked this as a target missed-optimization bug, but
arguably there's still an analyzer bug here.  Should we reassign this back to
the analyzer, or perhaps make a clone of the bug so that we can cover the two
aspects of this separately?

[Bug target/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3

[Bug analyzer/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
          Component|target                      |analyzer
           Assignee|unassigned at gcc dot gnu.org      |dmalcolm at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
I also think it's an analyzer problem, the && !fndecl_built_in_p
(callee_fndecl) is broken IMHO.

[Bug analyzer/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[dmalcolm at gcc dot gnu.org]

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

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Created attachment 54565
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54565&action=edit
Patch that reworks builtin handling

I've been testing this patch, but it might be too invasive at this point in GCC
13; attaching here to back it up while I try a less invasive approach.

[Bug analyzer/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:24ebc5404b88b765221b551dc5288f6d64ba3dc7

commit r13-6398-g24ebc5404b88b765221b551dc5288f6d64ba3dc7
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Mar 1 17:24:32 2023 -0500

    analyzer: fixes to side-effects for built-in functions [PR107565]

    Previously, if the analyzer saw a call to a non-pure and non-const
    built-in function that it didn't have explicit knowledge of the behavior
    of, it would fall back to assuming that the builtin could have arbitrary
    behavior, similar to a function defined outside of the current TU.

    However, this only worked for BUILTIN_NORMAL functions that matched
    gimple_builtin_call_types_compatible_p; for BUILT_IN_FRONTEND and
    BUILT_IN_MD, and for mismatched types the analyzer would erroneously
    assume that the builtin had no side-effects, leading e.g. to
    PR analyzer/107565, where the analyzer falsely reported that x
    was still uninitialized after this target-specific builtin:

      _1 = __builtin_ia32_rdrand64_step (&x);

    This patch generalizes the handling to cover all classes of builtin,
    fixing the above false positive.

    Unfortunately this patch regresses gcc.dg/analyzer/pr99716-1.c due to
    the:
      fprintf (fp, "hello");
    being optimized to:
       __builtin_fwrite ("hello", 1, (ssizetype)5, fp_6);
    and the latter has gimple_builtin_call_types_compatible_p return false,
    whereas the original call had it return true.  I'm assuming that this is
    an optimization bug, and have filed it as PR middle-end/108988.  The
    effect on the analyzer is that it fails to recognize the call to
    __builtin_fwrite and instead assumes arbitraty side-effects (including
    that it could call fclose on fp, hence the report about the leak goes
    away).

    I tried various more involved fixes with new heuristics for handling
    built-ins that aren't explicitly covered by the analyzer, but those
    fixes tended to introduce many more regressions, so I'm going with this
    simpler fix.

    gcc/analyzer/ChangeLog:
            PR analyzer/107565
            * region-model.cc (region_model::on_call_pre): Flatten logic by
            returning early.  Consolidate logic for detecting const and pure
            functions.  When considering whether an unhandled built-in
            function has side-effects, consider all kinds of builtin, rather
            than just BUILT_IN_NORMAL, and don't require
            gimple_builtin_call_types_compatible_p.

    gcc/testsuite/ChangeLog:
            PR analyzer/107565
            * gcc.dg/analyzer/builtins-pr107565.c: New test.
            * gcc.dg/analyzer/pr99716-1.c (test_2): Mark the leak as xfailing.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/107565] [12/13 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed on trunk for GCC 13 by the above patch.

Keeping this report open for now to track backporting the fix to GCC 12.

[Bug analyzer/107565] [12 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b

commit r13-6466-g56572a08ec4a0fc1a7802d3737cd7f7cc9089c4b
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Mar 3 17:59:21 2023 -0500

    analyzer: provide placeholder implementation of sprintf

    Previously, the analyzer lacked a known_function implementation of
    sprintf, and thus would handle calls to sprintf with the "anything could
    happen" fallback.

    Whilst working on PR analyzer/107565 I noticed that this was preventing
    a lot of genuine memory leaks from being reported for Doom; fixing
    thusly.

    Integration testing of the effect of the patch shows a big increase in
    true positives due to the case mentioned in Doom, and one new false
    positive (in pcre2), which I'm tracking as PR analyzer/109014.

    Comparison:
      GOOD:  67 -> 123 (+56); 10.91% -> 18.33%
       BAD: 547 -> 548 (+1)

    where the affected warnings/projects are:

      -Wanalyzer-malloc-leak:
        GOOD:  0 -> 56 (+56);  0.00% -> 41.48%
         BAD: 79
          True positives: 0 -> 56 (+56)
            (all in Doom)

      -Wanalyzer-use-of-uninitialized-value:
        GOOD: 0;  0.00%
         BAD: 80 -> 81 (+1)
          False positives:
            pcre2-10.42: 0 -> 1 (+1)

    gcc/analyzer/ChangeLog:
            * kf.cc (class kf_sprintf): New.
            (register_known_functions): Register it.

    gcc/testsuite/ChangeLog:
            * gcc.dg/analyzer/doom-d_main-IdentifyVersion.c: New test.
            * gcc.dg/analyzer/sprintf-1.c: New test.
            * gcc.dg/analyzer/sprintf-concat.c: New test.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/107565] [12 Regression] -Wanalyzer-use-of-uninitialized-value false positive with rdrand

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.