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

[David Blaikie]

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.

> that llvm_unreachable in llvm/clang codebase is a needed and correct
> defensive measure.

llvm_unreachable isn't a defensive measure - it causes UB if it's
reached in optimized builds. Indeed there's a thread about what Clang
should do by default for "falling off the end" type code. Currently it
traps on non-opt builds and is unreachable (not llvm_unreachable
(which asserts/prints stuff in debug builds) but the unreachable llvm
instruction which is used for optimization) in optimized builds.

- David