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 &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& 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& opt) +{ + if (!opt) { + invoke_assertion_handler(); + } + + *opt; // no-warning +} + template void function_template_without_user(const absl::optional &opt) { opt.value(); // no-warning diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index f70a039bf3517..6ada8785fd0ba 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -2630,6 +2630,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 e0362245d6ecd..19336a8e942cd 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -3595,6 +3595,10 @@ bool FunctionDecl::isNoReturn() const { return false; } +bool FunctionDecl::isAnalyzerNoReturn() const { + return isNoReturn() || hasAttr(); +} + 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 d960d5130332b..5dcde0052e00c 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" @@ -2832,9 +2833,8 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) { // (see [expr.call]). if (!FD->isVariadic()) findConstructionContextsForArguments(C); - - if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context)) - NoReturn = true; + if (!NoReturn) + NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context); if (FD->hasAttr()) AddEHEdge = false; if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) || @@ -6288,6 +6288,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; @@ -6298,10 +6304,43 @@ 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 StmtElm = Elm.getAs()) + return isa(StmtElm->getStmt()); + return false; + })) + return true; + + auto HasNoReturnCall = [](const CallExpr *CE) { + if (!CE) + return false; + + static thread_local llvm::SmallPtrSet InProgress; + + auto *FD = CE->getDirectCallee(); + + 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; + + 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 StmtElm = Elm.getAs()) - if (isa(StmtElm->getStmt())) - return true; + return HasNoReturnCall(dyn_cast(StmtElm->getStmt())); return false; })) return true; 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); 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(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(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/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> &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 9fb7bebdbe41e..54b63e5606843 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; @@ -407,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 { -public: - explicit NonConvergingAnalysis(ASTContext &Context) - : DataflowAnalysis( - 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( - 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 @@ -693,6 +651,101 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) { // FIXME: Called functions at point `p` should contain only "foo". } +class AnalyzerNoreturnTest : public Test { +protected: + template + void runDataflow(llvm::StringRef Code, Matcher Expectations) { + tooling::FileContentMappings FilesContents; + FilesContents.push_back( + std::make_pair("noreturn_test_defs.h", R"( + void assertionHandler() __attribute__((analyzer_noreturn)); + + void assertionTrampoline() { + assertionHandler(); + } + + void trap() {} + )")); + + ASSERT_THAT_ERROR( + test::checkDataflow( + AnalysisInputs( + 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> &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"))))))); +} + +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 {