[Clang] improve -Wimplicit-fallthrough GCC compat

[nickdesaulniers]

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=[e-mail]/T/#t
Signed-off-by: Nick Desaulniers <[e-mail]>

[nickdesaulniers]

clang/lib/Sema/AnalysisBasedWarnings.cpp

1156

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

[nickdesaulniers]

• check the successor's terminator

[nickdesaulniers]

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]

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

[nickdesaulniers]

• rewrite to handle a few more cases from the Linux kernel

[nickdesaulniers]

• git-clang-format HEAD~

[nathanchance]

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/[e-mail]/. 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.

[nickdesaulniers]

clang/lib/Sema/AnalysisBasedWarnings.cpp

1102

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

[nickdesaulniers]

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.

[rsmith]

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]

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]

In D91895#2417170, @kees wrote:

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.

clang/lib/Sema/AnalysisBasedWarnings.cpp

bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt,
       int UnannotatedCnt = 0;
       AnnotatedCnt = 0;
 
       std::deque<const CFGBlock*> BlockQueue(B.pred_begin(), B.pred_end());
       while (!BlockQueue.empty()) {
         const CFGBlock *P = BlockQueue.front();
         BlockQueue.pop_front();
         if (!P) continue;
 
         const Stmt *Term = P->getTerminatorStmt();
         if (Term && isa<SwitchStmt>(Term))
           continue; // Switch statement, good.
 
         const SwitchCase *SW = dyn_cast_or_null<SwitchCase>(P->getLabel());
         if (SW && SW->getSubStmt() == B.getLabel() && P->begin() == P->end())
           continue; // Previous case label has no statements, good.
 
bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt,
 
         const Stmt *LastStmt = getLastStmt(*P);
         if (const AttributedStmt *AS = asFallThroughAttr(LastStmt)) {
           markFallthroughVisited(AS);
           ++AnnotatedCnt;
           continue; // Fallthrough annotation, good.
         }
 
+        // If next statment in P's successor is a break with no other
+        // statements, no issue.
+        if (P->succ_size() == 1) {
+          const CFGBlock::AdjacentBlock &Succ = *P->succ_begin();
+          const Stmt *Term = B.getTerminatorStmt();
+          if (Succ->size() == 0 && Term && isa<BreakStmt>(Term))
+            continue;
+        }
+
         if (!LastStmt) { // This block contains no executable statements.
           // Traverse its predecessors.
           std::copy(P->pred_begin(), P->pred_end(),
                     std::back_inserter(BlockQueue));
           continue;
         }
 
         ++UnannotatedCnt;

clang/test/SemaCXX/switch-implicit-fallthrough-blocks.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -fblocks -std=c++11 -Wimplicit-fallthrough %s
 
 void fallthrough_in_blocks() {
   void (^block)() = ^{
     int x = 0;
     switch (x) {
     case 0:
       x++;
       [[clang::fallthrough]]; // no diagnostics
+    case 2:
+      x++;
+    case 3: // no diagnostics
+      break;
     case 1:
       x++;
     default: // \
         expected-warning{{unannotated fall-through between switch labels}} \
+        expected-note{{insert '[[clang::fallthrough]];' to silence this warning}} \
         expected-note{{insert 'break;' to avoid fall-through}}
+      x++;
       break;
     }
   };
   block();
 }

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

 }
 
 #define MY_SWITCH(X, Y, Z, U, V) switch (X) { case Y: Z; case U: V; }
 #define MY_SWITCH2(X, Y, Z) switch (X) { Y; Z; }
 #define MY_CASE(X, Y) case X: Y
 #define MY_CASE2(X, Y, U, V) case X: Y; case U: V
 
 int fallthrough_macro1(int n) {
-  MY_SWITCH(n, 13, n *= 2, 14, break)  // expected-warning{{unannotated fall-through between switch labels}}
+  MY_SWITCH(n, 13, n *= 2, 14, break)
 
   switch (n + 1) {
     MY_CASE(33, n += 2);
-    MY_CASE(44, break);  // expected-warning{{unannotated fall-through between switch labels}}
-    MY_CASE(55, n += 3);
+    MY_CASE(44, n += 3);  // expected-warning{{unannotated fall-through between switch labels}}
+    MY_CASE(55, break);
   }
 
   switch (n + 3) {
     MY_CASE(333, return 333);
-    MY_CASE2(444, n += 44, 4444, break);  // expected-warning{{unannotated fall-through between switch labels}}
+    MY_CASE2(444, n += 44, 4444, break);
     MY_CASE(555, n += 33);
   }
 
-  MY_SWITCH2(n + 4, MY_CASE(17, n *= 3), MY_CASE(19, break))  // expected-warning{{unannotated fall-through between switch labels}}
+  MY_SWITCH2(n + 4, MY_CASE(17, n *= 3), MY_CASE(19, break))
 
-  MY_SWITCH2(n + 5, MY_CASE(21, break), MY_CASE2(23, n *= 7, 25, break))  // expected-warning{{unannotated fall-through between switch labels}}
+  MY_SWITCH2(n + 5, MY_CASE(21, break), MY_CASE2(23, n *= 7, 25, break))
 
   return n;
 }
 
 void fallthrough_cfgblock_with_null_successor(int x) {
   (x && "") ? (void)(0) : (void)(1);
   switch (x) {}
 }
void fallthrough_in_local_class() {
   class C {
     void f(int x) {
       switch (x) {
         case 0:
           x++;
           [[clang::fallthrough]]; // no diagnostics
         case 1:
           x++;
+        case 2: // no diagnostics
+          break;
+        case 3:
+          x++;
         default: // \
             expected-warning{{unannotated fall-through between switch labels}} \
+            expected-note{{insert '[[clang::fallthrough]];' to silence this warning}} \
             expected-note{{insert 'break;' to avoid fall-through}}
+          x++;
           break;
       }
     }
   };
 }
 
 // Fallthrough annotations in lambdas used to generate "fallthrough
 // annotation does not directly precede switch label" warning.
 void fallthrough_in_lambda() {
   (void)[] {
     int x = 0;
     switch (x) {
     case 0:
       x++;
       [[clang::fallthrough]]; // no diagnostics
     case 1:
       x++;
+    case 2: // no diagnostics
+      break;
+    case 3:
+      x++;
     default: // \
         expected-warning{{unannotated fall-through between switch labels}} \
+        expected-note{{insert '[[clang::fallthrough]];' to silence this warning}} \
         expected-note{{insert 'break;' to avoid fall-through}}
+      x++;
       break;
     }
   };
 }
 
 namespace PR18983 {
   void fatal() __attribute__((noreturn));
   int num();