[clang][Sema] Avoid duplicate diagnostics for unreachable fallthrough attribute

[hazohelet]

Summary

This patch checks whether -Wunreachable-code-fallthrough is enabled when clang encounters unreachable fallthrough attributes and, if so, suppresses code will never be executed warning to avoid duplicate warnings.
This fixes https://github.com/llvm/llvm-project/issues/60416

[shafik]

clang/lib/Analysis/ReachableCode.cpp

677

Your passing through DiagnosticsEngine just to check if diag::warn_unreachable_fallthrough_attr is ignored. I think it would be better just to pass through a boolean flag. This would avoid one include as well.

[hazohelet]

Address comments from @shafik

• Pass bool flag UnreachableFallThroughDiagIsEnabled instead of DiagnosticsEngine

[aaron.ballman]

clang/include/clang/Analysis/Analyses/ReachableCode.h

64

We don't typically use const on value types in the project, so dropping this for style consistency. You should make the same edit elsewhere in the patch as well.

clang/lib/Analysis/ReachableCode.cpp

674

Would it be cleaner (less changes) to pass a boolean into this callback to say "the statement had a fallthrough attribute on it" so that UnreachableCodeHandler::HandleUnreachable() can check to see whether the unreachable fallthrough diagnostic is enabled or not? It has access to a Sema object, so it should be able to do the work. FindUnreachableCode() would then not have to change its signature and it keeps the "should I suppress this diagnostic logic" nearby where we issue the diagnostic along with the other duplicate handling logic.

[hazohelet]

Address comments from @aaron.ballman

• Dropped const qualifier from value objects for style consistency
• Passed boolean HasFallThroughAttr to callback and handled warning suppression in HandleUnreachable in order to make code cleaner.

[aaron.ballman]

Thank you, I think this is heading in the right direction! Can you also add a release note to clang/docs/ReleaseNotes.rst about the fix?

clang/lib/Analysis/ReachableCode.cpp

633–635

NFC, just a stylistic choice.

Alternatively: change const AttributedStmt *AS = ... into const auto *AS = ...

722

Accidental whitespace change?

[hazohelet]

Address comments from @aaron.ballman

• NFC stylistic changes

Thanks for the review! Since I don't have commit access, would you please land this patch for me?
Please use "Takuya Shimizu <[e-mail]>" for the patch attribution.

[aaron.ballman]

LGTM!

clang/docs/ReleaseNotes.rst

- Clang had failed to emit some ``-Wundefined-internal`` for members of a local
   class if that class was first introduced with a forward declaration.
 - Diagnostic notes and fix-its are now generated for ``ifunc``/``alias`` attributes
   which point to functions whose names are mangled.
 - Diagnostics relating to macros on the command line of a preprocessed assembly
   file are now reported as coming from the file ``<command line>`` instead of
   ``<built-in>``.
 - Clang constexpr evaluator now provides a more concise diagnostic when calling
   function pointer that is known to be null.
+- Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
+  previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
+  by prioritizing ``-Wunreachable-code-fallthrough``.
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``

clang/include/clang/Analysis/Analyses/ReachableCode.h

enum UnreachableKind {
   UK_Loop_Increment,
   UK_Other
 };
 
 class Callback {
   virtual void anchor();
 public:
   virtual ~Callback() {}
-  virtual void HandleUnreachable(UnreachableKind UK,
-                                 SourceLocation L,
-                                 SourceRange ConditionVal,
-                                 SourceRange R1,
-                                 SourceRange R2) = 0;
+  virtual void HandleUnreachable(UnreachableKind UK, SourceLocation L,
+                                 SourceRange ConditionVal, SourceRange R1,
+                                 SourceRange R2, bool HasFallThroughAttr) = 0;
 };
 
 /// ScanReachableFromBlock - Mark all blocks reachable from Start.
 /// Returns the total number of blocks that were marked reachable.
 unsigned ScanReachableFromBlock(const CFGBlock *Start,
                                 llvm::BitVector &Reachable);
 
 void FindUnreachableCode(AnalysisDeclContext &AC, Preprocessor &PP,
                          Callback &CB);
 
 }} // end namespace clang::reachable_code
 
 #endif

clang/lib/Analysis/ReachableCode.cpp

 //===-- ReachableCode.cpp - Code Reachability Analysis --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a flow-sensitive, path-insensitive analysis of
 // determining reachable blocks within a CFG.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/ReachableCode.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Basic/Builtins.h"
void DeadCodeScan::reportDeadCode(const CFGBlock *B,
   } else if (isTrivialDoWhile(B, S) || isBuiltinUnreachable(S) ||
              isBuiltinAssumeFalse(B, S, C)) {
     return;
   }
   else if (isDeadReturn(B, S)) {
     UK = reachable_code::UK_Return;
   }
 
+  const auto *AS = dyn_cast<AttributedStmt>(S);
+  bool HasFallThroughAttr =
+      AS && hasSpecificAttr<FallThroughAttr>(AS->getAttrs());
+
   SourceRange SilenceableCondVal;
 
   if (UK == reachable_code::UK_Other) {
     // Check if the dead code is part of the "loop target" of
     // a for/for-range loop.  This is the block that contains
     // the increment code.
     if (const Stmt *LoopTarget = B->getLoopTarget()) {
       SourceLocation Loc = LoopTarget->getBeginLoc();
       SourceRange R1(Loc, Loc), R2;
 
       if (const ForStmt *FS = dyn_cast<ForStmt>(LoopTarget)) {
         const Expr *Inc = FS->getInc();
         Loc = Inc->getBeginLoc();
         R2 = Inc->getSourceRange();
       }
 
-      CB.HandleUnreachable(reachable_code::UK_Loop_Increment,
-                           Loc, SourceRange(), SourceRange(Loc, Loc), R2);
+      CB.HandleUnreachable(reachable_code::UK_Loop_Increment, Loc,
+                           SourceRange(), SourceRange(Loc, Loc), R2,
+                           HasFallThroughAttr);
       return;
     }
 
     // Check if the dead block has a predecessor whose branch has
     // a configuration value that *could* be modified to
     // silence the warning.
     CFGBlock::const_pred_iterator PI = B->pred_begin();
     if (PI != B->pred_end()) {
       if (const CFGBlock *PredBlock = PI->getPossiblyUnreachableBlock()) {
         const Stmt *TermCond =
             PredBlock->getTerminatorCondition(/* strip parens */ false);
         isConfigurationValue(TermCond, PP, &SilenceableCondVal);
       }
     }
   }
 
   SourceRange R1, R2;
   SourceLocation Loc = GetUnreachableLoc(S, R1, R2);
-  CB.HandleUnreachable(UK, Loc, SilenceableCondVal, R1, R2);
+  CB.HandleUnreachable(UK, Loc, SilenceableCondVal, R1, R2, HasFallThroughAttr);
 }
 
 //===----------------------------------------------------------------------===//
 // Reachability APIs.
 //===----------------------------------------------------------------------===//
 
 namespace clang { namespace reachable_code {
 
 void Callback::anchor() { }
 
 unsigned ScanReachableFromBlock(const CFGBlock *Start,
for (const CFGBlock *block : *cfg) {
 
     DeadCodeScan DS(reachable, PP, AC.getASTContext());
     numReachable += DS.scanBackwards(block, CB);
 
     if (numReachable == cfg->getNumBlockIDs())
       return;
   }
 }
 
 }} // end namespace clang::reachable_code

clang/lib/Sema/AnalysisBasedWarnings.cpp

 namespace {
   class UnreachableCodeHandler : public reachable_code::Callback {
     Sema &S;
     SourceRange PreviousSilenceableCondVal;
 
   public:
     UnreachableCodeHandler(Sema &s) : S(s) {}
 
-    void HandleUnreachable(reachable_code::UnreachableKind UK,
-                           SourceLocation L,
-                           SourceRange SilenceableCondVal,
-                           SourceRange R1,
-                           SourceRange R2) override {
+    void HandleUnreachable(reachable_code::UnreachableKind UK, SourceLocation L,
+                           SourceRange SilenceableCondVal, SourceRange R1,
+                           SourceRange R2, bool HasFallThroughAttr) override {
+      // If the diagnosed code is `[[fallthrough]];` and
+      // `-Wunreachable-code-fallthrough` is  enabled, suppress `code will never
+      // be executed` warning to avoid generating diagnostic twice
+      if (HasFallThroughAttr &&
+          !S.getDiagnostics().isIgnored(diag::warn_unreachable_fallthrough_attr,
+                                        SourceLocation()))
+        return;
+
       // Avoid reporting multiple unreachable code diagnostics that are
       // triggered by the same conditional value.
       if (PreviousSilenceableCondVal.isValid() &&
           SilenceableCondVal.isValid() &&
           PreviousSilenceableCondVal == SilenceableCondVal)
         return;
       PreviousSilenceableCondVal = SilenceableCondVal;
 

clang/test/Sema/warn-unreachable-fallthrough.c

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c2x -Wunreachable-code-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c2x -Wunreachable-code %s
+// RUN: %clang_cc1 -fsyntax-only -verify=code -std=c2x -Wunreachable-code -Wno-unreachable-code-fallthrough %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c2x -Wno-unreachable-code -Wunreachable-code-fallthrough %s
+
+int n;
+void f(void){
+     switch (n){
+         [[fallthrough]]; // expected-warning{{fallthrough annotation in unreachable code}}
+                          // code-warning@-1{{never be executed}}
+         case 1:;
+     }
+}