[cfe-dev] False positive for -Wunreachable-code

[Abramo Bagnara]

Il 28/10/2012 18:04, David Blaikie ha scritto:
> 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.

Yes, I confirm that.

> 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.

In my mind things are very simple: unreachable code is code that cannot
be executed and can be omitted from code generation.

So the default statement in my testcase above is not unreachable code
(as it is code reachable and actually executed).

To emit a warning about unreachable code for that is grossly misleading.

Now let consider your testcase about sizeof: there is no doubt that at
least one conditional branch is unreachable code.

There is the possibility that despite the fact it is definitely true
that this code is unreachable the programmers get annoyed to see such
warnings when he knows that the code is unreachable by design.

Then under a quality of implementation perspective we might choose to
have a system to silent that, but this has nothing to do with
"unreachable code" concept and it is on an orthogonal plane.

> 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

This phrase amazes me, can you make an example where considering a
reachable default as unreachable reduces the false positives of some
other warnings?

> 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)

I don't think this is needed, IMHO we should only have a CFG concept of
unreachable code congruent with the one used for code generation.

-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara at bugseng.com