llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-driver Author: Maksim Ivanov (emaxx-google) <details> <summary>Changes</summary> 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 --- Full diff: https://github.com/llvm/llvm-project/pull/198041.diff 10 Files Affected: - (modified) clang/include/clang/AST/Stmt.h (+13) - (modified) clang/include/clang/Analysis/CFG.h (+5-1) - (modified) clang/include/clang/Basic/LangOptions.def (+1) - (modified) clang/include/clang/Options/Options.td (+5) - (modified) clang/lib/AST/Stmt.cpp (+2) - (modified) clang/lib/Analysis/CFG.cpp (+10-3) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+8) - (modified) clang/lib/Sema/SemaStmt.cpp (+60-1) - (added) clang/test/SemaCXX/warn-return-type-exhaustive-enum.cpp (+138) ``````````diff 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; + } +} `````````` </details> https://github.com/llvm/llvm-project/pull/198041 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
