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

[David Blaikie]

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

We need the two-state CFG to represent maximally and minimally
reachable code. Maximal for -Wunreachable and minimal for 'runtime'
diagnostics.

- David