[CFG] Add a BuildOption to consider default branch of switch on covered enumerations

[frederic-tingaud-sonarsource]

By default, the default: branch (or the successor if there is no default and cases return) of a switch on fully covered enumerations is considered as "Unreachable". It is a sane assumption in most cases, but not always. That commit allows to change such behavior when needed.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Fred Tingaud (frederic-tingaud-sonarsource)

Changes

By default, the default: branch (or the successor if there is no default and cases return) of a switch on fully covered enumerations is considered as "Unreachable". It is a sane assumption in most cases, but not always. That commit allows to change such behavior when needed.

---

Full diff: https://github.com/llvm/llvm-project/pull/161345.diff

3 Files Affected:

• (modified) clang/include/clang/Analysis/CFG.h (+1)
• (modified) clang/lib/Analysis/CFG.cpp (+4-1)
• (modified) clang/unittests/Analysis/CFGTest.cpp (+152)

diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
index 1b1ff5e558ec5..edfd655b579b9 100644
--- a/clang/include/clang/Analysis/CFG.h
+++ b/clang/include/clang/Analysis/CFG.h
@@ -1251,6 +1251,7 @@ class CFG {
     bool MarkElidedCXXConstructors = false;
     bool AddVirtualBaseBranches = false;
     bool OmitImplicitValueInitializers = false;
+    bool SwitchKeepDefaultCoveredEnum = false;
 
     BuildOptions() = default;
 
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 60a2d113c08e2..e5843f3815c5f 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4516,9 +4516,12 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) {
   //
   // Note: We add a successor to a switch that is considered covered yet has no
   //       case statements if the enumeration has no enumerators.
+  //       We also consider this successor reachable if
+  //       BuildOpts.SwitchReqDefaultCoveredEnum is true.
   bool SwitchAlwaysHasSuccessor = false;
   SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
-  SwitchAlwaysHasSuccessor |= Terminator->isAllEnumCasesCovered() &&
+  SwitchAlwaysHasSuccessor |= !BuildOpts.SwitchKeepDefaultCoveredEnum &&
+                              Terminator->isAllEnumCasesCovered() &&
                               Terminator->getSwitchCaseList();
   addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
                !SwitchAlwaysHasSuccessor);
diff --git a/clang/unittests/Analysis/CFGTest.cpp b/clang/unittests/Analysis/CFGTest.cpp
index 46a6751391cf5..cce0f6bbcfa74 100644
--- a/clang/unittests/Analysis/CFGTest.cpp
+++ b/clang/unittests/Analysis/CFGTest.cpp
@@ -93,6 +93,158 @@ TEST(CFG, DependantBaseAddImplicitDtors) {
                 .getStatus());
 }
 
+TEST(CFG, SwitchCoveredEnumNoDefault) {
+  const char *Code = R"(
+    enum class E {E1, E2};
+    int f(E e) {
+      switch(e) {
+        case E::E1:
+          return 1;
+        case E::E2:
+          return 2;
+      }
+      return 0;
+    }
+  )";
+  CFG::BuildOptions Options;
+  Options.SwitchKeepDefaultCoveredEnum = true;
+  BuildResult B = BuildCFG(Code, Options);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+
+  // [B5 (ENTRY)]
+  //   Succs (1): B2
+  //
+  // [B1]
+  //   1: 0
+  //   2: return [B1.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B2]
+  //   1: e (ImplicitCastExpr, LValueToRValue, E)
+  //   T: switch [B2.1]
+  //   Preds (1): B5
+  //   Succs (3): B3 B4 B1
+  //
+  // [B3]
+  //  case E::E2:
+  //   1: 2
+  //   2: return [B3.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B4]
+  //  case E::E1:
+  //   1: 1
+  //   2: return [B4.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B0 (EXIT)]
+  //   Preds (3): B1 B3 B4
+
+  const auto &Entry = B.getCFG()->getEntry();
+  EXPECT_EQ(1u, Entry.succ_size());
+  // First successor of Entry is the switch
+  CFGBlock *SwitchBlock = *Entry.succ_begin();
+  EXPECT_EQ(3u, SwitchBlock->succ_size());
+  // Last successor of the switch is after the switch
+  auto NoCaseSucc = SwitchBlock->succ_rbegin();
+  EXPECT_TRUE(NoCaseSucc->isReachable());
+
+  // Checking that the same node is Unreachable without this setting
+  Options.SwitchKeepDefaultCoveredEnum = false;
+  B = BuildCFG(Code, Options);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+
+  const auto &Entry2 = B.getCFG()->getEntry();
+  EXPECT_EQ(1u, Entry2.succ_size());
+  CFGBlock *SwitchBlock2 = *Entry2.succ_begin();
+  EXPECT_EQ(3u, SwitchBlock2->succ_size());
+  auto NoCaseSucc2 = SwitchBlock2->succ_rbegin();
+  EXPECT_FALSE(NoCaseSucc2->isReachable());
+}
+
+TEST(CFG, SwitchCoveredEnumWithDefault) {
+  const char *Code = R"(
+    enum class E {E1, E2};
+    int f(E e) {
+      switch(e) {
+        case E::E1:
+          return 1;
+        case E::E2:
+          return 2;
+        default:
+          return 0;
+      }
+      return -1;
+    }
+  )";
+  CFG::BuildOptions Options;
+  Options.SwitchKeepDefaultCoveredEnum = true;
+  BuildResult B = BuildCFG(Code, Options);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+
+  // [B6 (ENTRY)]
+  //   Succs (1): B2
+  //
+  // [B1]
+  //   1: -1
+  //   2: return [B1.1];
+  //   Succs (1): B0
+  //
+  // [B2]
+  //   1: e (ImplicitCastExpr, LValueToRValue, E)
+  //   T: switch [B2.1]
+  //   Preds (1): B6
+  //   Succs (3): B4 B5 B3
+  //
+  // [B3]
+  //  default:
+  //   1: 0
+  //   2: return [B3.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B4]
+  //  case E::E2:
+  //   1: 2
+  //   2: return [B4.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B5]
+  //  case E::E1:
+  //   1: 1
+  //   2: return [B5.1];
+  //   Preds (1): B2
+  //   Succs (1): B0
+  //
+  // [B0 (EXIT)]
+  //   Preds (4): B1 B3 B4 B5
+
+  const auto &Entry = B.getCFG()->getEntry();
+  EXPECT_EQ(1u, Entry.succ_size());
+  // First successor of Entry is the switch
+  CFGBlock *SwitchBlock = *Entry.succ_begin();
+  EXPECT_EQ(3u, SwitchBlock->succ_size());
+  // Last successor of the switch is the default branch
+  auto defaultBlock = SwitchBlock->succ_rbegin();
+  EXPECT_TRUE(defaultBlock->isReachable());
+
+  // Checking that the same node is Unreachable without this setting
+  Options.SwitchKeepDefaultCoveredEnum = false;
+  B = BuildCFG(Code, Options);
+  EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus());
+
+  const auto &Entry2 = B.getCFG()->getEntry();
+  EXPECT_EQ(1u, Entry2.succ_size());
+  CFGBlock *SwitchBlock2 = *Entry2.succ_begin();
+  EXPECT_EQ(3u, SwitchBlock2->succ_size());
+  auto defaultBlock2 = SwitchBlock2->succ_rbegin();
+  EXPECT_FALSE(defaultBlock2->isReachable());
+}
+
 TEST(CFG, IsLinear) {
   auto expectLinear = [](bool IsLinear, const char *Code) {
     BuildResult B = BuildCFG(Code);

[frederic-tingaud-sonarsource]

Pinging @gribozavr, @ymand and @sgatev as official maintainers of CFG analysis.

[steakhal]

LGTM now. Thanks.

[frederic-tingaud-sonarsource]

@steakhal I don't have the rights to merge. Could you please do it for me?