[clang] Expose unreachable fallthrough annotation warning

[nathanchance]

Summary

The Linux kernel has a macro called IS_ENABLED(), which evaluates to a
constant 1 or 0 based on Kconfig selections, allowing C code to be
unconditionally enabled or disabled at build time. For example:

int foo(struct *a, int b) {
    switch (b) {
    case 1:
        if (a->flag || !IS_ENABLED(CONFIG_64BIT))
            return 1;
        __attribute__((fallthrough));
    case 2:
        return 2;
    default:
        return 3;
    }
}

There is an unreachable warning about the fallthrough annotation in the
first case because !IS_ENABLED(CONFIG_64BIT) can be evaluated to 1,
which looks like

return 1;
__attribute__((fallthrough));

to clang.

This type of warning is pointless for the Linux kernel because it does
this trick all over the place due to the sheer number of configuration
options that it has.

Add -Wunreachable-code-fallthrough, enabled under -Wunreachable-code, so
that projects that want to warn on unreachable code get this warning but
projects that do not care about unreachable code can still use
-Wimplicit-fallthrough without having to make changes to their code
base.

Fixes PR51094.

[nickdesaulniers]

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

211–214

These 4 lines don't add anything to the test coverage. Remove them?

[xbolva00]

GCC does not warn (with common -Wall) for this case, right? I think Clang should not as well.

ImplicitFallthroughUnreachable could be enabled with -Wunreachable-code, if you think we should have it.

[dblaikie]

In D107933#2942023, @xbolva00 wrote:

GCC does not warn (with common -Wall) for this case, right? I think Clang should not as well.

ImplicitFallthroughUnreachable could be enabled with -Wunreachable-code, if you think we should have it.

Yeah, some of that was discussed on the bug: https://bugs.llvm.org/show_bug.cgi?id=51094 & I'd still be in favor of that sort of direction. I might go so far as to say: Maybe we should drop this warning flag (and/or move it under -Wunreachable-code - questionable, I don't think anyone's really using that, it's pretty noisy still, I think) entirely even if no one's willing to reimplement it more robustly... not sure.

[nathanchance]

In D107933#2942023, @xbolva00 wrote:

GCC does not warn (with common -Wall) for this case, right? I think Clang should not as well.

Correct, GCC does not warn at all about unreachable fallthrough annotations as far as I am aware.

In D107933#2942102, @dblaikie wrote:

In D107933#2942023, @xbolva00 wrote:

ImplicitFallthroughUnreachable could be enabled with -Wunreachable-code, if you think we should have it.

Yeah, some of that was discussed on the bug: https://bugs.llvm.org/show_bug.cgi?id=51094 & I'd still be in favor of that sort of direction. I might go so far as to say: Maybe we should drop this warning flag (and/or move it under -Wunreachable-code - questionable, I don't think anyone's really using that, it's pretty noisy still, I think) entirely even if no one's willing to reimplement it more robustly... not sure.

The kernel does not use -Wunreachable-code for this reason and several others; we have explored it in the past and found that there were pretty much no instances where the warnings indicated a bug and kernel maintainers were irritated with some of the patches sent. So moving this warning under there (as -Wunreachable-code-fallthrough?) would work for us. I do not have a strong opinion though. I am more than happy to remove the warning entirely as well.

[xbolva00]

I think we should just move it, not delete it.

[nathanchance]

So something more along the lines of

diff
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 1555a9ee2465..0aa9a82e3d11 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -823,10 +823,12 @@ def UnreachableCode : DiagGroup<"unreachable-code",
                                 [UnreachableCodeLoopIncrement]>;
 def UnreachableCodeBreak : DiagGroup<"unreachable-code-break">;
 def UnreachableCodeReturn : DiagGroup<"unreachable-code-return">;
+def UnreachableCodeFallthrough : DiagGroup<"unreachable-code-fallthrough">;
 def UnreachableCodeAggressive : DiagGroup<"unreachable-code-aggressive",
                                     [UnreachableCode,
                                      UnreachableCodeBreak,
-                                     UnreachableCodeReturn]>;
+                                     UnreachableCodeReturn,
+                                     UnreachableCodeFallthrough]>;

 // Aggregation warning settings.

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index bf857f58951b..f533076a6c77 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -682,6 +682,9 @@ def warn_unreachable_return : Warning<
 def warn_unreachable_loop_increment : Warning<
   "loop will run at most once (loop increment never executed)">,
   InGroup<UnreachableCodeLoopIncrement>, DefaultIgnore;
+def warn_unreachable_fallthrough_attr : Warning<
+  "fallthrough annotation in unreachable code">,
+  InGroup<UnreachableCodeFallthrough>, DefaultIgnore;
 def note_unreachable_silence : Note<
   "silence by adding parentheses to mark code as explicitly dead">;

@@ -9578,9 +9581,6 @@ def err_fallthrough_attr_outside_switch : Error<
   "fallthrough annotation is outside switch statement">;
 def err_fallthrough_attr_invalid_placement : Error<
   "fallthrough annotation does not directly precede switch label">;
-def warn_fallthrough_attr_unreachable : Warning<
-  "fallthrough annotation in unreachable code">,
-  InGroup<ImplicitFallthrough>, DefaultIgnore;

 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index aa2602c8d925..99ce143d3559 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1125,7 +1125,7 @@ namespace {
                 // unreachable in all instantiations of the template.
                 if (!IsTemplateInstantiation)
                   S.Diag(AS->getBeginLoc(),
-                         diag::warn_fallthrough_attr_unreachable);
+                         diag::warn_unreachable_fallthrough_attr);
                 markFallthroughVisited(AS);
                 ++AnnotatedCnt;
                 break;

if I understand correctly?

[xbolva00]

Yes, something like that, plus I think you want put UnreachableCodeFallthrough into group UnreachableCode as well.

[dblaikie]

Probably still worth fixing the bug too? (though maybe not a priority once it's moved out into -Wunreachable-code) - though the general fix, as discussed on the bug (comment 18 and 19 about why this doesn't already produce an unreachable-code warning), may require a significant amount of work.

[nathanchance]

In D107933#2942430, @xbolva00 wrote:

Yes, something like that, plus I think you want put UnreachableCodeFallthrough into group UnreachableCode as well.

So you would recommend adding it to UnreachableCode rather than UnreachableCodeAggressive?

In D107933#2942432, @dblaikie wrote:

Probably still worth fixing the bug too? (though maybe not a priority once it's moved out into -Wunreachable-code) - though the general fix, as discussed on the bug (comment 18 and 19 about why this doesn't already produce an unreachable-code warning), may require a significant amount of work.

I guess not warning on fallthrough attributes that are preceded by an if statement with an integer constant would remove all problematic instances in the kernel I believe. I am just not sure how to put that into code :)

[aaron.ballman]

In D107933#2944135, @nathanchance wrote:

In D107933#2942430, @xbolva00 wrote:

Yes, something like that, plus I think you want put UnreachableCodeFallthrough into group UnreachableCode as well.

So you would recommend adding it to UnreachableCode rather than UnreachableCodeAggressive?

I would recommend adding it to UnreachableCode as I don't see this being a particularly aggressive unreachable warning. FWIW, I would be opposed to dropping the diagnostic entirely as the standard recommends diagnosing an unreachable fallthrough statement (https://eel.is/c++draft/dcl.attr.fallthrough#2.sentence-2 and the similar wording in C2x 6.7.11.5p3.

[dblaikie]

In D107933#2944204, @aaron.ballman wrote:

In D107933#2944135, @nathanchance wrote:

In D107933#2942430, @xbolva00 wrote:

Yes, something like that, plus I think you want put UnreachableCodeFallthrough into group UnreachableCode as well.

So you would recommend adding it to UnreachableCode rather than UnreachableCodeAggressive?

I would recommend adding it to UnreachableCode as I don't see this being a particularly aggressive unreachable warning. FWIW, I would be opposed to dropping the diagnostic entirely as the standard recommends diagnosing an unreachable fallthrough statement (https://eel.is/c++draft/dcl.attr.fallthrough#2.sentence-2 and the similar wording in C2x 6.7.11.5p3.

(totally not to derail this, but... - I'm not sure that wording in the spec is especially informative/worth worrying about too much. If we have a warning we know isn't especially informative and is off-by-default, I'm not sure it makes too much difference whether we have it all. The spec doesn't have a lot of practice spec'ing warnings, which have a bunch of nuance that the spec doesn't usually have to deal with)

[nathanchance]

• -Wimplicit-fallthrough-unreachable -> -Wunreachable-code-fallthrough, enabled under -Wunreachable-code (Dávid, Aaron).

• Drop "case 225" from new test, as it does not add to test coverage (Nick).

• Add -Wunreachable-code-fallthrough to PR30636 test command, as it is explicitly testing that there is no unreachable warning with template function instantiation.

[nathanchance]

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

211–214

Done.

[aaron.ballman]

LGTM, I think this is incremental progress.

[dblaikie]

Patch description probably needs an update - looks like it's out of date ("Add -Wimplicit-fallthrough-unreachable, which is default enabled with -Wimplicit-fallthrough" should read, I guess "Add -Wunreachable-code-fallthrough, which is default enabled with -Wunreachable-code" - also, I might avoid using the "default enabled" as it may lead to some confusion/sound like it's suggesting that "this subwarning is on by default as it's grouped under this other on-by-default warning", which isn't the case - maybe "which is enabled with -Wunreachable-code" would suffice? or "which is part of the -Wunreachable-code group")

[nathanchance]

• Update commit message (forgot to use arc diff --edit --verbatim) [David]

clang/include/clang/Basic/DiagnosticGroups.td

 
 // Unreachable code warning groups.
 //
 //  The goal is make -Wunreachable-code on by default, in -Wall, or at
 //  least actively used, with more noisy versions of the warning covered
 //  under separate flags.
 //
 def UnreachableCodeLoopIncrement : DiagGroup<"unreachable-code-loop-increment">;
+def UnreachableCodeFallthrough : DiagGroup<"unreachable-code-fallthrough">;
 def UnreachableCode : DiagGroup<"unreachable-code",
-                                [UnreachableCodeLoopIncrement]>;
+                                [UnreachableCodeLoopIncrement,
+                                 UnreachableCodeFallthrough]>;
 def UnreachableCodeBreak : DiagGroup<"unreachable-code-break">;
 def UnreachableCodeReturn : DiagGroup<"unreachable-code-return">;
 def UnreachableCodeAggressive : DiagGroup<"unreachable-code-aggressive",
                                     [UnreachableCode,
                                      UnreachableCodeBreak,
                                      UnreachableCodeReturn]>;
 
 // Aggregation warning settings.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_unreachable_break : Warning<
   "'break' will never be executed">,
   InGroup<UnreachableCodeBreak>, DefaultIgnore;
 def warn_unreachable_return : Warning<
   "'return' will never be executed">,
   InGroup<UnreachableCodeReturn>, DefaultIgnore;
 def warn_unreachable_loop_increment : Warning<
   "loop will run at most once (loop increment never executed)">,
   InGroup<UnreachableCodeLoopIncrement>, DefaultIgnore;
+def warn_unreachable_fallthrough_attr : Warning<
+  "fallthrough annotation in unreachable code">,
+  InGroup<UnreachableCodeFallthrough>, DefaultIgnore;
 def note_unreachable_silence : Note<
   "silence by adding parentheses to mark code as explicitly dead">;
 
 /// Built-in functions.
 def ext_implicit_lib_function_decl : ExtWarn<
   "implicitly declaring library function '%0' with type %1">,
   InGroup<ImplicitFunctionDeclare>;
 def note_include_header_or_declare : Note<
def note_insert_break_fixit : Note<
   "insert 'break;' to avoid fall-through">;
 def err_fallthrough_attr_wrong_target : Error<
   "%0 attribute is only allowed on empty statements">;
 def note_fallthrough_insert_semi_fixit : Note<"did you forget ';'?">;
 def err_fallthrough_attr_outside_switch : Error<
   "fallthrough annotation is outside switch statement">;
 def err_fallthrough_attr_invalid_placement : Error<
   "fallthrough annotation does not directly precede switch label">;
-def warn_fallthrough_attr_unreachable : Warning<
-  "fallthrough annotation in unreachable code">,
-  InGroup<ImplicitFallthrough>, DefaultIgnore;
 
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup<CoveredSwitchDefault>, DefaultIgnore;
 def warn_not_in_enum : Warning<"case value not in enumerated type %0">,
   InGroup<Switch>;
 def warn_not_in_enum_assignment : Warning<"integer constant not in range "
   "of enumerated type %0">, InGroup<DiagGroup<"assign-enum">>, DefaultIgnore;

clang/lib/Sema/AnalysisBasedWarnings.cpp

bool checkFallThroughIntoBlock(const CFGBlock &B, int &AnnotatedCnt,
                ElemIt != ElemEnd; ++ElemIt) {
             if (Optional<CFGStmt> CS = ElemIt->getAs<CFGStmt>()) {
               if (const AttributedStmt *AS = asFallThroughAttr(CS->getStmt())) {
                 // Don't issue a warning for an unreachable fallthrough
                 // attribute in template instantiations as it may not be
                 // unreachable in all instantiations of the template.
                 if (!IsTemplateInstantiation)
                   S.Diag(AS->getBeginLoc(),
-                         diag::warn_fallthrough_attr_unreachable);
+                         diag::warn_unreachable_fallthrough_attr);
                 markFallthroughVisited(AS);
                 ++AnnotatedCnt;
                 break;
               }
               // Don't care about other unreachable statements.
             }
           }
           // If there are no unreachable statements, this may be a special

clang/test/SemaCXX/P30636.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough -Wunreachable-code-fallthrough %s
 // expected-no-diagnostics
 
 template<bool param>
 int fallthrough_template(int i)
 {
   switch (i) {
     case 1:
       if (param)

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

-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wimplicit-fallthrough -Wunreachable-code-fallthrough %s
 
 
 int fallthrough(int n) {
   switch (n / 10) {
       if (n - 1) {
         n = 100;
       } else if (n - 2) {
         n = 101;
case 222:
       return 2;
       __attribute__((fallthrough)); // expected-warning{{fallthrough annotation in unreachable code}}
     case 223:
       n += 400;
     case 224: // 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}}
         ;
   }
 
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunreachable-code-fallthrough"
+  switch (n) {
+      n += 300;
+      [[clang::fallthrough]];  // no warning here
+    case 221:
+      return 1;
+      [[clang::fallthrough]];  // no warning here
+    case 222:
+      return 2;
+      __attribute__((fallthrough)); // no warning here
+    case 223:
+      if (1)
+        return 3;
+      __attribute__((fallthrough)); // no warning here
+    case 224:
+      n += 400;
+  }
+#pragma clang diagnostic pop
+
   long p = static_cast<long>(n) * n;
   switch (sizeof(p)) {
     case 9:
       n += static_cast<int>(p >> 32);
       [[clang::fallthrough]];  // no warning here
     case 5:
       n += static_cast<int>(p);
       [[clang::fallthrough]];  // no warning here