https://github.com/negativ updated https://github.com/llvm/llvm-project/pull/146355
>From fc3b77d8c4b5dd264bd746ed997bcea6cddaf389 Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 30 Jun 2025 17:05:41 +0300 Subject: [PATCH 1/8] Initial implementation --- .../bugprone/unchecked-optional-access.cpp | 36 +++++++++++ clang/include/clang/AST/Decl.h | 5 ++ clang/lib/AST/Decl.cpp | 4 ++ clang/lib/Analysis/CFG.cpp | 62 ++++++++++++++++++- .../TypeErasedDataflowAnalysis.cpp | 4 +- 5 files changed, 107 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp index 3167b85f0e024..f4f82199a0bb5 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp @@ -141,6 +141,42 @@ void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo } } +void assertion_handler_imp() __attribute__((analyzer_noreturn)); + +void assertion_handler() { + do { + assertion_handler_imp(); + } while(0); +} + +void function_calling_analyzer_noreturn(const bsl::optional<int>& opt) +{ + if (!opt) { + assertion_handler(); + } + + *opt; // no-warning +} + +void abort(); + +void do_fail() { + abort(); // acts like 'abort()' C-function +} + +void invoke_assertion_handler() { + do_fail(); +} + +void function_calling_well_known_noreturn(const bsl::optional<int>& opt) +{ + if (!opt) { + invoke_assertion_handler(); + } + + *opt; // no-warning +} + template <typename T> void function_template_without_user(const absl::optional<T> &opt) { opt.value(); // no-warning diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index c4202f1f3d07e..0b27cb7f2cb4e 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -2624,6 +2624,11 @@ class FunctionDecl : public DeclaratorDecl, /// an attribute on its declaration or its type. bool isNoReturn() const; + /// Determines whether this function is known to never return for CFG + /// analysis. Checks for noreturn attributes on the function declaration + /// or its type, including 'analyzer_noreturn' attribute. + bool isAnalyzerNoReturn() const; + /// True if the function was a definition but its body was skipped. bool hasSkippedBody() const { return FunctionDeclBits.HasSkippedBody; } void setHasSkippedBody(bool Skipped = true) { diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index 5cdf75d71e4d7..72f63978c085b 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -3603,6 +3603,10 @@ bool FunctionDecl::isNoReturn() const { return false; } +bool FunctionDecl::isAnalyzerNoReturn() const { + return isNoReturn() || hasAttr<AnalyzerNoReturnAttr>(); +} + bool FunctionDecl::isMemberLikeConstrainedFriend() const { // C++20 [temp.friend]p9: // A non-template friend declaration with a requires-clause [or] diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index cf7595952be27..02876e5770c2c 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2835,8 +2835,37 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) { if (!FD->isVariadic()) findConstructionContextsForArguments(C); - if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context)) - NoReturn = true; + if (!NoReturn) + NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context); + + // Some well-known 'noreturn' functions + if (!NoReturn) + NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString()) + .Case("BloombergLP::bsls::Assert::invokeHandler", true) + .Case("std::terminate", true) + .Case("std::abort", true) + .Case("exit", true) + .Case("abort", true) + .Case("panic", true) + .Case("error", true) + .Case("Assert", true) + .Case("ziperr", true) + .Case("assfail", true) + .Case("db_error", true) + .Case("__assert", true) + .Case("__assert2", true) + .Case("_wassert", true) + .Case("__assert_rtn", true) + .Case("__assert_fail", true) + .Case("dtrace_assfail", true) + .Case("yy_fatal_error", true) + .Case("_XCAssertionFailureHandler", true) + .Case("_DTAssertionFailureHandler", true) + .Case("_TSAssertionFailureHandler", true) + .Case("__builtin_trap", true) + .Case("__builtin_unreachable", true) + .Default(false); + if (FD->hasAttr<NoThrowAttr>()) AddEHEdge = false; if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) || @@ -6308,6 +6337,35 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) { })) return true; + auto HasNoReturnCall = [](const CallExpr *CE) { + if (!CE) + return false; + + auto *FD = CE->getDirectCallee(); + + if (!FD) + return false; + + auto NoReturnFromCFG = [FD]() { + if (!FD->getBody()) + return false; + + auto CalleeCFG = + CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {}); + + return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking(); + }; + + return FD->isAnalyzerNoReturn() || NoReturnFromCFG(); + }; + + if (llvm::any_of(*Blk, [&](const CFGElement &Elm) { + if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>()) + return HasNoReturnCall(dyn_cast<CallExpr>(StmtElm->getStmt())); + return false; + })) + return true; + return false; } diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 1113bbe7f4d9c..c799ca98e4a0d 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -283,7 +283,7 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) { JoinedStateBuilder Builder(AC, JoinBehavior); for (const CFGBlock *Pred : Preds) { // Skip if the `Block` is unreachable or control flow cannot get past it. - if (!Pred || Pred->hasNoReturnElement()) + if (!Pred || Pred->isInevitablySinking()) continue; // Skip if `Pred` was not evaluated yet. This could happen if `Pred` has a @@ -562,7 +562,7 @@ runTypeErasedDataflowAnalysis( BlockStates[Block->getBlockID()] = std::move(NewBlockState); // Do not add unreachable successor blocks to `Worklist`. - if (Block->hasNoReturnElement()) + if (Block->isInevitablySinking()) continue; Worklist.enqueueSuccessors(Block); >From 0b2e72df4761f41849c82070cbb1ec0fc0c18464 Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Fri, 4 Jul 2025 18:25:09 +0300 Subject: [PATCH 2/8] Unittests --- .../TypeErasedDataflowAnalysisTest.cpp | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 9fb7bebdbe41e..ac4588293bf09 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -49,6 +49,7 @@ using namespace ast_matchers; using llvm::IsStringMapEntry; using ::testing::DescribeMatcher; using ::testing::IsEmpty; +using ::testing::Not; using ::testing::NotNull; using ::testing::Test; using ::testing::UnorderedElementsAre; @@ -693,6 +694,87 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) { // FIXME: Called functions at point `p` should contain only "foo". } +class AnalyzerNoreturnTest : public Test { +protected: + template <typename Matcher> + void runDataflow(llvm::StringRef Code, Matcher Expectations) { + tooling::FileContentMappings FilesContents; + FilesContents.push_back(std::make_pair<std::string, std::string>( + "noreturn_test_defs.h", R"( + void assertionHandler() __attribute__((analyzer_noreturn)); + + void assertionTrampoline() { + assertionHandler(); + } + + void trap() {} + )")); + + ASSERT_THAT_ERROR( + test::checkDataflow<FunctionCallAnalysis>( + AnalysisInputs<FunctionCallAnalysis>( + Code, ast_matchers::hasName("target"), + [](ASTContext &C, Environment &) { + return FunctionCallAnalysis(C); + }) + .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}) + .withASTBuildVirtualMappedFiles(std::move(FilesContents)), + /*VerifyResults=*/ + [&Expectations]( + const llvm::StringMap< + DataflowAnalysisState<FunctionCallLattice>> &Results, + const AnalysisOutputs &) { + EXPECT_THAT(Results, Expectations); + }), + llvm::Succeeded()); + } +}; + +TEST_F(AnalyzerNoreturnTest, + Breathing) { + std::string Code = R"( + #include "noreturn_test_defs.h" + + void target() { + trap(); + // [[p]] + } + )"; + runDataflow(Code, UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap")))))); +} + +TEST_F(AnalyzerNoreturnTest, DirectNoReturnCall) { + std::string Code = R"( + #include "noreturn_test_defs.h" + + void target() { + assertionHandler(); + trap(); + // [[p]] + } + )"; + runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap"))))))); +} + +TEST_F(AnalyzerNoreturnTest, IndirectNoReturnCall) { + std::string Code = R"( + #include "noreturn_test_defs.h" + + void target() { + assertionTrampoline(); + trap(); + // [[p]] + } + )"; + runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap"))))))); +} + // Models an analysis that uses flow conditions. class SpecialBoolAnalysis final : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> { >From 06cbe1deec97d8756385eaee423d7e3bbb3f51b2 Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 7 Jul 2025 12:52:38 +0300 Subject: [PATCH 3/8] Crash fix & PR fixes --- clang/lib/Analysis/CFG.cpp | 19 +++-- .../Checkers/NoReturnFunctionChecker.cpp | 70 +++++++++++-------- .../TypeErasedDataflowAnalysisTest.cpp | 7 +- 3 files changed, 57 insertions(+), 39 deletions(-) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index fe99702a35d0d..bc47891216e7a 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -41,6 +41,7 @@ #include "llvm/ADT/APSInt.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallPtrSet.h" @@ -6317,6 +6318,12 @@ void CFGBlock::printTerminatorJson(raw_ostream &Out, const LangOptions &LO, // There may be many more reasons why a sink would appear during analysis // (eg. checkers may generate sinks arbitrarily), but here we only consider // sinks that would be obvious by looking at the CFG. +// +// This function also performs inter-procedural analysis by recursively +// examining called functions to detect forwarding chains to noreturn +// functions. When a function is determined to never return through this +// analysis, it's automatically marked with analyzer_noreturn attribute +// for caching and future reference. static bool isImmediateSinkBlock(const CFGBlock *Blk) { if (Blk->hasNoReturnElement()) return true; @@ -6327,10 +6334,9 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) { // at least for now, but once we have better support for exceptions, // we'd need to carefully handle the case when the throw is being // immediately caught. - if (llvm::any_of(*Blk, [](const CFGElement &Elm) { + if (llvm::any_of(*Blk, [](const CFGElement &Elm) -> bool { if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>()) - if (isa<CXXThrowExpr>(StmtElm->getStmt())) - return true; + return isa<CXXThrowExpr>(StmtElm->getStmt()); return false; })) return true; @@ -6339,11 +6345,16 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) { if (!CE) return false; + static thread_local llvm::SmallPtrSet<const FunctionDecl *, 32> InProgress; + auto *FD = CE->getDirectCallee(); - if (!FD) + if (!FD || InProgress.count(FD)) return false; + InProgress.insert(FD); + auto DoCleanup = llvm::make_scope_exit([&]() { InProgress.erase(FD); }); + auto NoReturnFromCFG = [FD]() { if (!FD->getBody()) return false; diff --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp index 17c3cb4e9e04c..834bd81c2fa21 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp @@ -50,37 +50,45 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent &CE, BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn(); } - if (!BuildSinks && CE.isGlobalCFunction()) { - if (const IdentifierInfo *II = CE.getCalleeIdentifier()) { - // HACK: Some functions are not marked noreturn, and don't return. - // Here are a few hardwired ones. If this takes too long, we can - // potentially cache these results. - BuildSinks - = llvm::StringSwitch<bool>(StringRef(II->getName())) - .Case("exit", true) - .Case("panic", true) - .Case("error", true) - .Case("Assert", true) - // FIXME: This is just a wrapper around throwing an exception. - // Eventually inter-procedural analysis should handle this easily. - .Case("ziperr", true) - .Case("assfail", true) - .Case("db_error", true) - .Case("__assert", true) - .Case("__assert2", true) - // For the purpose of static analysis, we do not care that - // this MSVC function will return if the user decides to continue. - .Case("_wassert", true) - .Case("__assert_rtn", true) - .Case("__assert_fail", true) - .Case("dtrace_assfail", true) - .Case("yy_fatal_error", true) - .Case("_XCAssertionFailureHandler", true) - .Case("_DTAssertionFailureHandler", true) - .Case("_TSAssertionFailureHandler", true) - .Default(false); - } - } + if (!BuildSinks && CE.isGlobalCFunction()) { + if (const IdentifierInfo *II = CE.getCalleeIdentifier()) { + // HACK: Some functions are not marked noreturn, and don't return. + // Here are a few hardwired ones. If this takes too long, we can + // potentially cache these results. + // + // (!) In case of function list update, please also update + // CFGBuilder::VisitCallExpr (CFG.cpp) + BuildSinks = + llvm::StringSwitch<bool>(StringRef(II->getName())) + .Case("exit", true) + .Case("abort", true) + .Case("panic", true) + .Case("error", true) + .Case("Assert", true) + // FIXME: This is just a wrapper around throwing an exception. + // Eventually inter-procedural analysis should handle this + // easily. + .Case("ziperr", true) + .Case("assfail", true) + .Case("db_error", true) + .Case("__assert", true) + .Case("__assert2", true) + // For the purpose of static analysis, we do not care that + // this MSVC function will return if the user decides to + // continue. + .Case("_wassert", true) + .Case("__assert_rtn", true) + .Case("__assert_fail", true) + .Case("dtrace_assfail", true) + .Case("yy_fatal_error", true) + .Case("_XCAssertionFailureHandler", true) + .Case("_DTAssertionFailureHandler", true) + .Case("_TSAssertionFailureHandler", true) + .Case("__builtin_trap", true) + .Case("__builtin_unreachable", true) + .Default(false); + } + } if (BuildSinks) C.generateSink(C.getState(), C.getPredecessor()); diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index ac4588293bf09..9150b3e155533 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -699,8 +699,8 @@ class AnalyzerNoreturnTest : public Test { template <typename Matcher> void runDataflow(llvm::StringRef Code, Matcher Expectations) { tooling::FileContentMappings FilesContents; - FilesContents.push_back(std::make_pair<std::string, std::string>( - "noreturn_test_defs.h", R"( + FilesContents.push_back( + std::make_pair<std::string, std::string>("noreturn_test_defs.h", R"( void assertionHandler() __attribute__((analyzer_noreturn)); void assertionTrampoline() { @@ -730,8 +730,7 @@ class AnalyzerNoreturnTest : public Test { } }; -TEST_F(AnalyzerNoreturnTest, - Breathing) { +TEST_F(AnalyzerNoreturnTest, Breathing) { std::string Code = R"( #include "noreturn_test_defs.h" >From 918475d28f3c4b31970c57fdae66e331aea35d3e Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 7 Jul 2025 19:06:07 +0300 Subject: [PATCH 4/8] Fix some test failures --- clang/lib/Analysis/CFG.cpp | 30 ---------- .../Analysis/FlowSensitive/TransferTest.cpp | 5 +- .../TypeErasedDataflowAnalysisTest.cpp | 58 +++++-------------- 3 files changed, 18 insertions(+), 75 deletions(-) diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index bc47891216e7a..5dcde0052e00c 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2833,38 +2833,8 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) { // (see [expr.call]). if (!FD->isVariadic()) findConstructionContextsForArguments(C); - if (!NoReturn) NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context); - - // Some well-known 'noreturn' functions - if (!NoReturn) - NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString()) - .Case("BloombergLP::bsls::Assert::invokeHandler", true) - .Case("std::terminate", true) - .Case("std::abort", true) - .Case("exit", true) - .Case("abort", true) - .Case("panic", true) - .Case("error", true) - .Case("Assert", true) - .Case("ziperr", true) - .Case("assfail", true) - .Case("db_error", true) - .Case("__assert", true) - .Case("__assert2", true) - .Case("_wassert", true) - .Case("__assert_rtn", true) - .Case("__assert_fail", true) - .Case("dtrace_assfail", true) - .Case("yy_fatal_error", true) - .Case("_XCAssertionFailureHandler", true) - .Case("_DTAssertionFailureHandler", true) - .Case("_TSAssertionFailureHandler", true) - .Case("__builtin_trap", true) - .Case("__builtin_unreachable", true) - .Default(false); - if (FD->hasAttr<NoThrowAttr>()) AddEHEdge = false; if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) || diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 214aaee9f97f6..1731189d0cace 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -5950,14 +5950,15 @@ TEST(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) { Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { - ASSERT_THAT(Results.keys(), UnorderedElementsAre("loop_body")); + // target() considered as 'noreturn' by CFG + EXPECT_TRUE(Results.keys().empty()); const Environment &LoopBodyEnv = getEnvironmentAtAnnotation(Results, "loop_body"); const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); ASSERT_THAT(FooDecl, NotNull()); - auto &LoopBodyFooVal= getFormula(*FooDecl, LoopBodyEnv); + auto &LoopBodyFooVal = getFormula(*FooDecl, LoopBodyEnv); EXPECT_FALSE(LoopBodyEnv.proves(LoopBodyFooVal)); }); } diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 9150b3e155533..54b63e5606843 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -408,49 +408,6 @@ TEST_F(DiscardExprStateTest, CallWithParenExprTreatedCorrectly) { EXPECT_NE(CallExpectState.Env.getValue(FnToPtrDecay), nullptr); } -struct NonConvergingLattice { - int State; - - bool operator==(const NonConvergingLattice &Other) const { - return State == Other.State; - } - - LatticeJoinEffect join(const NonConvergingLattice &Other) { - if (Other.State == 0) - return LatticeJoinEffect::Unchanged; - State += Other.State; - return LatticeJoinEffect::Changed; - } -}; - -class NonConvergingAnalysis - : public DataflowAnalysis<NonConvergingAnalysis, NonConvergingLattice> { -public: - explicit NonConvergingAnalysis(ASTContext &Context) - : DataflowAnalysis<NonConvergingAnalysis, NonConvergingLattice>( - Context, - // Don't apply builtin transfer function. - DataflowAnalysisOptions{std::nullopt}) {} - - static NonConvergingLattice initialElement() { return {0}; } - - void transfer(const CFGElement &, NonConvergingLattice &E, Environment &) { - ++E.State; - } -}; - -TEST_F(DataflowAnalysisTest, NonConvergingAnalysis) { - std::string Code = R"( - void target() { - while(true) {} - } - )"; - auto Res = runAnalysis<NonConvergingAnalysis>( - Code, [](ASTContext &C) { return NonConvergingAnalysis(C); }); - EXPECT_EQ(llvm::toString(Res.takeError()), - "maximum number of blocks processed"); -} - // Regression test for joins of bool-typed lvalue expressions. The first loop // results in two passes through the code that follows. Each pass results in a // different `StorageLocation` for the pointee of `v`. Then, the second loop @@ -774,6 +731,21 @@ TEST_F(AnalyzerNoreturnTest, IndirectNoReturnCall) { UnorderedElementsAre("trap"))))))); } +TEST_F(AnalyzerNoreturnTest, InfiniteLoop) { + std::string Code = R"( + #include "noreturn_test_defs.h" + + void target() { + while(true){} + trap(); + // [[p]] + } + )"; + runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap"))))))); +} + // Models an analysis that uses flow conditions. class SpecialBoolAnalysis final : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> { >From a581a5c529f270053f8989499f33a25c7cdd168b Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 14 Jul 2025 16:25:50 +0300 Subject: [PATCH 5/8] Refactoring initial solution --- clang/include/clang/AST/Decl.h | 5 +++- clang/include/clang/Basic/Attr.td | 3 +- clang/lib/AST/Decl.cpp | 10 +++++-- clang/lib/Analysis/CFG.cpp | 48 +++++++++++++++++++++---------- clang/lib/Sema/SemaDeclAttr.cpp | 18 +++++++++++- 5 files changed, 64 insertions(+), 20 deletions(-) diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index cbecf918e8d42..62d8dad48768b 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -2651,7 +2651,10 @@ class FunctionDecl : public DeclaratorDecl, /// Determines whether this function is known to never return for CFG /// analysis. Checks for noreturn attributes on the function declaration /// or its type, including 'analyzer_noreturn' attribute. - bool isAnalyzerNoReturn() const; + /// + /// Returns 'std::nullopt' if function declaration has no '*noreturn' + /// attributes + std::optional<bool> getAnalyzerNoReturn() const; /// True if the function was a definition but its body was skipped. bool hasSkippedBody() const { return FunctionDeclBits.HasSkippedBody; } diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 224cb6a32af28..671f9ef6ef159 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -974,7 +974,8 @@ def AnalyzerNoReturn : InheritableAttr { // vendor namespace, or should it use a vendor namespace specific to the // analyzer? let Spellings = [GNU<"analyzer_noreturn">]; - // TODO: Add subject list. + let Args = [DefaultBoolArgument<"Value", /*default=*/1, /*fake=*/0>]; + let Subjects = SubjectList<[Function]>; let Documentation = [Undocumented]; } diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index d2c79250f5442..8bdfd4cf92fd5 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -3596,8 +3596,14 @@ bool FunctionDecl::isNoReturn() const { return false; } -bool FunctionDecl::isAnalyzerNoReturn() const { - return isNoReturn() || hasAttr<AnalyzerNoReturnAttr>(); +std::optional<bool> FunctionDecl::getAnalyzerNoReturn() const { + if (isNoReturn()) + return true; + + if (auto *Attr = getAttr<AnalyzerNoReturnAttr>()) + return Attr->getValue(); + + return std::nullopt; } bool FunctionDecl::isMemberLikeConstrainedFriend() const { diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 5dcde0052e00c..09b37cd5864a3 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -41,7 +41,6 @@ #include "llvm/ADT/APSInt.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SetVector.h" #include "llvm/ADT/SmallPtrSet.h" @@ -2833,8 +2832,13 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) { // (see [expr.call]). if (!FD->isVariadic()) findConstructionContextsForArguments(C); - if (!NoReturn) - NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context); + + if (!NoReturn) { + auto NoRetAttrOpt = FD->getAnalyzerNoReturn(); + NoReturn = + (NoRetAttrOpt && *NoRetAttrOpt) || C->isBuiltinAssumeFalse(*Context); + } + if (FD->hasAttr<NoThrowAttr>()) AddEHEdge = false; if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) || @@ -6311,31 +6315,45 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) { })) return true; - auto HasNoReturnCall = [](const CallExpr *CE) { + auto HasNoReturnCall = [&](const CallExpr *CE) { if (!CE) return false; - static thread_local llvm::SmallPtrSet<const FunctionDecl *, 32> InProgress; - auto *FD = CE->getDirectCallee(); - if (!FD || InProgress.count(FD)) + if (!FD) return false; - InProgress.insert(FD); - auto DoCleanup = llvm::make_scope_exit([&]() { InProgress.erase(FD); }); + // HACK: we are gonna cache analysis result as implicit `analyzer_noreturn` + // attribute + auto *MutFD = const_cast<FunctionDecl *>(FD); + auto NoRetAttrOpt = FD->getAnalyzerNoReturn(); + auto NoReturn = false; - auto NoReturnFromCFG = [FD]() { - if (!FD->getBody()) - return false; + if (!NoRetAttrOpt && FD->getBody()) { + // Mark function as `analyzer_noreturn(false)` to: + // * prevent infinite recursion in noreturn analysis + // * indicate that we've already analyzed(-ing) this function + // * serve as a safe default assumption (function may return) + MutFD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( + FD->getASTContext(), false, FD->getLocation())); auto CalleeCFG = CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {}); - return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking(); - }; + NoReturn = CalleeCFG && CalleeCFG->getEntry().isInevitablySinking(); + + // Override to `analyzer_noreturn(true)` + if (NoReturn) { + MutFD->dropAttr<AnalyzerNoReturnAttr>(); + MutFD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( + FD->getASTContext(), NoReturn, FD->getLocation())); + } + + } else if (NoRetAttrOpt) + NoReturn = *NoRetAttrOpt; - return FD->isAnalyzerNoReturn() || NoReturnFromCFG(); + return NoReturn; }; if (llvm::any_of(*Blk, [&](const CFGElement &Elm) { diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 60a9aee2d41e7..66e4eed31b6c8 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -2060,7 +2060,23 @@ static void handleAnalyzerNoReturnAttr(Sema &S, Decl *D, const ParsedAttr &AL) { } } - D->addAttr(::new (S.Context) AnalyzerNoReturnAttr(S.Context, AL)); + bool Value = true; + + if (AL.getNumArgs() > 0) { + auto *E = AL.getArgAsExpr(0); + + if (S.CheckBooleanCondition(AL.getLoc(), E, true).isInvalid()) + return; + + if (!E->EvaluateAsBooleanCondition(Value, S.Context, true)) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type) + << AL << 1 << AANT_ArgumentIntOrBool << E->getSourceRange(); + + return; + } + } + + D->addAttr(::new (S.Context) AnalyzerNoReturnAttr(S.Context, AL, Value)); } // PS3 PPU-specific. >From 737e9ca05cd66f46e586909d9cee76e6242fba19 Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 14 Jul 2025 17:02:23 +0300 Subject: [PATCH 6/8] Fix tests (hope so) --- .../Analysis/FlowSensitive/TransferTest.cpp | 15 ++++-- .../TypeErasedDataflowAnalysisTest.cpp | 52 +++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 1731189d0cace..d1916673debb7 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -5940,8 +5940,18 @@ TEST(TransferTest, ForStmtBranchExtendsFlowCondition) { TEST(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) { std::string Code = R"( void target(bool Foo) { - for (;;) { + unsigned i = 0; + + for (;;++i) { (void)0; + + // preventing CFG from considering this function + // as 'noreturn' + if (i == ~0) + break; + else + i = 0; + // [[loop_body]] } } @@ -5950,8 +5960,6 @@ TEST(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) { Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { - // target() considered as 'noreturn' by CFG - EXPECT_TRUE(Results.keys().empty()); const Environment &LoopBodyEnv = getEnvironmentAtAnnotation(Results, "loop_body"); @@ -5961,6 +5969,7 @@ TEST(TransferTest, ForStmtBranchWithoutConditionDoesNotExtendFlowCondition) { auto &LoopBodyFooVal = getFormula(*FooDecl, LoopBodyEnv); EXPECT_FALSE(LoopBodyEnv.proves(LoopBodyFooVal)); }); +}); } TEST(TransferTest, ContextSensitiveOptionDisabled) { diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 54b63e5606843..de3fd4135945a 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -408,6 +408,58 @@ TEST_F(DiscardExprStateTest, CallWithParenExprTreatedCorrectly) { EXPECT_NE(CallExpectState.Env.getValue(FnToPtrDecay), nullptr); } + +struct NonConvergingLattice { + int State; + + bool operator==(const NonConvergingLattice &Other) const { + return State == Other.State; + } + + LatticeJoinEffect join(const NonConvergingLattice &Other) { + if (Other.State == 0) + return LatticeJoinEffect::Unchanged; + State += Other.State; + return LatticeJoinEffect::Changed; + } +}; + +class NonConvergingAnalysis + : public DataflowAnalysis<NonConvergingAnalysis, NonConvergingLattice> { +public: + explicit NonConvergingAnalysis(ASTContext &Context) + : DataflowAnalysis<NonConvergingAnalysis, NonConvergingLattice>( + Context, + // Don't apply builtin transfer function. + DataflowAnalysisOptions{std::nullopt}) {} + + static NonConvergingLattice initialElement() { return {0}; } + + void transfer(const CFGElement &, NonConvergingLattice &E, Environment &) { + ++E.State; + } +}; + +TEST_F(DataflowAnalysisTest, NonConvergingAnalysis) { + std::string Code = R"( + void target() { + unsigned i =0; + for(;;++i) { + // preventing CFG from considering this function + // as 'noreturn' + if (i == ~0) + break; + else + i = 0; + } + } + )"; + auto Res = runAnalysis<NonConvergingAnalysis>( + Code, [](ASTContext &C) { return NonConvergingAnalysis(C); }); + EXPECT_EQ(llvm::toString(Res.takeError()), + "maximum number of blocks processed"); +} + // Regression test for joins of bool-typed lvalue expressions. The first loop // results in two passes through the code that follows. Each pass results in a // different `StorageLocation` for the pointee of `v`. Then, the second loop >From f2f356e0d4356ce23a4314664acfcccb9db06f5e Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Mon, 14 Jul 2025 17:05:53 +0300 Subject: [PATCH 7/8] Rollback some changes --- .../Checkers/NoReturnFunctionChecker.cpp | 70 ++++++++----------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp index 834bd81c2fa21..17c3cb4e9e04c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp @@ -50,45 +50,37 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent &CE, BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn(); } - if (!BuildSinks && CE.isGlobalCFunction()) { - if (const IdentifierInfo *II = CE.getCalleeIdentifier()) { - // HACK: Some functions are not marked noreturn, and don't return. - // Here are a few hardwired ones. If this takes too long, we can - // potentially cache these results. - // - // (!) In case of function list update, please also update - // CFGBuilder::VisitCallExpr (CFG.cpp) - BuildSinks = - llvm::StringSwitch<bool>(StringRef(II->getName())) - .Case("exit", true) - .Case("abort", true) - .Case("panic", true) - .Case("error", true) - .Case("Assert", true) - // FIXME: This is just a wrapper around throwing an exception. - // Eventually inter-procedural analysis should handle this - // easily. - .Case("ziperr", true) - .Case("assfail", true) - .Case("db_error", true) - .Case("__assert", true) - .Case("__assert2", true) - // For the purpose of static analysis, we do not care that - // this MSVC function will return if the user decides to - // continue. - .Case("_wassert", true) - .Case("__assert_rtn", true) - .Case("__assert_fail", true) - .Case("dtrace_assfail", true) - .Case("yy_fatal_error", true) - .Case("_XCAssertionFailureHandler", true) - .Case("_DTAssertionFailureHandler", true) - .Case("_TSAssertionFailureHandler", true) - .Case("__builtin_trap", true) - .Case("__builtin_unreachable", true) - .Default(false); - } - } + if (!BuildSinks && CE.isGlobalCFunction()) { + if (const IdentifierInfo *II = CE.getCalleeIdentifier()) { + // HACK: Some functions are not marked noreturn, and don't return. + // Here are a few hardwired ones. If this takes too long, we can + // potentially cache these results. + BuildSinks + = llvm::StringSwitch<bool>(StringRef(II->getName())) + .Case("exit", true) + .Case("panic", true) + .Case("error", true) + .Case("Assert", true) + // FIXME: This is just a wrapper around throwing an exception. + // Eventually inter-procedural analysis should handle this easily. + .Case("ziperr", true) + .Case("assfail", true) + .Case("db_error", true) + .Case("__assert", true) + .Case("__assert2", true) + // For the purpose of static analysis, we do not care that + // this MSVC function will return if the user decides to continue. + .Case("_wassert", true) + .Case("__assert_rtn", true) + .Case("__assert_fail", true) + .Case("dtrace_assfail", true) + .Case("yy_fatal_error", true) + .Case("_XCAssertionFailureHandler", true) + .Case("_DTAssertionFailureHandler", true) + .Case("_TSAssertionFailureHandler", true) + .Default(false); + } + } if (BuildSinks) C.generateSink(C.getState(), C.getPredecessor()); >From a7c20d4ac3c02ac12363a7fbee2d4b46f80923de Mon Sep 17 00:00:00 2001 From: Andrey Karlov <dein.nega...@gmail.com> Date: Tue, 15 Jul 2025 12:50:52 +0300 Subject: [PATCH 8/8] Use canonical declaration for analysis --- .../bugprone/unchecked-optional-access.cpp | 17 +++--- clang/lib/Analysis/CFG.cpp | 25 +++++---- .../TypeErasedDataflowAnalysisTest.cpp | 52 +++++++++++++++++++ 3 files changed, 73 insertions(+), 21 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp index f4f82199a0bb5..c99c6c3a81477 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp @@ -143,6 +143,8 @@ void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo void assertion_handler_imp() __attribute__((analyzer_noreturn)); +void assertion_handler(); + void assertion_handler() { do { assertion_handler_imp(); @@ -158,20 +160,15 @@ void function_calling_analyzer_noreturn(const bsl::optional<int>& opt) *opt; // no-warning } -void abort(); - -void do_fail() { - abort(); // acts like 'abort()' C-function -} - -void invoke_assertion_handler() { - do_fail(); +// Should be considered as 'noreturn' by CFG +void no_return_from_cfg() { + for(;;) {} } -void function_calling_well_known_noreturn(const bsl::optional<int>& opt) +void function_calling_no_return_from_cfg(const bsl::optional<int>& opt) { if (!opt) { - invoke_assertion_handler(); + no_return_from_cfg(); } *opt; // no-warning diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 09b37cd5864a3..574780e8e5ba1 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -6324,30 +6324,33 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) { if (!FD) return false; - // HACK: we are gonna cache analysis result as implicit `analyzer_noreturn` - // attribute - auto *MutFD = const_cast<FunctionDecl *>(FD); - auto NoRetAttrOpt = FD->getAnalyzerNoReturn(); + auto *CanCD = FD->getCanonicalDecl(); + auto *DefFD = CanCD->getDefinition(); + auto NoRetAttrOpt = CanCD->getAnalyzerNoReturn(); auto NoReturn = false; - if (!NoRetAttrOpt && FD->getBody()) { + if (!NoRetAttrOpt && DefFD && DefFD->getBody()) { + // HACK: we are gonna cache analysis result as implicit + // `analyzer_noreturn` attribute + auto *MutCD = const_cast<FunctionDecl *>(CanCD); + // Mark function as `analyzer_noreturn(false)` to: // * prevent infinite recursion in noreturn analysis // * indicate that we've already analyzed(-ing) this function // * serve as a safe default assumption (function may return) - MutFD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( - FD->getASTContext(), false, FD->getLocation())); + MutCD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( + CanCD->getASTContext(), false, CanCD->getLocation())); auto CalleeCFG = - CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {}); + CFG::buildCFG(DefFD, DefFD->getBody(), &DefFD->getASTContext(), {}); NoReturn = CalleeCFG && CalleeCFG->getEntry().isInevitablySinking(); // Override to `analyzer_noreturn(true)` if (NoReturn) { - MutFD->dropAttr<AnalyzerNoReturnAttr>(); - MutFD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( - FD->getASTContext(), NoReturn, FD->getLocation())); + MutCD->dropAttr<AnalyzerNoReturnAttr>(); + MutCD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( + CanCD->getASTContext(), NoReturn, CanCD->getLocation())); } } else if (NoRetAttrOpt) diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index de3fd4135945a..0f51e7a64c9b0 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -718,6 +718,27 @@ class AnalyzerNoreturnTest : public Test { void trap() {} )")); + FilesContents.push_back(std::make_pair<std::string, std::string>( + "noreturn_test_defs_canonical.h", R"( + extern void assertionHandler(); + + void assertionSecondTrampoline() { + assertionHandler(); + } + )")); + FilesContents.push_back(std::make_pair<std::string, std::string>( + "noreturn_test_defs_noretcfg.h", R"( + // will be marged as noreturn by CFG + void assertionHandler() { + for (;;) {} + } + + void assertionTrampoline() { + assertionHandler(); + } + + void trap() {} + )")); ASSERT_THAT_ERROR( test::checkDataflow<FunctionCallAnalysis>( @@ -783,6 +804,37 @@ TEST_F(AnalyzerNoreturnTest, IndirectNoReturnCall) { UnorderedElementsAre("trap"))))))); } +TEST_F(AnalyzerNoreturnTest, CanonicalDeclCallCheck) { + std::string Code = R"( + #include "noreturn_test_defs.h" + #include "noreturn_test_defs_canonical.h" + + void target() { + assertionSecondTrampoline(); + trap(); + // [[p]] + } + )"; + runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap"))))))); +} + +TEST_F(AnalyzerNoreturnTest, NoReturnFromCFGCheck) { + std::string Code = R"( + #include "noreturn_test_defs_noretcfg.h" + + void target() { + assertionTrampoline(); + trap(); + // [[p]] + } + )"; + runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry( + "p", HoldsFunctionCallLattice(HasCalledFunctions( + UnorderedElementsAre("trap"))))))); +} + TEST_F(AnalyzerNoreturnTest, InfiniteLoop) { std::string Code = R"( #include "noreturn_test_defs.h" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits