[clang]: Propagate *noreturn attributes in CFG
#146355
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
negativ
changed the title
Clang: analyzer_noreturn propagation in CFGClang: *noreturn propagation in CFG
There was a problem hiding this comment.
Quick pass / haven't looked in-depth yet.
- I would mention in the description/summary that plain
noreturnalready has this behaviour (at least in one of the CFG.cpp vs. NoReturnFunctionChecker.cpp implementations) so this is adding the same behaviour foranalyzer_noreturn.
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
Outdated
Show resolved
Hide resolved
clang/lib/Analysis/CFG.cpp
Outdated
| if (!NoReturn) | ||
| NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context); | ||
|
|
||
| // Some well-known 'noreturn' functions |
There was a problem hiding this comment.
Can we consolidate this list with the one in NoReturnFunctionChecker.cpp? Or at least reference the other one to remind people to update both.
There was a problem hiding this comment.
@MikeWeller i've added comment in NoreturnFunctionChecker.cpp
negativ
changed the title
Clang: *noreturn propagation in CFG*noreturn propagation in CFG
negativ
changed the title
*noreturn propagation in CFG*noreturn propagation in CFG
negativ
changed the title
*noreturn propagation in CFG*noreturn attributes in CFG
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang-analysis Author: Andrey Karlov (negativ) ChangesSummaryThis PR extends ProblemCurrently, simple forwarder functions to no-return calls are not recognized as no-return themselves: void terminate() __attribute__((analyzer_noreturn)) { }
void fatal_error() {
terminate(); // This "never" returns, but fatal_error() is not marked as no-return
}
void handle_error(const std::optional<int> opt) {
if (!opt) {
fatal_error(); // Static analyzer doesn't know this never returns
}
*opt = 1; // False positive: analyzer thinks this is reachable
}SolutionEnhanced
Implementation Details
Full diff: https://github.com/llvm/llvm-project/pull/146355.diff 7 Files Affected:
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 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<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 d960d5130332b..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"
@@ -2833,8 +2834,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) ||
@@ -6288,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;
@@ -6298,10 +6334,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<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+ return isa<CXXThrowExpr>(StmtElm->getStmt());
+ return false;
+ }))
+ return true;
+
+ 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))
+ 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<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
- if (isa<CXXThrowExpr>(StmtElm->getStmt()))
- return true;
+ return HasNoReturnCall(dyn_cast<CallExpr>(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<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 9fb7bebdbe41e..9150b3e155533 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,86 @@ 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> {
|
|
@llvm/pr-subscribers-clang-tidy Author: Andrey Karlov (negativ) ChangesSummaryThis PR extends ProblemCurrently, simple forwarder functions to no-return calls are not recognized as no-return themselves: void terminate() __attribute__((analyzer_noreturn)) { }
void fatal_error() {
terminate(); // This "never" returns, but fatal_error() is not marked as no-return
}
void handle_error(const std::optional<int> opt) {
if (!opt) {
fatal_error(); // Static analyzer doesn't know this never returns
}
*opt = 1; // False positive: analyzer thinks this is reachable
}SolutionEnhanced
Implementation Details
Full diff: https://github.com/llvm/llvm-project/pull/146355.diff 7 Files Affected:
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 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<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 d960d5130332b..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"
@@ -2833,8 +2834,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) ||
@@ -6288,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;
@@ -6298,10 +6334,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<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+ return isa<CXXThrowExpr>(StmtElm->getStmt());
+ return false;
+ }))
+ return true;
+
+ 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))
+ 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<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
- if (isa<CXXThrowExpr>(StmtElm->getStmt()))
- return true;
+ return HasNoReturnCall(dyn_cast<CallExpr>(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<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 9fb7bebdbe41e..9150b3e155533 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,86 @@ 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> {
|
There was a problem hiding this comment.
I think this is a good change in general, but I wonder what is the performance impact.
Note that clang CFGs have many clients and some of them might not need this information. If there is a measurable performance cost, we might want to put this behind a flag, especially (if I remember correctly) as there are some on by default warnings that build the CFG.
clang/lib/Analysis/CFG.cpp
Outdated
|
|
||
| // Some well-known 'noreturn' functions | ||
| if (!NoReturn) | ||
| NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString()) |
There was a problem hiding this comment.
@AaronBallman I wonder how you feel about having a list of well known (non standard library) functions hardcoded in the compiler?
I wonder if we want to have a more principled approach here long term, e.g., having one place in the compiler that injects annotations (like noreturn), and the rest of the code paths only handling that one annotation.
There was a problem hiding this comment.
@AaronBallman I wonder how you feel about having a list of well known (non standard library) functions hardcoded in the compiler?
Ultimately, I determined that maintaining a hardcoded list of functions in the compiler was quite risky and could potentially cause unexpected behavior, so I removed it from the CFG implementation.
There was a problem hiding this comment.
Thanks! I don't think it's risky per se, but it would be frustrating to not be something the user can configure themselves. If we find we need something like this, that suggest a clang-tidy check may be a more appropriate way to surface the functionality (they've got per-check configuration options for these sort of situations).
clang/lib/Analysis/CFG.cpp
Outdated
| return false; | ||
|
|
||
| auto CalleeCFG = | ||
| CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {}); |
There was a problem hiding this comment.
Is it possible that we end up building the CFG for the same functions over and over again? This sounds like potentially wasteful.
There was a problem hiding this comment.
As I understand it (please correct me if I'm wrong), FunctionDecl* instances within the constructed AST will be reused when building the CFG - if so, we could do something like:
if( FD->isAnalyzerNoReturn() || NoReturnFromCFG() ) {
const_cast<FunctionDecl *>(FD)->addAttr(AnalyzerNoReturnAttr::Create(
FD->getASTContext(), FD->getLocation()));
return true;
}
There was a problem hiding this comment.
If we decide to create attrs, be sure to create them as implicit, as they were not spelled in source.
There was a problem hiding this comment.
If we decide to create attrs, be sure to create them as implicit, as they were not spelled in source.
Fixed. Thanks for the tip!
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/Analysis/CFG.cpp
Outdated
| return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking(); | ||
| }; | ||
|
|
||
| return FD->isAnalyzerNoReturn() || NoReturnFromCFG(); |
There was a problem hiding this comment.
What if we have redecls, does it matter which has the attribute, or do we always bind the attribute to the canonical decl?
There was a problem hiding this comment.
What if we have redecls, does it matter which has the attribute, or do we always bind the attribute to the canonical decl?
Good catch! I'll fix my code to use the canonical decl.
There was a problem hiding this comment.
I don't see explicit handling of this, could you also write unittests for this? The tests you added to clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp would be a good place IMO.
There was a problem hiding this comment.
@5chmidti i've updated both TypeErasedDataflowAnalysisTest.cpp and unhecked-optional-access.cpp files.
There was a problem hiding this comment.
What if we have redecls, does it matter which has the attribute, or do we always bind the attribute to the canonical decl?
Switched to using canonical decl for analyzer_noreturn lookup & caching. BTW: since AnalyzerNoReturnAttr inherits from InheritableAttr, we can check any function decl.
There was a problem hiding this comment.
Hmm, what does an InheritableAttr do? I couldn't find documentation for it. Do you know?
There was a problem hiding this comment.
It has to do with whether an attribute is automatically inherited on redeclarations. e.g., https://godbolt.org/z/1PEvG8q7M
There was a problem hiding this comment.
When I swapped the do_inherit declarations, then the first declaration didn't inherit the attribute if that was put on the second declaration. If I'm not mistaken that this would mean that any references to the do_inherit function (such as calls to it) would refer to the decl without the attribute. Consequently, we can't rely on the attribute inheritance.
| MutCD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( | ||
| CanCD->getASTContext(), false, CanCD->getLocation())); | ||
|
|
||
| auto CalleeCFG = |
There was a problem hiding this comment.
While performance-wise this is a big step ahead, we might still end up building the CFG twice for large functions. Once when we first do this inter-procedural no-return analysis and once when we run the actual analysis on them. Did you do any benchmarking if this has any affect on the compilation with some of the frequently used configurations? (default warnings, -Wall, -Wall -Wextra)
In case there is a measurable regression, we might want to move this behind a flag.
There was a problem hiding this comment.
I've discussed this with @Xazax-hun and I agree with him that we should have a look at the performance of some warnings. I think they live under the AnalysisBasedWarnings.cpp file.
There could be many, but I can recall one which is the noreturn analysis, like CheckFallThrough.
Another problem with the use of this analyzer_noreturn(bool) attribute is that it will basically touch/pollute every FunctionDecl in the AST - if I understand it correctly.
There was a problem hiding this comment.
@steakhal @Xazax-hun I'm thinking about making this feature configurable through a flag. It will clearly have significant performance implications, but it's really only required for specific use cases (like clang-tidy checkers). It might be safer to explicitly opt-in only where necessary and where performance drops are more acceptable (e.g., in the unchecked-optional-access checker)
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
Outdated
Show resolved
Hide resolved
clang/include/clang/AST/Decl.h
Outdated
| /// 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. | ||
| /// | ||
| /// Returns 'std::nullopt' if function declaration has no '*noreturn' | ||
| /// attributes | ||
| std::optional<bool> getAnalyzerNoReturn() const; |
There was a problem hiding this comment.
The name of this function would suggest that it only checks the presence of analyzer_noreturn.
How about if we would rename this to something like isNoreturnForAnalyses?
BTW, how about other callables that are not FunctionDecls?
Such as ObjCMethodDecl or BlockDecl?
There was a problem hiding this comment.
@steakhal i've renamed this function to getAnalyzerSinkKind().
clang/include/clang/Basic/Attr.td
Outdated
| @@ -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]>; | |||
There was a problem hiding this comment.
I think we could attach this attribute to ObjCMethodDecl and BlockDecl too.
There was a problem hiding this comment.
I think we could attach this attribute to
ObjCMethodDeclandBlockDecltoo.
I'd prefer not to overcomplicate things right now and just focus on the core approach and C++ implementation - mainly because I'm completely clueless about testing ObjC code =)
Co-authored-by: Balazs Benics <[email protected]>
…optional-access.cpp Co-authored-by: Balazs Benics <[email protected]>
…optional-access.cpp Co-authored-by: Balazs Benics <[email protected]>
…optional-access.cpp Co-authored-by: Balazs Benics <[email protected]>
| MutCD->addAttr(AnalyzerNoReturnAttr::CreateImplicit( | ||
| CanCD->getASTContext(), false, CanCD->getLocation())); | ||
|
|
||
| auto CalleeCFG = |
There was a problem hiding this comment.
I've discussed this with @Xazax-hun and I agree with him that we should have a look at the performance of some warnings. I think they live under the AnalysisBasedWarnings.cpp file.
There could be many, but I can recall one which is the noreturn analysis, like CheckFallThrough.
Another problem with the use of this analyzer_noreturn(bool) attribute is that it will basically touch/pollute every FunctionDecl in the AST - if I understand it correctly.
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Show resolved
Hide resolved
|
The CFG construction doesn't seem to be the right place to propagate noreturn attributes. It is already fairly complicated and the annotations seem like an orthogonal concern. Adding deduced attributes should be a separate step in the ASTConsumers pipeline. This worked for us downstream for a similar use-cases. This also side-steps patching the CFG construction, because by the time we get there, we are already done with the annotation phase thus, it would see the implict (deduced) We build the CallGraph and iterate it in Post-order to make sure leaf calls are inferred first, thus the callers would get to see the deduced If I had time, I could probably try to upstream this part. The beauty of this approach is that its a separate component, and we would not need to piggyback on a default bool parameter of the |
Could you please point to some good example of using ASTConsumers pipeline?
Wouldn't that mean we'd have to reimplement CFG's useful features like unreachable code detection?
I'I'd appreciate it if you could point me to parts of the codebase I could look at to learn and try implementing the feature myself =) |
|
I’m thinking of splitting this into two PRs: one for basic analyzer_noreturn attribute processing, and another for implementing the propagation mechanism. |
Summary
This PR extends
Clang'sCFGanalysis to automatically propagateanalyzer_noreturnattributes through function call chains. When a function exclusively calls no-return functions, it is automatically treated as no-return as well, improving static analysis precision.Problem
Currently, simple forwarder functions to no-return calls are not recognized as no-return themselves:
Solution
analyzer_noreturnattribute to take one extraboolparam:trueif the function should be considered asno-returnby analyzer andfalseotherwiseCFG::BuildOptionswith the new optionExtendedNoReturnAnalysiswhich controls enablement of the newinter-procedural analysisFunctionDeclclass by addinggetAnalyzerSinkKind()function which adaptsanalyzer_noreturnattribute statecallexprinCFGBuildercheckExtendedNoReturnAnalysisflag and if it istruebuild newCFGfor the callee function and performno-return analysisby recursively calling functionisInevitablySinkingno-return analysisresults directly in AST (by callingFunctionDecl::setAnalyzerSinkKind()function)CFG::buildOptionsargument todataflow::diagnoseFunctionfunction to be able to pre-setupCFGBuilderKnown limitations
CFG::BuildOptions(seeclang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp)isInevitablySinking()can add and removeFunctionDeclattributes inASTduring analysis(!)