Created attachment 52807 [details] 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 |
Created attachment 52808 [details] test case without an enum A slightly amended test case, showing that the enum isn't per-se the issue (same with constant ints).
...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!
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)
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.
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 = §ors[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.
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.
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>
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.
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.