[Bug c/87983] New: Feature: Add a warning when case labels from a different enum than the one in switch(EXPR) are used
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87983
Bug ID: 87983
Summary: Feature: Add a warning when case labels from a
different enum than the one in switch(EXPR) are used
Product: gcc
Version: unknown
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: c
Assignee: unassigned at gcc dot gnu.org
Reporter: avarab at gmail dot com
Target Milestone: ---
A bug was fixed in git that would have been spotted by the following program
warning:
```
#include
enum { A, B } foo = A;
enum { C, D } bar = C;
int main(void)
{
switch (foo) {
case C: /* Should warn: switch() on C instead of A */
puts("A");
break;
case B:
puts("B");
break;
}
}
```
I don't know how hard it would be to implement this. I understand why it's not
warning, in C enums are only skin-deep, so the compiler would need to keep
track of "foo" and the name (not just value) of C and B, and it wouldn't work
in the more general case of:
```
switch (some_complex_function(foo)) [...]
```
[Bug c/87983] Feature: Add a warning when case labels from a different enum than the one in switch(EXPR) are used
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87983 --- Comment #1 from Ævar Arnfjörð Bjarmason --- FYI: I filed a bug with clang for the same feature: https://bugs.llvm.org/show_bug.cgi?id=39635
gcc-bugs@gcc.gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264 Bug ID: 105264 Summary: -Wanalyzer-use-of-uninitialized-value gets confused about var + i v.s. &var[i] 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: --- After reading https://developers.redhat.com/articles/2022/04/12/state-static-analysis-gcc-12-compiler I tried out -fanalyzer on GCC 12 (built from c1ff207af66 (ppc: testsuite: skip pr60203 on no ldbl128, 2022-04-12)) against git.git, and discover what seems to be a bug. When compiling git (https://github.com/git/git/) as: $ make CC=gcc CFLAGS=-fanalyzer builtin/merge-file.o It will complain about: builtin/merge-file.c:86:28: error: use of uninitialized value ‘mmfs[i].size’ [CWE-457] [-Werror=analyzer-use-of-uninitialized-value] 86 | if (mmfs[i].size > MAX_XDIFF_SIZE || [...] The basic control flow is: mmfile_t mmfs[3]; [...] for-loop [...] ret = read_mmfile(mmfs + i, fname); [...] Where read_mmfile() function is always either returning -1 or populating mmfs[i] structure, in the case of -1 we can't reach the code -fanalyzer raises an issue about. The warning will go away if I apply: diff --git a/builtin/merge-file.c b/builtin/merge-file.c index e695867ee54..0ca3580b27d 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -77,7 +77,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) names[i] = argv[i]; fname = prefix_filename(prefix, argv[i]); - ret = read_mmfile(mmfs + i, fname); + ret = read_mmfile(&mmfs[i], fname); free(fname); if (ret) return -1; Which to me suggests a bug in the analyzer, that's not the most obvious code in the world and probably could use that patch in any case, but the analyzer should understand that mmfs+i and &mmfs[i] yield the same pointer. analyzer-use-of-uninitialized-value
[Bug c++/87404] Implement -Wenum-compare and -Wenum-compare-switch
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87404 --- Comment #8 from Ævar Arnfjörð Bjarmason --- Eric: I filed bug 87983. I think it makes sense to mark it as a duplicate only if this one covers both C and C++, right now the "component" for this one is C++. As bug 87983 notes in passing C does not have first-class enums, so implementing this will presumably be a bit harder than C++, and .
gcc-bugs@gcc.gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264 --- Comment #2 from Ævar Arnfjörð Bjarmason --- I think I can do one better. Here's a stand-alone reproducible test case without any headers except standard headers, I've expanded the gcc -E version of that too, but presumably you won't need it. It's screaming bloddy murder about various other things, since I did a rather bad hatchet job of copy/pasting various structs I needed from local headers into the testcase, and deleted some code I could do away with, but as noted in the now-added comment in the code: 1. "mmfs + i" to "&mmfs[i]" makes it go away, so if it's a loop-iterator problem wouldn't that apply to both? 2. I tried copying the relevant function into the file and that makes it go away too, so some cross-file analysis limitation? Will follow-up and attach both the testcase & the gcc -E version, tested with: gcc -o testcase -fanalyzer testcase.c
gcc-bugs@gcc.gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264 Ævar Arnfjörð Bjarmason changed: What|Removed |Added CC||avarab at gmail dot com --- Comment #3 from Ævar Arnfjörð Bjarmason --- Created attachment 52805 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52805&action=edit testcase.c A stand-alone testcase of builtin/merge-file.c from git.git
gcc-bugs@gcc.gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264 --- Comment #4 from Ævar Arnfjörð Bjarmason --- Created attachment 52806 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52806&action=edit testcase-full.c (gcc -E of testcase.c) The gcc -E version of testcase.c, probably useless since it only uses headers from glibc, but just in case...
[Bug analyzer/105273] New: -Wanalyzer-use-of-uninitialized-value warns on "missing" default for switch when callers can be statically determined
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105273 --- Comment #1 from Ævar Arnfjörð Bjarmason --- 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105273 --- Comment #2 from Ævar Arnfjörð Bjarmason --- ...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 c++/87404] Implement -Wenum-compare and -Wenum-compare-switch
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87404 --- Comment #10 from Ævar Arnfjörð Bjarmason --- Thanks, just clarifying. I saw this one was in the C++ component unlike the other one. On Thu, Apr 14, 2022, 19:57 egallager at gcc dot gnu.org < gcc-bugzi...@gcc.gnu.org> wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87404 > > --- Comment #9 from Eric Gallager --- > (In reply to Ævar Arnfjörð Bjarmason from comment #8) > > Eric: I filed bug 87983. I think it makes sense to mark it as a duplicate > > only if this one covers both C and C++, right now the "component" for > this > > one is C++. > > > > As bug 87983 notes in passing C does not have first-class enums, so > > implementing this will presumably be a bit harder than C++, and . > > My understanding was that this bug would cover both C and C++, as I'm > pretty > sure I've seen this clang warning in plain-C code before, too (and there's > no > component for the code shared by the C family of languages, which is where > I'd > assume this would be implemented) > > -- > You are receiving this mail because: > You are on the CC list for the bug.
[Bug analyzer/105285] New: False positive with -Wanalyzer-null-dereference in git.git's reftable/reader.c
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105285
Bug ID: 105285
Summary: False positive with -Wanalyzer-null-dereference in
git.git's reftable/reader.c
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 52813
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52813&action=edit
gcc -E of reftable/reader.c
I didn't have time to come up with a nice isolated test case this time for
$subject, sorry, but (maybe this is easier):
git clone https://github.com/git/git/
cd git
make CC=gcc CFLAGS=-fanalyzer reftable/reader.o
Or alternatively the gcc -E output that's attached, but most informative is the
patch I'll attach to git.git to work around this (as the code was pretty
nonsensical anyway).
Why do I think it's a false positive? It's code that's basically doing:
struct x = { 0 }; /* "foo" member is NULL */
if (x >= 0)
return 0; /* early abort */
[...]
if (x >= 0)
return 1; /* we don't init "x.foo" */
x.foo
I.e. the analyzer thinks you can go through these two and for "x >= 0" to be
true the first time, and false the next, or the other way around.
In this case the types didn't have "const" on them, but to make sure these
*really* weren't being changed I added that in all the relevant places, but it
still complained. The warning was:
reftable/reader.c: In function ‘extract_block_size’:
reftable/reader.c:274:20: warning: dereference of NULL ‘data’ [CWE-476]
[-Wanalyzer-null-dereference]
274 | *typ = data[0];
|^~~
Also, for the GCC 11.2 I have locally (I tested the warning on near-trunk GCC
12.0) I got two different warnings from -fanalyzer, so this seems to be an area
that's seen active changes recently...
[Bug analyzer/105285] False positive with -Wanalyzer-null-dereference in git.git's reftable/reader.c
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105285
--- Comment #1 from Ævar Arnfjörð Bjarmason ---
Created attachment 52814
--> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52814&action=edit
A patch to git.git that works around the -fanalyzer false positive
A fix to git.git to work around the analyzer false positive, this diff on top
shows an annotated path through the program it thinks we'd take. 24 and 28
can't be true/false false/true, only false/false true/true.
diff --git a/reftable/reader.c b/reftable/reader.c
index ea66771165f..d3a4639d6ca 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -61,6 +61,9 @@ static int reader_get_block(struct reftable_block *dest,
uint32_t sz, const uint64_t off,
const uint64_t r_size)
{
+ if (off >= r_size) /* (28) following ‘true’ branch (when ‘off >=
r_size’)... */
+ return 0;
+
if (off + sz > r_size) {
sz = r_size - off;
}
@@ -288,13 +291,14 @@ int reader_init_block_reader(struct reftable_reader *r,
struct block_reader *br,
uint32_t header_off = next_off ? 0 : header_size(r->version);
int32_t block_size = 0;
- if (next_off >= r_size)
+ if (next_off >= r_size) /* (24) following ‘false’ branch (when
‘next_off < r_size’)... */
return 1;
err = reader_get_block(&block, &r->source, next_off, guess_block_size,
r_size);
- if (err < 0)
+ if (err < 0) /* (31) following ‘false’ branch (when ‘err >= 0’)... */
goto done;
+ /* We'll supposedly deference NULL block.data[0] here ... */
block_size = extract_block_size(block.data, &block_typ, next_off,
r->version);
if (block_size < 0) {
[Bug analyzer/105285] False positive with -Wanalyzer-null-dereference in git.git's reftable/reader.c
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105285 --- Comment #2 from Ævar Arnfjörð Bjarmason --- This code also errors under -Werror=analyzer-too-complex, including in some adjacent code, so perhaps the analyzer gave up?
gcc-bugs@gcc.gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264 --- Comment #9 from Ævar Arnfjörð Bjarmason --- Thanks a lot, I can confirm that this fixes the issue in builtin/merge-file.c in git.git.
[Bug c/87983] Feature: Add a warning when case labels from a different enum than the one in switch(EXPR) are used
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87983
--- Comment #4 from Ævar Arnfjörð Bjarmason ---
(In reply to Eric Gallager from comment #3)
> Is the expectation that this would come from -Wswitch, -Wswitch-enum,
> -Wenum-compare, -Wenum-conversion, or some new flag?
I think a new flag would be best. The clang compiler has a C++-only
-Wenum-compare-switch flag which will warn about this.
Here's a better and updated testcase showing how it works:
```
#include
enum enum_x { A, B };
enum enum_y { C, D };
int main(void)
{
enum enum_y y = C;
switch (y) {
case A: /* Should warn: switch() on C instead of A */
puts("A");
break;
case D:
puts("B");
break;
}
}
```
And a one-liner showing how gcc, g++, clang and clang++ handle it:
```
$ for cc in gcc g++ clang clang++; do echo $cc: && $cc -Wswitch -Wswitch-enum
-Wenum-compare -o test test.c; ./test; done
gcc:
A
g++:
A
clang:
A
clang++:
clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is
deprecated [-Wdeprecated]
test.c:10:12: warning: comparison of different enumeration types in switch
statement ('enum enum_y' and 'enum_x') [-Wenum-compare-switch]
case A: /* Should warn: switch() on C instead of A */
^
1 warning generated.
A
```
(This is with a local GCC 10.* and LLVM 13.*, on a Debian box). Documentation
for the Clang warning:
https://clang.llvm.org/docs/DiagnosticsReference.html#wenum-compare-switch
