[clang] Strict enum coverage check in switches#198041
Open
emaxx-google wants to merge 3 commits into
Open
Conversation
Add a new option "-fstrict-enum-switch-coverage": when checking whether a switch over enums handles all cases, the enum's underlying type range is used instead of only considering defined enumerators. For example, enabling this option allows to diagnose a missing return when a switch covers all defined enumerators but not all integers from the underlying type's range.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvmorg-github-actions
Bot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Maksim Ivanov (emaxx-google) ChangesAdd a new option "-fstrict-enum-switch-coverage": when checking whether a switch over enums handles all cases, the enum's underlying type range is used instead of only considering defined enumerators. For example, enabling this option allows to diagnose a missing return when a switch covers all defined enumerators but not all integers from the underlying type's range. Fixes: #123153 Assisted-by: Gemini Full diff: https://github.com/llvm/llvm-project/pull/198041.diff 10 Files Affected:
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index d940aa6562c4c..2fe5015d733ed 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -228,6 +228,11 @@ class alignas(void *) Stmt {
LLVM_PREFERRED_TYPE(bool)
unsigned AllEnumCasesCovered : 1;
+ /// True if all possible values of the underlying type are covered by
+ /// 'case' labels, independent of any 'default:' label.
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned SwitchCasesExhaustive : 1;
+
/// The location of the "switch".
SourceLocation SwitchLoc;
};
@@ -2676,6 +2681,14 @@ class SwitchStmt final : public Stmt,
return SwitchStmtBits.AllEnumCasesCovered;
}
+ void setSwitchCasesExhaustive() {
+ SwitchStmtBits.SwitchCasesExhaustive = true;
+ }
+
+ bool areSwitchCasesExhaustive() const {
+ return SwitchStmtBits.SwitchCasesExhaustive;
+ }
+
SourceLocation getBeginLoc() const { return getSwitchLoc(); }
SourceLocation getEndLoc() const LLVM_READONLY {
return getBody() ? getBody()->getEndLoc()
diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
index 72b7ac013ccc2..d98da9a6d1fc0 100644
--- a/clang/include/clang/Analysis/CFG.h
+++ b/clang/include/clang/Analysis/CFG.h
@@ -1045,9 +1045,12 @@ class CFGBlock {
unsigned IgnoreNullPredecessors : 1;
LLVM_PREFERRED_TYPE(bool)
unsigned IgnoreDefaultsWithCoveredEnums : 1;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned StrictEnumSwitchCoverage : 1;
FilterOptions()
- : IgnoreNullPredecessors(1), IgnoreDefaultsWithCoveredEnums(0) {}
+ : IgnoreNullPredecessors(1), IgnoreDefaultsWithCoveredEnums(0),
+ StrictEnumSwitchCoverage(0) {}
};
static bool FilterEdge(const FilterOptions &F, const CFGBlock *Src,
@@ -1285,6 +1288,7 @@ class CFG {
bool AddVirtualBaseBranches = false;
bool OmitImplicitValueInitializers = false;
bool AssumeReachableDefaultInSwitchStatements = false;
+ bool ForceStrictEnumSwitchCoverage = false;
BuildOptions() = default;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 596bce9e897f7..1f7eb4b177439 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -210,6 +210,7 @@ ENUM_LANGOPT(MSPointerToMemberRepresentationMethod, PragmaMSPointersToMembersKin
ENUM_LANGOPT(DefaultCallingConv, DefaultCallingConvention, 3, DCC_None, NotCompatible, "default calling convention")
LANGOPT(ShortEnums , 1, 0, NotCompatible, "short enum types")
+LANGOPT(StrictEnumSwitchCoverage, 1, 0, Benign, "Strict enum switch coverage")
LANGOPT(OpenCL , 1, 0, NotCompatible, "OpenCL")
LANGOPT(OpenCLVersion , 32, 0, NotCompatible, "OpenCL C version")
diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td
index 5fdc60adc8986..ff14db9ff85a8 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -4482,6 +4482,11 @@ def fstrict_enums : Flag<["-"], "fstrict-enums">, Group<f_Group>,
HelpText<"Enable optimizations based on the strict definition of an enum's "
"value range">,
MarshallingInfoFlag<CodeGenOpts<"StrictEnums">>;
+defm strict_enum_switch_coverage : BoolFOption<"strict-enum-switch-coverage",
+ LangOpts<"StrictEnumSwitchCoverage">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option], "Enable strict coverage checking for enum switch statements based on the underlying type's value range">,
+ NegFlag<SetFalse, [], [ClangOption, CC1Option]>>;
+
defm strict_vtable_pointers : BoolFOption<"strict-vtable-pointers",
CodeGenOpts<"StrictVTablePointers">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption, CC1Option],
diff --git a/clang/lib/AST/Stmt.cpp b/clang/lib/AST/Stmt.cpp
index 15d0e6435aaf3..fd3fe0a826cfe 100644
--- a/clang/lib/AST/Stmt.cpp
+++ b/clang/lib/AST/Stmt.cpp
@@ -1146,6 +1146,7 @@ SwitchStmt::SwitchStmt(const ASTContext &Ctx, Stmt *Init, VarDecl *Var,
SwitchStmtBits.HasInit = HasInit;
SwitchStmtBits.HasVar = HasVar;
SwitchStmtBits.AllEnumCasesCovered = false;
+ SwitchStmtBits.SwitchCasesExhaustive = false;
setCond(Cond);
setBody(nullptr);
@@ -1162,6 +1163,7 @@ SwitchStmt::SwitchStmt(EmptyShell Empty, bool HasInit, bool HasVar)
SwitchStmtBits.HasInit = HasInit;
SwitchStmtBits.HasVar = HasVar;
SwitchStmtBits.AllEnumCasesCovered = false;
+ SwitchStmtBits.SwitchCasesExhaustive = false;
}
SwitchStmt *SwitchStmt::Create(const ASTContext &Ctx, Stmt *Init, VarDecl *Var,
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 1b4d10b72e249..1c19e658f57d4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4592,9 +4592,13 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) {
// BuildOpts.SwitchReqDefaultCoveredEnum is true.
bool SwitchAlwaysHasSuccessor = false;
SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
+
+ bool IsExhaustive = BuildOpts.ForceStrictEnumSwitchCoverage
+ ? Terminator->areSwitchCasesExhaustive()
+ : Terminator->isAllEnumCasesCovered();
SwitchAlwaysHasSuccessor |=
- !BuildOpts.AssumeReachableDefaultInSwitchStatements &&
- Terminator->isAllEnumCasesCovered() && Terminator->getSwitchCaseList();
+ !BuildOpts.AssumeReachableDefaultInSwitchStatements && IsExhaustive &&
+ Terminator->getSwitchCaseList();
addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
!SwitchAlwaysHasSuccessor);
@@ -5612,7 +5616,10 @@ bool CFGBlock::FilterEdge(const CFGBlock::FilterOptions &F,
// CaseStmt then filter this edge.
if (const SwitchStmt *S =
dyn_cast_or_null<SwitchStmt>(From->getTerminatorStmt())) {
- if (S->isAllEnumCasesCovered()) {
+ bool IsExhaustive = F.StrictEnumSwitchCoverage
+ ? S->areSwitchCasesExhaustive()
+ : S->isAllEnumCasesCovered();
+ if (IsExhaustive) {
const Stmt *L = To->getLabel();
if (!L || !isa<CaseStmt>(L))
return true;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 92b3045dceff2..4934fa851b144 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5942,6 +5942,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
options::OPT_fno_allow_editor_placeholders);
Args.addOptInFlag(CmdArgs, options::OPT_fstrict_vtable_pointers,
options::OPT_fno_strict_vtable_pointers);
+ Args.addOptInFlag(CmdArgs, options::OPT_fstrict_enum_switch_coverage,
+ options::OPT_fno_strict_enum_switch_coverage);
Args.addOptInFlag(CmdArgs, options::OPT_fforce_emit_vtables,
options::OPT_fno_force_emit_vtables);
Args.addOptOutFlag(CmdArgs, options::OPT_foptimize_sibling_calls,
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 2b30aad880a27..3b4f8ea55cc34 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -607,6 +607,8 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
// enums in a switch(X) have explicit case statements.
CFGBlock::FilterOptions FO;
FO.IgnoreDefaultsWithCoveredEnums = 1;
+ FO.StrictEnumSwitchCoverage =
+ AC.getASTContext().getLangOpts().StrictEnumSwitchCoverage;
for (CFGBlock::filtered_pred_iterator I =
cfg->getExit().filtered_pred_start_end(FO);
@@ -2843,6 +2845,8 @@ void sema::AnalysisBasedWarnings::issueWarningsForRegisteredVarDecl(
AC.getCFGBuildOptions().AddTemporaryDtors = true;
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
auto Range = VarDeclPossiblyUnreachableDiags.equal_range(VD);
auto SecondRange =
@@ -2927,6 +2931,8 @@ LifetimeSafetyTUAnalysis(Sema &S, TranslationUnitDecl *TU,
AC.getCFGBuildOptions().AddParameterLifetimes = true;
AC.getCFGBuildOptions().AddInitializers = true;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
AC.getCFGBuildOptions().setAllAlwaysAdd();
if (AC.getCFG())
runLifetimeSafetyAnalysis(AC, &SemaHelper, LSStats, S.CollectStats);
@@ -3037,6 +3043,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
AC.getCFGBuildOptions().AddTemporaryDtors = true;
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
bool EnableLifetimeSafetyAnalysis =
S.getLangOpts().EnableLifetimeSafety &&
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 531147ef35b08..7ef469acbe184 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1292,6 +1292,57 @@ static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond,
<< Case->getSourceRange();
}
+static bool areSwitchCasesExhaustive(
+ unsigned CondWidthBeforePromotion, bool CondIsSignedBeforePromotion,
+ unsigned CondWidth, ArrayRef<std::pair<llvm::APSInt, CaseStmt *>> CaseVals,
+ ArrayRef<std::pair<llvm::APSInt, CaseStmt *>> CaseRanges,
+ ArrayRef<llvm::APSInt> HiVals) {
+ // +2 bits: 1 bit to ensure unsigned max values are treated as positive
+ // signed integers, and 1 bit to prevent overflow when evaluating ++Current
+ // at the maximum value.
+ unsigned CheckWidth = std::max(CondWidthBeforePromotion, CondWidth) + 2;
+ llvm::APSInt Min = llvm::APSInt::getMinValue(CondWidthBeforePromotion,
+ !CondIsSignedBeforePromotion);
+ llvm::APSInt Max = llvm::APSInt::getMaxValue(CondWidthBeforePromotion,
+ !CondIsSignedBeforePromotion);
+ Min = Min.extOrTrunc(CheckWidth);
+ Min.setIsSigned(true);
+ Max = Max.extOrTrunc(CheckWidth);
+ Max.setIsSigned(true);
+
+ llvm::APSInt Current = Min;
+ auto VI = CaseVals.begin(), VE = CaseVals.end();
+ auto RI = CaseRanges.begin(), RE = CaseRanges.end();
+ auto HI = HiVals.begin();
+
+ while (VI != VE || RI != RE) {
+ llvm::APSInt First, Last;
+ if (VI != VE && (RI == RE || VI->first < RI->first)) {
+ First = VI->first;
+ Last = VI->first;
+ ++VI;
+ } else {
+ First = RI->first;
+ Last = *HI;
+ ++RI;
+ ++HI;
+ }
+
+ First = First.extOrTrunc(CheckWidth);
+ First.setIsSigned(true);
+ Last = Last.extOrTrunc(CheckWidth);
+ Last.setIsSigned(true);
+
+ if (Current < First)
+ break;
+ if (Current <= Last) {
+ Current = Last;
+ ++Current;
+ }
+ }
+ return Current > Max;
+}
+
StmtResult
Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
Stmt *BodyStmt) {
@@ -1484,13 +1535,13 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
// Detect duplicate case ranges, which usually don't exist at all in
// the first place.
+ std::vector<llvm::APSInt> HiVals;
if (!CaseRanges.empty()) {
// Sort all the case ranges by their low value so we can easily detect
// overlaps between ranges.
llvm::stable_sort(CaseRanges);
// Scan the ranges, computing the high values and removing empty ranges.
- std::vector<llvm::APSInt> HiVals;
for (unsigned i = 0, e = CaseRanges.size(); i != e; ++i) {
llvm::APSInt &LoVal = CaseRanges[i].first;
CaseStmt *CR = CaseRanges[i].second;
@@ -1710,6 +1761,14 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
SS->setAllEnumCasesCovered();
}
enum_out:;
+
+ if (!CaseListIsErroneous && !CaseListIsIncomplete && !HasConstantCond &&
+ CondWidthBeforePromotion > 0) {
+ if (areSwitchCasesExhaustive(CondWidthBeforePromotion,
+ CondIsSignedBeforePromotion, CondWidth,
+ CaseVals, CaseRanges, HiVals))
+ SS->setSwitchCasesExhaustive();
+ }
}
if (BodyStmt)
diff --git a/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp b/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp
new file mode 100644
index 0000000000000..dc1dee0dbb99c
--- /dev/null
+++ b/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp
@@ -0,0 +1,138 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -verify=default,expected %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -fno-strict-enum-switch-coverage -verify=default,expected %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -fstrict-enum-switch-coverage -verify=strict,expected %s
+
+enum E { A, B };
+
+int test_missing_return(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+int test_unreachable(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ }
+ return 3;
+}
+
+void test_array_bounds(E e) {
+ int x[2]; // strict-note {{array 'x' declared here}}
+ switch (e) {
+ case A: return;
+ case B: return;
+ }
+ x[2] = 0; // strict-warning {{array index 2 is past the end of the array (that has type 'int[2]')}}
+}
+
+int test_useless_default_no_unreachable_warning(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ default: return 3;
+ }
+}
+
+int test_nested(E e1, E e2) {
+ switch (e1) {
+ case A:
+ switch (e2) {
+ case A: return 1;
+ case B: return 2;
+ }
+ break; // break out of outer switch if e2 falls through!
+ case B:
+ return 3;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+enum class BoolEnum : bool { False = false, True = true };
+
+int test_full_range_exhaustive(BoolEnum e) {
+ switch (e) {
+ case BoolEnum::False: return 0;
+ case BoolEnum::True: return 1;
+ }
+}
+
+int test_useless_default_full_range_exhaustive(BoolEnum e) {
+ switch (e) {
+ case BoolEnum::False: return 0;
+ case BoolEnum::True: return 1;
+ default: return 2;
+ }
+}
+
+int test_bool(bool b) {
+ switch (b) { // expected-warning {{switch condition has boolean value}}
+ case false: return 0;
+ case true: return 1;
+ }
+} // default-warning {{non-void function does not return a value in all control paths}}
+
+enum class SignedCharEnum : signed char { Min = -128, Max = 127 };
+
+int test_signed_char_exhaustive(SignedCharEnum e) {
+ switch (e) {
+ case SignedCharEnum::Min ... SignedCharEnum::Max: return 0;
+ }
+}
+
+enum class BoolEnumMissing : bool { False = false };
+
+int test_bool_enum_missing(BoolEnumMissing e) {
+ switch (e) {
+ case BoolEnumMissing::False: return 0;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+int test_uninit_exhaustive(E e) {
+ int x; // strict-note {{initialize the variable 'x' to silence this warning}}
+ switch (e) {
+ case A: x = 1; break;
+ case B: x = 2; break;
+ }
+ return x; // strict-warning {{variable 'x' may be uninitialized when used here}}
+}
+
+int test_uninit_missing(BoolEnumMissing e) {
+ int x; // strict-note {{initialize the variable 'x' to silence this warning}}
+ switch (e) {
+ case BoolEnumMissing::False: x = 1; break;
+ }
+ return x; // strict-warning {{variable 'x' may be uninitialized when used here}}
+}
+
+enum EmptyEnum {};
+
+int test_empty_enum(EmptyEnum e) {
+ switch (e) {
+ }
+} // expected-warning {{non-void function does not return a value}}
+
+int test_empty_switch_bool(bool b) {
+ switch (b) { // expected-warning {{switch condition has boolean value}}
+ }
+} // expected-warning {{non-void function does not return a value}}
+
+template <typename T>
+int test_template(T e) {
+ switch (e) {
+ case (T)0: return 1;
+ case (T)1: return 2;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+template int test_template<E>(E); // strict-note {{in instantiation of function template specialization 'test_template<E>' requested here}}
+template int test_template<BoolEnum>(BoolEnum);
+
+enum class U32 : unsigned int { Min = 0, Max = 0xFFFFFFFF };
+
+int test_overflow(U32 e) {
+ switch (e) {
+ case U32::Min ... U32::Max: return 0;
+ }
+}
|
|
@llvm/pr-subscribers-clang-analysis Author: Maksim Ivanov (emaxx-google) ChangesAdd a new option "-fstrict-enum-switch-coverage": when checking whether a switch over enums handles all cases, the enum's underlying type range is used instead of only considering defined enumerators. For example, enabling this option allows to diagnose a missing return when a switch covers all defined enumerators but not all integers from the underlying type's range. Fixes: #123153 Assisted-by: Gemini Full diff: https://github.com/llvm/llvm-project/pull/198041.diff 10 Files Affected:
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index d940aa6562c4c..2fe5015d733ed 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -228,6 +228,11 @@ class alignas(void *) Stmt {
LLVM_PREFERRED_TYPE(bool)
unsigned AllEnumCasesCovered : 1;
+ /// True if all possible values of the underlying type are covered by
+ /// 'case' labels, independent of any 'default:' label.
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned SwitchCasesExhaustive : 1;
+
/// The location of the "switch".
SourceLocation SwitchLoc;
};
@@ -2676,6 +2681,14 @@ class SwitchStmt final : public Stmt,
return SwitchStmtBits.AllEnumCasesCovered;
}
+ void setSwitchCasesExhaustive() {
+ SwitchStmtBits.SwitchCasesExhaustive = true;
+ }
+
+ bool areSwitchCasesExhaustive() const {
+ return SwitchStmtBits.SwitchCasesExhaustive;
+ }
+
SourceLocation getBeginLoc() const { return getSwitchLoc(); }
SourceLocation getEndLoc() const LLVM_READONLY {
return getBody() ? getBody()->getEndLoc()
diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h
index 72b7ac013ccc2..d98da9a6d1fc0 100644
--- a/clang/include/clang/Analysis/CFG.h
+++ b/clang/include/clang/Analysis/CFG.h
@@ -1045,9 +1045,12 @@ class CFGBlock {
unsigned IgnoreNullPredecessors : 1;
LLVM_PREFERRED_TYPE(bool)
unsigned IgnoreDefaultsWithCoveredEnums : 1;
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned StrictEnumSwitchCoverage : 1;
FilterOptions()
- : IgnoreNullPredecessors(1), IgnoreDefaultsWithCoveredEnums(0) {}
+ : IgnoreNullPredecessors(1), IgnoreDefaultsWithCoveredEnums(0),
+ StrictEnumSwitchCoverage(0) {}
};
static bool FilterEdge(const FilterOptions &F, const CFGBlock *Src,
@@ -1285,6 +1288,7 @@ class CFG {
bool AddVirtualBaseBranches = false;
bool OmitImplicitValueInitializers = false;
bool AssumeReachableDefaultInSwitchStatements = false;
+ bool ForceStrictEnumSwitchCoverage = false;
BuildOptions() = default;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 596bce9e897f7..1f7eb4b177439 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -210,6 +210,7 @@ ENUM_LANGOPT(MSPointerToMemberRepresentationMethod, PragmaMSPointersToMembersKin
ENUM_LANGOPT(DefaultCallingConv, DefaultCallingConvention, 3, DCC_None, NotCompatible, "default calling convention")
LANGOPT(ShortEnums , 1, 0, NotCompatible, "short enum types")
+LANGOPT(StrictEnumSwitchCoverage, 1, 0, Benign, "Strict enum switch coverage")
LANGOPT(OpenCL , 1, 0, NotCompatible, "OpenCL")
LANGOPT(OpenCLVersion , 32, 0, NotCompatible, "OpenCL C version")
diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td
index 5fdc60adc8986..ff14db9ff85a8 100644
--- a/clang/include/clang/Options/Options.td
+++ b/clang/include/clang/Options/Options.td
@@ -4482,6 +4482,11 @@ def fstrict_enums : Flag<["-"], "fstrict-enums">, Group<f_Group>,
HelpText<"Enable optimizations based on the strict definition of an enum's "
"value range">,
MarshallingInfoFlag<CodeGenOpts<"StrictEnums">>;
+defm strict_enum_switch_coverage : BoolFOption<"strict-enum-switch-coverage",
+ LangOpts<"StrictEnumSwitchCoverage">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option], "Enable strict coverage checking for enum switch statements based on the underlying type's value range">,
+ NegFlag<SetFalse, [], [ClangOption, CC1Option]>>;
+
defm strict_vtable_pointers : BoolFOption<"strict-vtable-pointers",
CodeGenOpts<"StrictVTablePointers">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption, CC1Option],
diff --git a/clang/lib/AST/Stmt.cpp b/clang/lib/AST/Stmt.cpp
index 15d0e6435aaf3..fd3fe0a826cfe 100644
--- a/clang/lib/AST/Stmt.cpp
+++ b/clang/lib/AST/Stmt.cpp
@@ -1146,6 +1146,7 @@ SwitchStmt::SwitchStmt(const ASTContext &Ctx, Stmt *Init, VarDecl *Var,
SwitchStmtBits.HasInit = HasInit;
SwitchStmtBits.HasVar = HasVar;
SwitchStmtBits.AllEnumCasesCovered = false;
+ SwitchStmtBits.SwitchCasesExhaustive = false;
setCond(Cond);
setBody(nullptr);
@@ -1162,6 +1163,7 @@ SwitchStmt::SwitchStmt(EmptyShell Empty, bool HasInit, bool HasVar)
SwitchStmtBits.HasInit = HasInit;
SwitchStmtBits.HasVar = HasVar;
SwitchStmtBits.AllEnumCasesCovered = false;
+ SwitchStmtBits.SwitchCasesExhaustive = false;
}
SwitchStmt *SwitchStmt::Create(const ASTContext &Ctx, Stmt *Init, VarDecl *Var,
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 1b4d10b72e249..1c19e658f57d4 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4592,9 +4592,13 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) {
// BuildOpts.SwitchReqDefaultCoveredEnum is true.
bool SwitchAlwaysHasSuccessor = false;
SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
+
+ bool IsExhaustive = BuildOpts.ForceStrictEnumSwitchCoverage
+ ? Terminator->areSwitchCasesExhaustive()
+ : Terminator->isAllEnumCasesCovered();
SwitchAlwaysHasSuccessor |=
- !BuildOpts.AssumeReachableDefaultInSwitchStatements &&
- Terminator->isAllEnumCasesCovered() && Terminator->getSwitchCaseList();
+ !BuildOpts.AssumeReachableDefaultInSwitchStatements && IsExhaustive &&
+ Terminator->getSwitchCaseList();
addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
!SwitchAlwaysHasSuccessor);
@@ -5612,7 +5616,10 @@ bool CFGBlock::FilterEdge(const CFGBlock::FilterOptions &F,
// CaseStmt then filter this edge.
if (const SwitchStmt *S =
dyn_cast_or_null<SwitchStmt>(From->getTerminatorStmt())) {
- if (S->isAllEnumCasesCovered()) {
+ bool IsExhaustive = F.StrictEnumSwitchCoverage
+ ? S->areSwitchCasesExhaustive()
+ : S->isAllEnumCasesCovered();
+ if (IsExhaustive) {
const Stmt *L = To->getLabel();
if (!L || !isa<CaseStmt>(L))
return true;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 92b3045dceff2..4934fa851b144 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5942,6 +5942,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
options::OPT_fno_allow_editor_placeholders);
Args.addOptInFlag(CmdArgs, options::OPT_fstrict_vtable_pointers,
options::OPT_fno_strict_vtable_pointers);
+ Args.addOptInFlag(CmdArgs, options::OPT_fstrict_enum_switch_coverage,
+ options::OPT_fno_strict_enum_switch_coverage);
Args.addOptInFlag(CmdArgs, options::OPT_fforce_emit_vtables,
options::OPT_fno_force_emit_vtables);
Args.addOptOutFlag(CmdArgs, options::OPT_foptimize_sibling_calls,
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 2b30aad880a27..3b4f8ea55cc34 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -607,6 +607,8 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
// enums in a switch(X) have explicit case statements.
CFGBlock::FilterOptions FO;
FO.IgnoreDefaultsWithCoveredEnums = 1;
+ FO.StrictEnumSwitchCoverage =
+ AC.getASTContext().getLangOpts().StrictEnumSwitchCoverage;
for (CFGBlock::filtered_pred_iterator I =
cfg->getExit().filtered_pred_start_end(FO);
@@ -2843,6 +2845,8 @@ void sema::AnalysisBasedWarnings::issueWarningsForRegisteredVarDecl(
AC.getCFGBuildOptions().AddTemporaryDtors = true;
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
auto Range = VarDeclPossiblyUnreachableDiags.equal_range(VD);
auto SecondRange =
@@ -2927,6 +2931,8 @@ LifetimeSafetyTUAnalysis(Sema &S, TranslationUnitDecl *TU,
AC.getCFGBuildOptions().AddParameterLifetimes = true;
AC.getCFGBuildOptions().AddInitializers = true;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
AC.getCFGBuildOptions().setAllAlwaysAdd();
if (AC.getCFG())
runLifetimeSafetyAnalysis(AC, &SemaHelper, LSStats, S.CollectStats);
@@ -3037,6 +3043,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
AC.getCFGBuildOptions().AddTemporaryDtors = true;
AC.getCFGBuildOptions().AddCXXNewAllocator = false;
AC.getCFGBuildOptions().AddCXXDefaultInitExprInCtors = true;
+ AC.getCFGBuildOptions().ForceStrictEnumSwitchCoverage =
+ S.getLangOpts().StrictEnumSwitchCoverage;
bool EnableLifetimeSafetyAnalysis =
S.getLangOpts().EnableLifetimeSafety &&
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 531147ef35b08..7ef469acbe184 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -1292,6 +1292,57 @@ static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond,
<< Case->getSourceRange();
}
+static bool areSwitchCasesExhaustive(
+ unsigned CondWidthBeforePromotion, bool CondIsSignedBeforePromotion,
+ unsigned CondWidth, ArrayRef<std::pair<llvm::APSInt, CaseStmt *>> CaseVals,
+ ArrayRef<std::pair<llvm::APSInt, CaseStmt *>> CaseRanges,
+ ArrayRef<llvm::APSInt> HiVals) {
+ // +2 bits: 1 bit to ensure unsigned max values are treated as positive
+ // signed integers, and 1 bit to prevent overflow when evaluating ++Current
+ // at the maximum value.
+ unsigned CheckWidth = std::max(CondWidthBeforePromotion, CondWidth) + 2;
+ llvm::APSInt Min = llvm::APSInt::getMinValue(CondWidthBeforePromotion,
+ !CondIsSignedBeforePromotion);
+ llvm::APSInt Max = llvm::APSInt::getMaxValue(CondWidthBeforePromotion,
+ !CondIsSignedBeforePromotion);
+ Min = Min.extOrTrunc(CheckWidth);
+ Min.setIsSigned(true);
+ Max = Max.extOrTrunc(CheckWidth);
+ Max.setIsSigned(true);
+
+ llvm::APSInt Current = Min;
+ auto VI = CaseVals.begin(), VE = CaseVals.end();
+ auto RI = CaseRanges.begin(), RE = CaseRanges.end();
+ auto HI = HiVals.begin();
+
+ while (VI != VE || RI != RE) {
+ llvm::APSInt First, Last;
+ if (VI != VE && (RI == RE || VI->first < RI->first)) {
+ First = VI->first;
+ Last = VI->first;
+ ++VI;
+ } else {
+ First = RI->first;
+ Last = *HI;
+ ++RI;
+ ++HI;
+ }
+
+ First = First.extOrTrunc(CheckWidth);
+ First.setIsSigned(true);
+ Last = Last.extOrTrunc(CheckWidth);
+ Last.setIsSigned(true);
+
+ if (Current < First)
+ break;
+ if (Current <= Last) {
+ Current = Last;
+ ++Current;
+ }
+ }
+ return Current > Max;
+}
+
StmtResult
Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
Stmt *BodyStmt) {
@@ -1484,13 +1535,13 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
// Detect duplicate case ranges, which usually don't exist at all in
// the first place.
+ std::vector<llvm::APSInt> HiVals;
if (!CaseRanges.empty()) {
// Sort all the case ranges by their low value so we can easily detect
// overlaps between ranges.
llvm::stable_sort(CaseRanges);
// Scan the ranges, computing the high values and removing empty ranges.
- std::vector<llvm::APSInt> HiVals;
for (unsigned i = 0, e = CaseRanges.size(); i != e; ++i) {
llvm::APSInt &LoVal = CaseRanges[i].first;
CaseStmt *CR = CaseRanges[i].second;
@@ -1710,6 +1761,14 @@ Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
SS->setAllEnumCasesCovered();
}
enum_out:;
+
+ if (!CaseListIsErroneous && !CaseListIsIncomplete && !HasConstantCond &&
+ CondWidthBeforePromotion > 0) {
+ if (areSwitchCasesExhaustive(CondWidthBeforePromotion,
+ CondIsSignedBeforePromotion, CondWidth,
+ CaseVals, CaseRanges, HiVals))
+ SS->setSwitchCasesExhaustive();
+ }
}
if (BodyStmt)
diff --git a/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp b/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp
new file mode 100644
index 0000000000000..dc1dee0dbb99c
--- /dev/null
+++ b/clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp
@@ -0,0 +1,138 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -verify=default,expected %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -fno-strict-enum-switch-coverage -verify=default,expected %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-type -Wunreachable-code -Warray-bounds -Wconditional-uninitialized -fstrict-enum-switch-coverage -verify=strict,expected %s
+
+enum E { A, B };
+
+int test_missing_return(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+int test_unreachable(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ }
+ return 3;
+}
+
+void test_array_bounds(E e) {
+ int x[2]; // strict-note {{array 'x' declared here}}
+ switch (e) {
+ case A: return;
+ case B: return;
+ }
+ x[2] = 0; // strict-warning {{array index 2 is past the end of the array (that has type 'int[2]')}}
+}
+
+int test_useless_default_no_unreachable_warning(E e) {
+ switch (e) {
+ case A: return 1;
+ case B: return 2;
+ default: return 3;
+ }
+}
+
+int test_nested(E e1, E e2) {
+ switch (e1) {
+ case A:
+ switch (e2) {
+ case A: return 1;
+ case B: return 2;
+ }
+ break; // break out of outer switch if e2 falls through!
+ case B:
+ return 3;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+enum class BoolEnum : bool { False = false, True = true };
+
+int test_full_range_exhaustive(BoolEnum e) {
+ switch (e) {
+ case BoolEnum::False: return 0;
+ case BoolEnum::True: return 1;
+ }
+}
+
+int test_useless_default_full_range_exhaustive(BoolEnum e) {
+ switch (e) {
+ case BoolEnum::False: return 0;
+ case BoolEnum::True: return 1;
+ default: return 2;
+ }
+}
+
+int test_bool(bool b) {
+ switch (b) { // expected-warning {{switch condition has boolean value}}
+ case false: return 0;
+ case true: return 1;
+ }
+} // default-warning {{non-void function does not return a value in all control paths}}
+
+enum class SignedCharEnum : signed char { Min = -128, Max = 127 };
+
+int test_signed_char_exhaustive(SignedCharEnum e) {
+ switch (e) {
+ case SignedCharEnum::Min ... SignedCharEnum::Max: return 0;
+ }
+}
+
+enum class BoolEnumMissing : bool { False = false };
+
+int test_bool_enum_missing(BoolEnumMissing e) {
+ switch (e) {
+ case BoolEnumMissing::False: return 0;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+int test_uninit_exhaustive(E e) {
+ int x; // strict-note {{initialize the variable 'x' to silence this warning}}
+ switch (e) {
+ case A: x = 1; break;
+ case B: x = 2; break;
+ }
+ return x; // strict-warning {{variable 'x' may be uninitialized when used here}}
+}
+
+int test_uninit_missing(BoolEnumMissing e) {
+ int x; // strict-note {{initialize the variable 'x' to silence this warning}}
+ switch (e) {
+ case BoolEnumMissing::False: x = 1; break;
+ }
+ return x; // strict-warning {{variable 'x' may be uninitialized when used here}}
+}
+
+enum EmptyEnum {};
+
+int test_empty_enum(EmptyEnum e) {
+ switch (e) {
+ }
+} // expected-warning {{non-void function does not return a value}}
+
+int test_empty_switch_bool(bool b) {
+ switch (b) { // expected-warning {{switch condition has boolean value}}
+ }
+} // expected-warning {{non-void function does not return a value}}
+
+template <typename T>
+int test_template(T e) {
+ switch (e) {
+ case (T)0: return 1;
+ case (T)1: return 2;
+ }
+} // strict-warning {{non-void function does not return a value in all control paths}}
+
+template int test_template<E>(E); // strict-note {{in instantiation of function template specialization 'test_template<E>' requested here}}
+template int test_template<BoolEnum>(BoolEnum);
+
+enum class U32 : unsigned int { Min = 0, Max = 0xFFFFFFFF };
+
+int test_overflow(U32 e) {
+ switch (e) {
+ case U32::Min ... U32::Max: return 0;
+ }
+}
|
Contributor
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
If we can add a warning flag, instead of a "-f" dialect flag, I think that would be better. It would allow using existing pragmas to control the warning, so you could enable/disable it in specific regions. Not sure how hard that would be.
As an alternative, maybe we can tweak the existing behavior so that we treat switches with an explicit "default" differently from switches which don't have one. If you have a switch with a "default", it probably makes sense to assume the path through the "default" is reachable for the purpose of -Wreturn-type, even if all named enums values are covered. Then we add a flag -Wenum-switch-default so you can request warnings for switches over enums that don't have a "default".
The end result isn't exactly the same thing, but maybe sufficient for the specific coding style described.
| case false: return 0; | ||
| case true: return 1; | ||
| } | ||
| } // default-warning {{non-void function does not return a value in all control paths}} |
Contributor
There was a problem hiding this comment.
Why do we want a warning for a bool switch that covers all possible values?
Member
|
I agree that this should probably be a
Maybe I misunderstood and you’re suggesting the same thing, but if the point of this is primarily to diagnose a missing return, then my suggestion would be to just introduce an option that makes the CFG code assume that we can still fall off the end of a I’m not sure checking for all possible values of a type would be that useful because most people aren’t writing switch statements w/ |
Contributor
What you're describing is basically the current version of this PR: add a flag that flips how the CFG code handles switches. The point of my alternative suggestion was to avoid having two different analysis modes. Instead of flipping the handling based on a flag, flip it based on whether the switch has a "default". |
Contributor
Author
|
Thanks for the reviews, @efriedma-quic and @Sirraide!
This alternative sounds good to me. I've started a PR #198416 for the first part - assuming the "default" is reachable if present. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a new option "-fstrict-enum-switch-coverage": when checking whether a switch over enums handles all cases, the enum's underlying type range is used instead of only considering defined enumerators.
For example, enabling this option allows to diagnose a missing return when a switch covers all defined enumerators but not all integers from the underlying type's range.
Fixes: #123153
Assisted-by: Gemini