[Bug analyzer/105273] New: -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[Bug analyzer/105273] New: -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[avarab at gmail dot com]

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

            Bug ID: 105273
           Summary: -Wanalyzer-use-of-uninitialized-value warns on
                    "missing" default for switch when callers can be
                    statically determined
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: avarab at gmail dot com
  Target Milestone: ---

Created attachment 52807
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52807&action=edit
test case with an enum

With GCC 12.0 built from c1ff207af66 (ppc: testsuite: skip pr60203 on no
ldbl128, 2022-04-12) I get a warning on the attached test case:

 gcc -fanalyzer analyze.c
analyze.c: In function ‘vreportf’:
analyze.c:28:17: warning: use of uninitialized value ‘pfx’ [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]
   28 |                 snprintf(buf, sizeof(buf), "%s", pfx);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘vreportf’: events 1-6
    |
    |   13 |         const char *pfx;
    |      |                     ^~~
    |      |                     |
    |      |                     (1) region created on stack here
    |   14 | 
    |   15 |         switch (kind) {
    |      |         ~~~~~~       
    |      |         |
    |      |         (2) following ‘default:’ branch...
    |......
    |   25 |         if (kind == USAGE_BUG)
    |      |            ~         
    |      |            |
    |      |            (3) ...to here
    |      |            (4) following ‘false’ branch (when ‘kind != 1’)...
    |......
    |   28 |                 snprintf(buf, sizeof(buf), "%s", pfx);
    |      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                 |
    |      |                 (5) ...to here
    |      |                 (6) use of uninitialized value ‘pfx’ here
    |

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[avarab at gmail dot com]

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

--- Comment #1 from Ævar Arnfjörð Bjarmason <avarab at gmail dot com> ---
Created attachment 52808
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52808&action=edit
test case without an enum

A slightly amended test case, showing that the enum isn't per-se the issue
(same with constant ints).

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[avarab at gmail dot com]

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

--- Comment #2 from Ævar Arnfjörð Bjarmason <avarab at gmail dot com> ---
...To finish the report (Bugzilla's eager submitting threw me for a loop) the
issue is that while the analyzer is right in the *general* case about a
"switch" with a missing "default" being something that *could* be fed any
arbitrary value, in this case all of the possible values can be determined at a
compile-time.

Which is all this bug report is suggesting as an initial report, that it would
be nice to have that narrow case handled.

END OF NARROW REPORT

More generally though (and perhaps I should submit another report for this)
it's a really useful feature of GCC (and clang) that with C they put a bit of
trust in the programmer with -Wswitch (which is enabled under -Wall).

Because even though there are cases where the programmer is wrong and
exhaustively enumerating the enum labels isn't sufficient, in the general case
being able to avoid "default" cases in favor of exhaustively listing the labels
avoids a *lot* of subtle bugs in larger codebases.

That's because the values being thrown around to "switch" on are validated
already by [insert magic here], but whenever *new* values are added it's really
important to not miss 1/N switch statements that new labels need to be added
to.

In the testcase for this bug the compiler has enough visibility to determine
this to be true without the "[insert magic here]", but in cases where that's
not true it seems to me that those users -fanalyzer would be encouraged to add
"default" cases just to appease the compiler, and thus get worse warnings from
-Wswitch.

I may be missing something obvious, but it would be nice to have some way out
of that where you can have your cake & eat it too. I.e. only have -fanalyze
complain about this class of issue where -Wswitch would complain, and have the
current behavior in GCC 12.0 hidden behind some opt-in sub-flag of
-Wanalyzer-use-of-uninitialized-value.

Anyway, just my 0.02. Thanks a lot for -fanalyze, I've been trying it out on
the git codebase and it's uncovered a lot of genuine issues already. I'm just
filing some bugs for the long tail where the analyzer seemed to be
wrong/lacking. Thanks!

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[egallager at gcc dot gnu.org]

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

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
                 CC|                            |egallager at gcc dot gnu.org
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=61864

--- Comment #3 from Eric Gallager <egallager at gcc dot gnu.org> ---
For more discussion of how "default" branches of switch statements affect
diagnostics, see also bug 61864 (although that's just for regular diagnostics,
and predates the analyzer)

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[dmalcolm at gcc dot gnu.org]

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

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

IIRC in the initial GCC 10 release of the analyzer, it didn't directly explore
within static functions, and instead only explored them via callsites.  I
tweaked the policy for this in commit r11-7029-g8a2750086d57d1.

There's definitely room for improvement here, possibly with a heuristic for
switch statements, or due to fixing how the analyzer handles call summaries.

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[dmalcolm at gcc dot gnu.org]

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

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Similar thing seen in linuxdoom-1.10:

p_floor.c: In function ‘EV_BuildStairs’:
p_floor.c:503:22: warning: use of uninitialized value ‘speed’ [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]
  503 |         floor->speed = speed;
      |         ~~~~~~~~~~~~~^~~~~~~
  ‘EV_BuildStairs’: events 1-9
    |
    |  472 |     fixed_t             speed;
    |      |                         ^~~~~
    |      |                         |
    |      |                         (1) region created on stack here
    |      |                         (2) capacity: 4 bytes
    |......
    |  476 |     while ((secnum = P_FindSectorFromLineTag(line,secnum)) >= 0)
    |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                                                            |
    |      |                                                            (3)
following ‘true’ branch (when ‘secnum >= 0’)...
    |  477 |     {
    |  478 |         sec = &sectors[secnum];
    |      |               ~~~~~~~~~~~~~~~~
    |      |               |
    |      |               (4) ...to here
    |......
    |  481 |         if (sec->specialdata)
    |      |            ~             
    |      |            |
    |      |            (5) following ‘false’ branch...
    |......
    |  485 |         rtn = 1;
    |      |         ~~~~~~~          
    |      |             |
    |      |             (6) ...to here
    |......
    |  492 |         switch(type)
    |      |         ~~~~~~           
    |      |         |
    |      |         (7) following ‘default:’ branch...
    |......
    |  503 |         floor->speed = speed;
    |      |         ~~~~~~~~~~~~~~~~~~~~
    |      |                      |
    |      |                      (8) ...to here
    |      |                      (9) use of uninitialized value ‘speed’ here
    |


and also with "stairsize".

In both cases the analyzer considers the case of taking the "default" branch
at:

    |  492 |         switch(type)
    |      |         ~~~~~~           
    |      |         |
    |      |         (7) following ‘default:’ branch...


which would leave this uninitialized, where:

int
EV_BuildStairs
( line_t*       line,
  stair_e       type )

and p_spec.h has:

typedef enum
{
    build8,     // slowly build by 8
    turbo16     // quickly build by 16

} stair_e;

and the only calls to EV_BuildStairs are in other TUs:

p_spec.c: 576:  EV_BuildStairs(line,build8);
p_spec.c: 739:  EV_BuildStairs(line,turbo16);
p_switch.c: 350:        if (EV_BuildStairs(line,build8))
p_switch.c: 488:        if (EV_BuildStairs(line,turbo16))

Probably should assume that a switch on an enum takes one of the enum's values.

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[dmalcolm at gcc dot gnu.org]

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

--- Comment #6 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Another instance from Doom, this time where the enum is in a field lookup,
rather than an input parameter:

p_maputl.c: In function ‘P_BoxOnLineSide’:
p_maputl.c:151:8: warning: use of uninitialized value ‘p1’ [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]
  151 |     if (p1 == p2)
      |        ^
  ‘P_BoxOnLineSide’: events 1-5
    |
    |  115 |     int         p1;
    |      |                 ^~
    |      |                 |
    |      |                 (1) region created on stack here
    |      |                 (2) capacity: 4 bytes
    |......
    |  118 |     switch (ld->slopetype)
    |      |     ~~~~~~       
    |      |     |
    |      |     (3) following ‘default:’ branch...
    |......
    |  151 |     if (p1 == p2)
    |      |        ~         
    |      |        |
    |      |        (4) ...to here
    |      |        (5) use of uninitialized value ‘p1’ here
    |

where r_defs.h has:

//
// Move clipping aid for LineDefs.
//
typedef enum
{
    ST_HORIZONTAL,
    ST_VERTICAL,
    ST_POSITIVE,
    ST_NEGATIVE

} slopetype_t;

typedef struct line_s
{
    [...snip...]

    // To aid move clipping.
    slopetype_t slopetype;

    [...snip...]
} line_t;

and all four enum values are covered by the switch statement.

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[cvs-commit at gcc dot gnu.org]

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

--- 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:ccd4df81aa6537c3c935b026905f6e2fd839654e

commit r13-5159-gccd4df81aa6537c3c935b026905f6e2fd839654e
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Jan 13 17:51:26 2023 -0500

    analyzer: add heuristics for switch on enum type [PR105273]

    Assume that switch on an enum doesn't follow an implicit default
    skipping all cases when all enum values are covered by cases.

    Fixes various false positives from -Wanalyzer-use-of-uninitialized-value
    such as this one seen in Doom:

    p_maputl.c: In function 'P_BoxOnLineSide':
    p_maputl.c:151:8: warning: use of uninitialized value 'p1' [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]
      151 |     if (p1 == p2)
          |        ^
      'P_BoxOnLineSide': events 1-5
        |
        |  115 |     int         p1;
        |      |                 ^~
        |      |                 |
        |      |                 (1) region created on stack here
        |      |                 (2) capacity: 4 bytes
        |......
        |  118 |     switch (ld->slopetype)
        |      |     ~~~~~~
        |      |     |
        |      |     (3) following 'default:' branch...
        |......
        |  151 |     if (p1 == p2)
        |      |        ~
        |      |        |
        |      |        (4) ...to here
        |      |        (5) use of uninitialized value 'p1' here
        |

    where "ld->slopetype" is a "slopetype_t" enum, and for every value of
    that enum the switch has a case that initializes "p1".

    gcc/analyzer/ChangeLog:
            PR analyzer/105273
            * region-model.cc (has_nondefault_case_for_value_p): New.
            (has_nondefault_cases_for_all_enum_values_p): New.
            (region_model::apply_constraints_for_gswitch): Skip
            implicitly-created "default" when switching on an enum
            and all enum values have non-default cases.
            (rejected_default_case::dump_to_pp): New.
            * region-model.h (region_model_context::possibly_tainted_p): New
            decl.
            (class rejected_default_case): New.
            * sm-taint.cc (region_model_context::possibly_tainted_p): New.
            * supergraph.cc (switch_cfg_superedge::dump_label_to_pp): Dump
            when implicitly_created_default_p.
            (switch_cfg_superedge::implicitly_created_default_p): New.
            * supergraph.h
            (switch_cfg_superedge::implicitly_created_default_p): New decl.

    gcc/testsuite/ChangeLog:
            PR analyzer/105273
            * gcc.dg/analyzer/switch-enum-1.c: New test.
            * gcc.dg/analyzer/switch-enum-2.c: New test.
            * gcc.dg/analyzer/switch-enum-pr105273-git-vreportf-2.c: New test.
            * gcc.dg/analyzer/switch-enum-taint-1.c: New test.
            * gcc.dg/analyzer/switch-wrong-enum.c: New test.
            * gcc.dg/analyzer/torture/switch-enum-pr105273-doom-p_floor.c: New
            test.
            * gcc.dg/analyzer/torture/switch-enum-pr105273-doom-p_maputl.c:
            New test.
            * gcc.dg/analyzer/torture/switch-enum-pr105273-git-vreportf-1.c:
            New test.

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

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-01-13

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

I hope to backport this to GCC 12; keeping this bug open to track that.

[Bug analyzer/105273] -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined

[dmalcolm at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Unfortunately this turned out to be nontrivial to backport to gcc 12 e.g. due
to r13-575-g2c05a2d1a8e469 changing the representation of enums in the C
frontend from gcc 12 to 12.

gcc 13 has the fix; closing this out.