[cfe-dev] False positive for -Wunreachable-code
David Blaikie
dblaikie at gmail.com
Sun Oct 28 10:04:13 PDT 2012
On Sun, Oct 28, 2012 at 6:49 AM, Abramo Bagnara
<abramo.bagnara at bugseng.com> wrote:
> Il 28/10/2012 13:27, Matthieu Monrocq ha scritto:
>>
>>
>> On Sun, Oct 28, 2012 at 12:08 PM, Abramo Bagnara
>> <abramo.bagnara at bugseng.com <mailto:abramo.bagnara at bugseng.com>> wrote:
>>
>> Il 28/10/2012 11:47, Matthieu Monrocq ha scritto:
>> >
>> >
>> > On Sun, Oct 28, 2012 at 8:12 AM, Abramo Bagnara
>> > <abramo.bagnara at bugseng.com <mailto:abramo.bagnara at bugseng.com>
>> <mailto:abramo.bagnara at bugseng.com
>> <mailto:abramo.bagnara at bugseng.com>>> wrote:
>> >
>> > $ cat p.c
>> > #include <stdio.h>
>> >
>> > enum e { a, b = 4 } x = 3;
>> >
>> > void g(int v) {
>> > printf("%d\n", v);
>> > }
>> >
>> > int main(int argc, char **argv) {
>> > switch (x) {
>> > case a:
>> > g(0);
>> > break;
>> > case b:
>> > g(1);
>> > break;
>> > default:
>> > g(2);
>> > break;
>> > }
>> > }
>> > $ _clang -Wunreachable-code -Wcovered-switch-default -O2 p.c
>> > p.c:17:3: warning: default label in switch which covers all
>> enumeration
>> > values
>> > [-Wcovered-switch-default]
>> > default:
>> > ^
>> > p.c:18:7: warning: will never be executed [-Wunreachable-code]
>> > g(2);
>> > ^
>> > $ ./a.out
>> > 2
>> >
>> > Of course -Wcovered-switch-default warning is a perfectly true
>> positive.
>> >
>> > My reading of the standard is that nothing prevent an enum to
>> have a
>> > value different from listed enum constants if this value is
>> compatible
>> > with enum range (and code generation seems to agree on that).
>> >
>> > Are there any objection to file a bug report?
This is, if I understand you correctly, already the subject of PR13815.
Essentially there's a huge class of -Wunreachable-code false positives
(if you think of "positive" in the case of "warns" rather than
"doesn't warn") because our CFG is used for both "warn about these
constructs only if they're reachable" and "warn about unreachable
code". The problem is that this buckets all blocks into one of those
two categories, when in fact there's a 3rd middle ground of "code that
we're not sure is reachable or unreachable".
for example code like this:
if (sizeof(int) == 4) {
...
} else if (sizeof(int) == 8) {
...
} ...
warns always on one of those two blocks, but we really don't want to
warn on either, yet we only want to emit the warnings related to
reachable code constructs (like dereferencing null, for example - but
see any other uses of DiagnoseRuntimeBehavior in Clang) on code we're
fairly sure is reachable.
The same goes for the default case in a covered switch. To reduce
false positives for runtime-behavior warnings we probably want to
consider it unreachable. But to reduce false positives in the
unreachable-code warning, we probably want to consider it reachable
(or at least "maybe reachable").
It requires modifying the CFG to represent this 3rd intermediate state
& I haven't got around to doing that yet. (once we have that mechanism
we'll then have to start doing more things to detect what kind of edge
classification each CFG edge will get (to detect things like "depends
on sizeof" and "depends on enums having only enumerate values", etc)
to remove all those -Wunreachable-code false positives)
- David
>> >
>> >
>> > Well, I would object on the basis that for all "regular" uses of the
>> > switch/enum combination: ie, the enum values are only the enumerators
>> > and the switch does cover all enumerators; then this warning is
>> > perfectly valid.
>> >
>> > Therefore I would rather say that people who use enums for
>> bit-masks and
>> > thus have variables of the enum type with values other than the
>> > enumerators (which is fine) should either not be using a switch on
>> this
>> > enum OR simply disable this warning. Maybe casting `x` to `int`
>> such as
>> > `switch(int(x))` would also work.
>> >
>> > How do you propose to distinguish the two widely different usages
>> of enum ?
>> >
>> > I personally would be rather disappointed to see
>> > -Wcovered-switch-default disappear since at both the company I work on
>> > and in my pet programs we only ever use enum for its enumerators and
>> > thus this warning is very useful.
>>
>> There is a misunderstanding here:
>>
>> - I think that -Wcovered-switch-default is very useful and the warning
>> show above is a *TRUE* positive, so there is nothing to fix there
>>
>> - my point is that -Wunreachable-code is showing a false negative for
>> the example above and should be fixed
>>
>>
>> I am afraid I did not expressed myself clearly either: if we accept that
>> the switch is perfectly covered (and thus warn that the default is not
>> needed), then it falls out that the code in the default branch is
>> unreachable.
>
> The fact that the switch is "perfectly" covered means only that every
> enum constant listed is handled in case statements. This says nothing
> about the fact that the default might be needed for valid values in the
> enum range but not present in enum constant list.
>
> Note that the default branch in the example above is perfectly reachable
> and is subject to code generation (it cannot be optimized away and it is
> not optimized away with clang or gcc or other compilers).
>
>> I am fine to remove the `-Wunreachable` warning in the case were the
>> `-Wcovered-switch-default` one has already been emitted, they are redundant.
>
> I'm inclined to disagree, these two warning show very different infos:
>
> -Wunreachable: this code is never executeed and might be removed from
> code generation
>
> -Wcovered-switch-default: the default might be needless if the enum
> typed switch guard is expected to not have any value different from enum
> constants listed in the enum definition
>
> Of course there are marginal cases where both condition are true (e.g.
> an enum with unsigned char underlying type and all enum constants with
> value from 0 to 255 are represented in case statements, but this is not
> really relevant.
>
>> The real question is: when Clang does not warn about the switch already
>> being covered, should it warn about the code being unreachable ?
>>
>> What do you think of a note suggesting a cast to the underlying integral
>> type of `x` such as `switch(int(x))` added to `-Wunreachable` to show
>> how to silence it ? (and making it so that it does silence it)
>
> This would not help, the default is reachable and subject to code
> generation with and without int cast.
>
>
> --
> Abramo Bagnara
>
> BUGSENG srl - http://bugseng.com
> mailto:abramo.bagnara at bugseng.com
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
More information about the cfe-dev
mailing list