Page MenuHomePhabricator

[Clang] improve -Wimplicit-fallthrough GCC compat
Needs ReviewPublic

Authored by nickdesaulniers on Nov 20 2020, 2:44 PM.

Details

Summary

Clang differs slightly than GCC for a few patterns:

switch (x) {
  case 0:
    ++x;
  default:
    break;
}

switch (x) {
  case 0:
    ++x;
  default:
    goto y;
}
y:;

switch (x) {
  case 0:
    ++x;
  default:
    return;
}

Clang will warn, GCC will not. This is making it excessively painful to
enable -Wimplicit-fallthrough for Linux kernel builds with Clang, see
below link in which 140 patches were sent to try to fix up these
differences in kernel code. Kernel and GCC developers point out that
Clang should not warn in this case.

Intentionally diverge from GCC in one case:

switch (x) {
  case 0:
    ++x;
  default:
    ;
}

Clang will warn, GCC will not.

Fixes: https://github.com/ClangBuiltLinux/linux/issues/636
Link: https://github.com/ClangBuiltLinux/linux/issues/236
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
Link: https://lore.kernel.org/lkml/CANiq72=E_gEVvqUUTSqU4zegC2=yZSTM4b=4G-iofp6d3=UgWQ@mail.gmail.com/T/#t
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Diff Detail

Unit TestsFailed

TimeTest
370 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2020, 2:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers requested review of this revision.Nov 20 2020, 2:44 PM
clang/lib/Sema/AnalysisBasedWarnings.cpp
1156

ah, we want Succ's terminator stmt, not B's.

  • check the successor's terminator
nickdesaulniers edited the summary of this revision. (Show Details)Nov 20 2020, 3:56 PM

Testing this on the kernel with -Wimplicit-fallthrough, there's some additional cases that are still warning:

// mm/vmscan.c
1371         mapping = page_mapping(page);                                                                                                                                                                                                 
1372       case PAGE_CLEAN: // -Wimplicit-fallthrough                                                                                                                                                                                                               
1373         ; /* try to free the page below */                                                                                                                                                                                            
1374       }
// fs/nfs/nfs4client.c
 611       nfs4_schedule_path_down_recovery(pos);                                                                                                                                                                                          
 612     default: // -Wimplicit-fallthrough                                                                                                                                                                                                                         
 613       goto out;
// drivers/hid/usbhid/hid-core.c
 490   case -ESHUTDOWN:  /* unplug */                                                                                                                                                                                                      
 491     unplug = 1; 
 492   case -EILSEQ:   /* protocol error or unplug */ // -Wimplicit-fallthrough...how?!                                                                                                                                     
 493   case -EPROTO:   /* protocol error or unplug */                                                                                                                                                                                      
 494   case -ECONNRESET: /* unlink */                                                                                                                                                                                                      
 495   case -ENOENT:                                                                                                                                                                                                                       
 496   case -EPIPE:    /* report not available */                                                                                                                                                                                          
 497     break;
// security/keys/process_keys.c
782       default:                                                                                                                                                                                                                         
783         if (need_perm != KEY_AUTHTOKEN_OVERRIDE &&                                                                                                                                                                                     
784             need_perm != KEY_DEFER_PERM_CHECK)                                                                                                                                                                                         
785           goto invalid_key;                                                                                                                                                                                                            
786       case 0: // -Wimplicit-fallthrough...how?!                                                                                                                                                                                      
787         break;                                                                                                                                                                                                                         
788       }

Ah, I think I need to reconsider the case that there is more than one successor, or at least differentiate which successor is the fallthrough block, rather than only checking blocks that have 1 successor.

nickdesaulniers planned changes to this revision.Nov 20 2020, 4:50 PM

And for drivers/hid/usbhid/hid-core.c it looks like multiple labels are modeled as individual blocks in the control flow graph.

  • rewrite to handle a few more cases from the Linux kernel
  • git-clang-format HEAD~
nickdesaulniers retitled this revision from [Clang] avoid -Wimplicit-fallthrough for fallthrough to break to [Clang] improve -Wimplicit-fallthrough GCC compat.Nov 23 2020, 4:57 PM
nickdesaulniers edited the summary of this revision. (Show Details)

I think that not warning upon falling through to goto or return is a mistake. We have a confirmed case of a bug where clang would catch this bug whereas gcc would not: https://lore.kernel.org/lkml/20201121124019.21116-1-ogabbay@kernel.org/. I suspect that warnings from falling through to break and ; make up the vast majority of the noise in the Linux kernel and I highly doubt that those are bugs. The other two generally have code flow implications that should be annotated. I will modify this patch tonight and see how many warnings we get from goto and return statements against mainline.

clang/lib/Sema/AnalysisBasedWarnings.cpp
1102

is there maybe a nicer way to do this using std::bind or std::mem_fn?

I'm happy to modify the patch based on feedback from LKML, though

switch (x) {
  case 0:
    ++x;
  default:
    break;
}

is the important case that's 99% of the differences between GCC and Clang, which should not get removed from this patch.

I'd suggest instead of removing these warnings, we split the warning flag in two:

  1. A warning flag that warns that there is implicit fallthrough that's not equivalent to break; (which sounds like it's what the kernel folks want in the 99% case) -- that is, warn if the language rule "if execution from one case label reaches a case label attached to a different statement, an implicit break is evaluated" could give different program behavior.
  2. A warning flag that warns that there is implicit fallthrough at all (a superset of (1), and the same as our current warning flag) -- that is, warn if control flow from one case label is able to fall through a case label attached to a different statement.

The linux kernel folks aren't the only users of this facility, and some other users will want an explicit break; even before a set of case labels that themselves label a break;, and will want that enforced by warning. I'd also suggest that we retain our existing warning flag for (2) and add a new warning flag for the subset behavior of (1).

kees added a subscriber: kees.EditedNov 25 2020, 1:33 PM

The kernel's stance on switch statements reads:

All switch/case blocks must end in one of:

break;
fallthrough;
continue;
goto <label>;
return [expression];

And we've had multiple real bugs now of the "missing break before default return" style:

https://git.kernel.org/linus/291ddeb621e4a9f1ced8302a777fbd7fbda058c6

So, from the kernel's perspective, please don't make -Wimplicit-fallthrough more permissive -- we're finding real bugs. :)

And to speak to another example in the thread:

// drivers/hid/usbhid/hid-core.c
 490   case -ESHUTDOWN:  /* unplug */                                                                                                                                                                                                      
 491     unplug = 1; 
 492   case -EILSEQ:   /* protocol error or unplug */ // -Wimplicit-fallthrough...how?!                                                                                                                                     
 493   case -EPROTO:   /* protocol error or unplug */                                                                                                                                                                                      
 494   case -ECONNRESET: /* unlink */                                                                                                                                                                                                      
 495   case -ENOENT:                                                                                                                                                                                                                       
 496   case -EPIPE:    /* report not available */                                                                                                                                                                                          
 497     break;

I think this should warn too. While this won't turn into a "missing break" error, there's no way to know (from looking at code) what the _intent_ is here. Should it have done "return?" Why is "break" assumed to be safe here, etc?

Quuxplusone added a subscriber: Quuxplusone.EditedNov 25 2020, 2:37 PM

I think this should warn too. While this won't turn into a "missing break" error, there's no way to know (from looking at code) what the _intent_ is here.

Hear, hear. +1 on everything @kees said but especially this.
(Also: "The job of the compiler-diagnostic-developer is to create some separation between the space of inputs that the compiler considers “clearly X” and the space of inputs that the compiler considers “clearly Y.” Essentially, we create an error-correcting code by deliberately increasing the edit distance between pairs of inequivalent C++ programs — deliberately increasing the number of keystrokes the programmer would have to screw up in order to transform a working (and warning-free) C++ program into an inequivalent broken (yet warning-free) C++ program.")

Also also, edited to add: Has anyone contacted GCC to get them to improve their diagnostic capability in this area? It seems like that would be the vastly more useful way to align GCC's diagnostics with Clang's diagnostics.