Skip to content

[clang]: Propagate *noreturn attributes in CFG #146355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[clang]: Propagate *noreturn attributes in CFG #146355

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc3b77d
Initial implementation
negativ Jun 30, 2025
0b2e72d
Unittests
negativ Jul 4, 2025
3562ea0
Merge branch 'llvm:main' into analyzer_noreturn
negativ Jul 7, 2025
06cbe1d
Crash fix & PR fixes
negativ Jul 7, 2025
400dbd1
Merge branch 'main' into analyzer_noreturn
negativ Jul 7, 2025
918475d
Fix some test failures
negativ Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is no-warning handled by the test harness? I'm familiar/have seen // CHECK-NOT: warning comments. Unless any extraneous warnings already fail the test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a comment that we do not expect warning here. check_clang_tidy.py ignores it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, i consider moving this kind of check to clang\unittests\Analysis\FlowSensitive\UncheckedOptionalAccessModelTest.cpp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I was just checking/asking whether the test will fail if a warning is emitted or whether you need a // CHECK-NOT: warning. But it seems for most tests any warning will produce a failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy tests require an exact match of diagnostics, so too many or too few diagnostics will fail the test

}

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
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
79 changes: 74 additions & 5 deletions clang/lib/Analysis/CFG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consolidate this list with the one in NoReturnFunctionChecker.cpp? Or at least reference the other one to remind people to update both.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeWeller i've added comment in NoreturnFunctionChecker.cpp

if (!NoReturn)
NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

.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) ||
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of static locals is a red-flag.
Consider running gtests, where multiple clang instances will be created in sequence, slowly filling up the static cache with decl pointer that will get dangling after each test. Once in a while after a long running unittest you would get a cache hit for a dangling pointer and the disaster happens.

This is not a theoretical case. It cost me 2 days.


auto *FD = CE->getDirectCallee();

if (!FD || InProgress.count(FD))
return false;

InProgress.insert(FD);
Comment on lines +6322 to +6325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would do a double lookup here with InProgress. The first would be in count(FD and the second will be on insert(FD). You could use try_emplace and check if insertion happened.

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(), {});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that we end up building the CFG for the same functions over and over again? This sounds like potentially wasteful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
   }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to create attrs, be sure to create them as implicit, as they were not spelled in source.


return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking();
};

return FD->isAnalyzerNoReturn() || NoReturnFromCFG();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have redecls, does it matter which has the attribute, or do we always bind the attribute to the canonical decl?

};

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;
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
70 changes: 39 additions & 31 deletions clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Comment on lines +53 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the analyzer depend on the injected noreturn attributes here instead of matching again the raw identifier strings in the analyzer (again, after the CFG contruction)?


if (BuildSinks)
C.generateSink(C.getState(), C.getPredecessor());
Expand Down
81 changes: 81 additions & 0 deletions clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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> {
Expand Down
Loading