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

[David Blaikie]

On Tue, Oct 30, 2012 at 11:25 AM, Abramo Bagnara
<abramo.bagnara at bugseng.com> wrote:
> Il 30/10/2012 19:08, Abramo Bagnara ha scritto:
>> Il 30/10/2012 18:49, David Blaikie ha scritto:
>>> On Tue, Oct 30, 2012 at 10:39 AM, Abramo Bagnara
>>> <abramo.bagnara at bugseng.com> wrote:
>>>> Il 30/10/2012 18:25, David Blaikie ha scritto:
>>>>> On Tue, Oct 30, 2012 at 10:19 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>>>>
>>>>>> On Oct 30, 2012, at 10:17 , David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>
>>>>>>> On Tue, Oct 30, 2012 at 9:25 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>>>>>>
>>>>>>>> On Oct 30, 2012, at 2:34 , Abramo Bagnara <abramo.bagnara at bugseng.com> wrote:
>>>>>>>>
>>>>>>>>> Il 28/10/2012 08:12, Abramo Bagnara ha scritto:
>>>>>>>>>> $ 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).
>>>>>>>>>
>>>>>>>>> I've attached the patch for review.
>>>>>>>>>
>>>>>>>>> The fixed testcase shows well why to hide warnings about undefined
>>>>>>>>> behaviour in code actually generated is a bad thing.
>>>>>>>>
>>>>>>>> If we do this, we're going to want this under a CFG option at the very least. The static analyzer should continue assuming that an enum input to a switch is always a valid enum constant, in order to keep our false positive rate down.
>>>>>>>
>>>>>>> Yeah, I doubt this'll be any better in the compiler proper, really.
>>>>>>> The heuristic exists to, as you rightly point out, reduce false
>>>>>>> positives & that rationale exists in the compiler as well.
>>>>>>>
>>>>>>> While, yes, it means we lose some true positives as well, that
>>>>>>> probably isn't worth the increase in false positives.
>>>>>>
>>>>>> I can see Abramo's point, however, that in a more defensive coding style the current -Wunreachable warning can easily be considered a false positive. We don't optimize out the default case in an enum-covered switch.
>>>>>
>>>>> Flagging this as unreachable code is a bug & should be fixed - but
>>>>> probably in the way I described. Treating it purely as reachable code
>>>>> & emitting our 'runtime' diagnostics for code in such situations will
>>>>> (I believe - though I haven't run numbers) increase false positive
>>>>> rates substantially.
>>>>>
>>>>> A trivial example that GCC often warns about & Clang deliberately does not:
>>>>>
>>>>> int func(enum X v) {
>>>>>   switch (v) {
>>>>>   case A: return 1;
>>>>>   case B: return 2;
>>>>>   ... // fully covered
>>>>>   case Z: return 26;
>>>>>   }
>>>>>   // GCC warns that the function may exit without a return value here,
>>>>> Clang does not
>>>>>   // the LLVM/Clang codebase has a lot of llvm_unreachables after
>>>>> fully covered/exiting
>>>>>   // switches like this to silence GCC's warning. Each of those is,
>>>>> essentially, a GCC
>>>>>   // false positive (in the sense that the code is not buggy).
>>>>> }
>>>>
>>>> :-o
>>>>
>>>> Unless I'm missing something, the code is definitely buggy and leads to
>>>> undefined behaviour in C++ entering with v = Z + 1. Note that entering
>>>> with v = Z + 1, is not per se an undefined behavior: only the missing
>>>> return causes that.
>>>>
>>>> Can we at least agree on that?
>>>
>>> Yes & no. Yes a program exhibits UB if the function is called with v =
>>> Z + 1, no the code is not (necessarily) buggy if the function is never
>>> called with such invalid values.
>>>
>>> If code is written in such a way as to not violate that invariant,
>>> then the warning is a false positive (it has not found a bug in the
>>> code). If people often write code with this invariant then the false
>>> positive rate is "high" and the true positive rate is "not so high",
>>> so we try to avoid warning & producing more noise than good advice.
>>> (it's obviously not a 1:1 ratio, and it's certainly a judgement call)
>>>
>>>> If we'd agree on that we will easily discover that my proposed change
>>>> does not lead to any false positive diagnostics, that GCC is right and
>>>
>>> Your definition of "false positive" differs from mine/ours. Hopefully
>>> my description above helps describe the motivation here.
>>
>> Yes, my definition of false positive is different, see:
>>
>> http://en.wikipedia.org/wiki/Type_I_and_type_II_errors#False_positive_error
>>
>> "is the default unreachable"? ("is there a wolf near the herd?")
>>
>> If the message says "warning: will never be executed
>> [-Wunreachable-code]" ("Wolf, wolf!") then we have a false positive.
>>
>> The idea that a warning is a false positive if it has not found a bug in
>> the code is rather weird... do you know any warning that is able to
>> always find a bug without knowing the programmer intentions?
>
> I'd like to clarify furhter this point.
>
> What I mean is that if I ask to compiler to show me trigraphs uses with
> -Wtrigraph I expect that it shows me every use and that it does not show
> me the points where I don't use them (as this check can be done without
> false positive and without false negative).
>
> Similary if I ask to see portion of functions that are never executed I
> expect that it shows me all the points where this is provably true and
> that does not show me the points where this is provably false. About the
> points where the compiler is not able to prove this property I expect it
> will be silent (to reduce the signal/noise ration) or that, if useful,
> these points will be shown using a different warning that can be
> enabled/disable separately.

Regardless of our differing motivations/philosophies, I believe we agree here:

-Wunreachable-code should not warn about defaults in defaults in
covered switches.

But where we differ is that I believe (or at least my understanding of
the currently prevailing opinion on the Clang project is that) we
should not emit positive warnings related to the execution of code in
those blocks (including the warning about missing return statements)
because in the signal/noise isn't good enough.

Your change fixes the first issue (unreachable-code) while breaking
the second (missing return & many other warnings (search for
"RuntimeDiagnostic" in the Clang codebase to find other examples)).
That's not acceptable (since the second case contains many
on-by-default warnings that we can't regress in such a way, but
unreachable-code is off by default (precisely because of these sort of
imperfections) so we need to improve unreachable-code without breaking
our existing functionality - then, once we get it up to a higher
quality, we might be able to turn it on by default, or at least
advertise/use it more broadly)