From 4af057dd6b5709d7de819108a14949ad5a0445fd Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Mon, 29 Sep 2025 18:37:48 +0200 Subject: [PATCH 1/3] CFG: Add a BuildOption to consider default branch of switch on covered enumerations. By default, the `default:` branch of a switch on fully covered enumarations is considered as "Unreachable". It is a sane assumption in most cases, but not always. That commit allows to change such behavior when needed. --- clang/include/clang/Analysis/CFG.h | 1 + clang/lib/Analysis/CFG.cpp | 5 +- clang/unittests/Analysis/CFGTest.cpp | 152 +++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) 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); From dd2c98b92abfde18da44c39eab5a16bff1977eb3 Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Thu, 2 Oct 2025 11:54:01 +0200 Subject: [PATCH 2/3] Apply suggestions --- clang/include/clang/Analysis/CFG.h | 2 +- clang/lib/Analysis/CFG.cpp | 6 +++--- clang/unittests/Analysis/CFGTest.cpp | 27 ++++++++++++++------------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h index edfd655b579b9..6dd7d138e4357 100644 --- a/clang/include/clang/Analysis/CFG.h +++ b/clang/include/clang/Analysis/CFG.h @@ -1251,7 +1251,7 @@ class CFG { bool MarkElidedCXXConstructors = false; bool AddVirtualBaseBranches = false; bool OmitImplicitValueInitializers = false; - bool SwitchKeepDefaultCoveredEnum = false; + bool AssumeReachableDefaultInSwitchStatements = false; BuildOptions() = default; diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index e5843f3815c5f..cdde849b0e026 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -4520,9 +4520,9 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) { // BuildOpts.SwitchReqDefaultCoveredEnum is true. bool SwitchAlwaysHasSuccessor = false; SwitchAlwaysHasSuccessor |= switchExclusivelyCovered; - SwitchAlwaysHasSuccessor |= !BuildOpts.SwitchKeepDefaultCoveredEnum && - Terminator->isAllEnumCasesCovered() && - Terminator->getSwitchCaseList(); + SwitchAlwaysHasSuccessor |= + !BuildOpts.AssumeReachableDefaultInSwitchStatements && + Terminator->isAllEnumCasesCovered() && Terminator->getSwitchCaseList(); addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock, !SwitchAlwaysHasSuccessor); diff --git a/clang/unittests/Analysis/CFGTest.cpp b/clang/unittests/Analysis/CFGTest.cpp index cce0f6bbcfa74..aeeff6b723532 100644 --- a/clang/unittests/Analysis/CFGTest.cpp +++ b/clang/unittests/Analysis/CFGTest.cpp @@ -107,7 +107,7 @@ TEST(CFG, SwitchCoveredEnumNoDefault) { } )"; CFG::BuildOptions Options; - Options.SwitchKeepDefaultCoveredEnum = true; + Options.AssumeReachableDefaultInSwitchStatements = true; BuildResult B = BuildCFG(Code, Options); EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); @@ -143,24 +143,25 @@ TEST(CFG, SwitchCoveredEnumNoDefault) { // [B0 (EXIT)] // Preds (3): B1 B3 B4 - const auto &Entry = B.getCFG()->getEntry(); - EXPECT_EQ(1u, Entry.succ_size()); + auto *CFG = B.getCFG(); + const auto &Entry = CFG->getEntry(); + ASSERT_EQ(1u, Entry.succ_size()); // First successor of Entry is the switch CFGBlock *SwitchBlock = *Entry.succ_begin(); - EXPECT_EQ(3u, SwitchBlock->succ_size()); + ASSERT_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; + Options.AssumeReachableDefaultInSwitchStatements = false; B = BuildCFG(Code, Options); EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); const auto &Entry2 = B.getCFG()->getEntry(); - EXPECT_EQ(1u, Entry2.succ_size()); + ASSERT_EQ(1u, Entry2.succ_size()); CFGBlock *SwitchBlock2 = *Entry2.succ_begin(); - EXPECT_EQ(3u, SwitchBlock2->succ_size()); + ASSERT_EQ(3u, SwitchBlock2->succ_size()); auto NoCaseSucc2 = SwitchBlock2->succ_rbegin(); EXPECT_FALSE(NoCaseSucc2->isReachable()); } @@ -181,7 +182,7 @@ TEST(CFG, SwitchCoveredEnumWithDefault) { } )"; CFG::BuildOptions Options; - Options.SwitchKeepDefaultCoveredEnum = true; + Options.AssumeReachableDefaultInSwitchStatements = true; BuildResult B = BuildCFG(Code, Options); EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); @@ -224,23 +225,23 @@ TEST(CFG, SwitchCoveredEnumWithDefault) { // Preds (4): B1 B3 B4 B5 const auto &Entry = B.getCFG()->getEntry(); - EXPECT_EQ(1u, Entry.succ_size()); + ASSERT_EQ(1u, Entry.succ_size()); // First successor of Entry is the switch CFGBlock *SwitchBlock = *Entry.succ_begin(); - EXPECT_EQ(3u, SwitchBlock->succ_size()); + ASSERT_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; + Options.AssumeReachableDefaultInSwitchStatements = false; B = BuildCFG(Code, Options); EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); const auto &Entry2 = B.getCFG()->getEntry(); - EXPECT_EQ(1u, Entry2.succ_size()); + ASSERT_EQ(1u, Entry2.succ_size()); CFGBlock *SwitchBlock2 = *Entry2.succ_begin(); - EXPECT_EQ(3u, SwitchBlock2->succ_size()); + ASSERT_EQ(3u, SwitchBlock2->succ_size()); auto defaultBlock2 = SwitchBlock2->succ_rbegin(); EXPECT_FALSE(defaultBlock2->isReachable()); } From 87c8c8e6b989114d5dda3266e9c6a9282366ffe3 Mon Sep 17 00:00:00 2001 From: Fred Tingaud Date: Thu, 2 Oct 2025 16:42:17 +0200 Subject: [PATCH 3/3] Apply more suggestions --- clang/unittests/Analysis/CFGTest.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/unittests/Analysis/CFGTest.cpp b/clang/unittests/Analysis/CFGTest.cpp index aeeff6b723532..6aa09a8ff61a3 100644 --- a/clang/unittests/Analysis/CFGTest.cpp +++ b/clang/unittests/Analysis/CFGTest.cpp @@ -109,7 +109,7 @@ TEST(CFG, SwitchCoveredEnumNoDefault) { CFG::BuildOptions Options; Options.AssumeReachableDefaultInSwitchStatements = true; BuildResult B = BuildCFG(Code, Options); - EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); + ASSERT_EQ(BuildResult::BuiltCFG, B.getStatus()); // [B5 (ENTRY)] // Succs (1): B2 @@ -156,7 +156,7 @@ TEST(CFG, SwitchCoveredEnumNoDefault) { // Checking that the same node is Unreachable without this setting Options.AssumeReachableDefaultInSwitchStatements = false; B = BuildCFG(Code, Options); - EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); + ASSERT_EQ(BuildResult::BuiltCFG, B.getStatus()); const auto &Entry2 = B.getCFG()->getEntry(); ASSERT_EQ(1u, Entry2.succ_size()); @@ -184,7 +184,7 @@ TEST(CFG, SwitchCoveredEnumWithDefault) { CFG::BuildOptions Options; Options.AssumeReachableDefaultInSwitchStatements = true; BuildResult B = BuildCFG(Code, Options); - EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); + ASSERT_EQ(BuildResult::BuiltCFG, B.getStatus()); // [B6 (ENTRY)] // Succs (1): B2 @@ -236,7 +236,7 @@ TEST(CFG, SwitchCoveredEnumWithDefault) { // Checking that the same node is Unreachable without this setting Options.AssumeReachableDefaultInSwitchStatements = false; B = BuildCFG(Code, Options); - EXPECT_EQ(BuildResult::BuiltCFG, B.getStatus()); + ASSERT_EQ(BuildResult::BuiltCFG, B.getStatus()); const auto &Entry2 = B.getCFG()->getEntry(); ASSERT_EQ(1u, Entry2.succ_size());