[Analysis] Avoid some warnings about exit from noreturn function #144408
+382
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Serge Pavlov (spavloff) ChangesCompiler sometimes issues warnings on exit from 'noreturn' functions, in the code like: where exit cannot take place because the function pointer is actually a pointer to noreturn function. This change introduces small data analysis that can remove some of the warnings in the cases when compiler can prove that the set of reaching definitions consists of noreturn functions only. Full diff: https://github.com/llvm/llvm-project/pull/144408.diff 2 Files Affected:
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 2a107a36e24b4..2853e1e7ebf3e 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -36,6 +36,7 @@
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/CFGStmtMap.h"
+#include "clang/Analysis/FlowSensitive/DataflowWorklist.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/SourceLocation.h"
@@ -399,6 +400,108 @@ static bool isNoexcept(const FunctionDecl *FD) {
return false;
}
+/// Checks if the given variable, which is assumed to be a function pointer, is
+/// initialized with a function having 'noreturn' attribute.
+static bool isInitializedWithNoReturn(const VarDecl *VD) {
+ if (const Expr *Init = VD->getInit()) {
+ if (auto *ListInit = dyn_cast<InitListExpr>(Init);
+ ListInit && ListInit->getNumInits() > 0)
+ Init = ListInit->getInit(0);
+ if (auto *DeclRef = dyn_cast<DeclRefExpr>(Init->IgnoreParenCasts()))
+ if (auto *FD = dyn_cast<FunctionDecl>(DeclRef->getDecl()))
+ return FD->isNoReturn();
+ }
+ return false;
+}
+
+/// Checks if the given expression is a reference to a function with
+/// 'noreturn' attribute.
+static bool isReferenceToNoReturn(const Expr *E) {
+ if (auto *DRef = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts()))
+ if (auto *FD = dyn_cast<FunctionDecl>(DRef->getDecl()))
+ return FD->isNoReturn();
+ return false;
+}
+
+namespace {
+
+/// Looks for statements, that can define value of the given variable.
+struct TransferFunctions : public StmtVisitor<TransferFunctions> {
+ const VarDecl *Var;
+ std::optional<bool> AllValuesAreNoReturn;
+
+ TransferFunctions(const VarDecl *VD) : Var(VD) {}
+
+ void VisitDeclStmt(DeclStmt *DS) {
+ for (auto *DI : DS->decls())
+ if (auto *VD = dyn_cast<VarDecl>(DI))
+ if (VarDecl *Def = VD->getDefinition())
+ if (Def == Var)
+ AllValuesAreNoReturn = isInitializedWithNoReturn(Def);
+ }
+
+ void VisitUnaryOperator(UnaryOperator *UO) {
+ if (UO->getOpcode() == UO_AddrOf) {
+ if (auto *DRef =
+ dyn_cast<DeclRefExpr>(UO->getSubExpr()->IgnoreParenCasts()))
+ if (DRef->getDecl() == Var)
+ AllValuesAreNoReturn = false;
+ }
+ }
+
+ void VisitBinaryOperator(BinaryOperator *BO) {
+ if (BO->getOpcode() == BO_Assign)
+ if (auto *DRef = dyn_cast<DeclRefExpr>(BO->getLHS()->IgnoreParenCasts()))
+ if (DRef->getDecl() == Var)
+ AllValuesAreNoReturn = isReferenceToNoReturn(BO->getRHS());
+ }
+
+ void VisitCallExpr(CallExpr *CE) {
+ for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end(); I != E;
+ ++I) {
+ const Expr *Arg = *I;
+ if (Arg->isGLValue() && !Arg->getType().isConstQualified())
+ if (auto *DRef = dyn_cast<DeclRefExpr>(Arg->IgnoreParenCasts()))
+ if (auto VD = dyn_cast<VarDecl>(DRef->getDecl()))
+ if (VD->getDefinition() == Var)
+ AllValuesAreNoReturn = false;
+ }
+ }
+};
+} // namespace
+
+// Checks if all possible values of the given variable are functions with
+// 'noreturn' attribute.
+static bool areAllValuesNoReturn(const VarDecl *VD, const CFGBlock &VarBlk,
+ AnalysisDeclContext &AC) {
+ // The set of possible values of a constant variable is determined by
+ // its initializer.
+ if (VD->getType().isConstant(AC.getASTContext())) {
+ if (const VarDecl *Def = VD->getDefinition())
+ return isInitializedWithNoReturn(Def);
+ return false;
+ }
+
+ // Scan function statements for definitions of the given variable.
+ TransferFunctions TF(VD);
+ BackwardDataflowWorklist Worklist(*AC.getCFG(), AC);
+ Worklist.enqueueBlock(&VarBlk);
+ while (const CFGBlock *B = Worklist.dequeue()) {
+ for (CFGBlock::const_reverse_iterator ri = B->rbegin(), re = B->rend();
+ ri != re; ++ri) {
+ if (std::optional<CFGStmt> cs = ri->getAs<CFGStmt>()) {
+ const Stmt *S = cs->getStmt();
+ TF.Visit(const_cast<Stmt *>(S));
+ if (TF.AllValuesAreNoReturn)
+ return *TF.AllValuesAreNoReturn;
+ }
+ }
+ Worklist.enqueuePredecessors(B);
+ }
+
+ return false;
+}
+
//===----------------------------------------------------------------------===//
// Check for missing return value.
//===----------------------------------------------------------------------===//
@@ -525,6 +628,17 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
HasAbnormalEdge = true;
continue;
}
+ if (auto *Call = dyn_cast<CallExpr>(S)) {
+ const Expr *Callee = Call->getCallee();
+ if (Callee->getType()->isPointerType())
+ if (auto *DeclRef =
+ dyn_cast<DeclRefExpr>(Callee->IgnoreParenImpCasts()))
+ if (auto *VD = dyn_cast<VarDecl>(DeclRef->getDecl()))
+ if (areAllValuesNoReturn(VD, B, AC)) {
+ HasAbnormalEdge = true;
+ continue;
+ }
+ }
HasPlainEdge = true;
}
diff --git a/clang/test/SemaCXX/noreturn-vars.cpp b/clang/test/SemaCXX/noreturn-vars.cpp
new file mode 100644
index 0000000000000..31d16db46e4a2
--- /dev/null
+++ b/clang/test/SemaCXX/noreturn-vars.cpp
@@ -0,0 +1,107 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+[[noreturn]] extern void noret();
+[[noreturn]] extern void noret2();
+extern void ordinary();
+
+typedef void (*func_type)(void);
+
+// Constant initialization.
+
+void (* const const_fptr)() = noret;
+[[noreturn]] void test_global_const() {
+ const_fptr();
+}
+
+const func_type const_fptr_cast = (func_type)noret2;
+[[noreturn]] void test_global_cast() {
+ const_fptr_cast();
+}
+
+void (* const const_fptr_list)() = {noret};
+[[noreturn]] void test_global_list() {
+ const_fptr_list();
+}
+
+const func_type const_fptr_fcast = func_type(noret2);
+[[noreturn]] void test_global_fcast() {
+ const_fptr_fcast();
+}
+
+[[noreturn]] void test_local_const() {
+ void (* const fptr)() = noret;
+ fptr();
+}
+
+// Global variable assignment.
+void (*global_fptr)() = noret;
+
+[[noreturn]] void test_global_noassign() {
+ global_fptr();
+} // expected-warning {{function declared 'noreturn' should not return}}
+
+[[noreturn]] void test_global_assign() {
+ global_fptr = noret;
+ global_fptr();
+}
+
+[[noreturn]] void test_global_override() {
+ global_fptr = ordinary;
+ global_fptr = noret;
+ global_fptr();
+}
+
+[[noreturn]] void test_global_switch_01(int x) {
+ switch(x) {
+ case 1:
+ global_fptr = noret;
+ break;
+ default:
+ global_fptr = noret2;
+ break;
+ }
+ global_fptr();
+}
+
+[[noreturn]] void test_global_switch_02(int x) {
+ switch(x) {
+ case 1:
+ global_fptr = ordinary;
+ break;
+ default:
+ global_fptr = noret;
+ break;
+ }
+ global_fptr();
+}
+
+// Local variable assignment.
+
+[[noreturn]] void test_local_init() {
+ func_type func_ptr = noret;
+ func_ptr();
+}
+
+[[noreturn]] void test_local_assign() {
+ void (*func_ptr)(void);
+ func_ptr = noret;
+ func_ptr();
+}
+
+// Escaped value.
+
+extern void abc_01(func_type &);
+extern void abc_02(func_type *);
+
+[[noreturn]] void test_escape_ref() {
+ func_type func_ptr = noret;
+ abc_01(func_ptr);
+ func_ptr();
+} // expected-warning {{function declared 'noreturn' should not return}}
+
+[[noreturn]] void test_escape_addr() {
+ func_type func_ptr = noret;
+ abc_02(&func_ptr);
+ func_ptr();
+} // expected-warning {{function declared 'noreturn' should not return}}
+
|
clang/test/SemaCXX/noreturn-vars.cpp
Outdated
| global_fptr = noret; | ||
| break; | ||
| } | ||
| global_fptr(); |
There was a problem hiding this comment.
I think it's a bug that we don't emit a diagnostic here. Even barring thread shenanigans, this can very easily lead to returning if x is 1.
There was a problem hiding this comment.
This behavior is fixed.
Comment on lines
+44
to
+45
| global_fptr = noret; | ||
| global_fptr(); |
There was a problem hiding this comment.
I think this is possibly a reasonable behavior, but just because there's a local assignment does not mean that the function doesn't return; another thread can put a different value there between the assignment and the call.
The conundrum is: [[noreturn]] is supposed to diagnose if the call could return, so if we can't prove that it doesn't return, we would usually diagnose.
There was a problem hiding this comment.
You are right, multithreaded environment was not supported in the previous implementation.
erichkeane
reviewed
Jun 24, 2025
| @@ -399,6 +401,145 @@ static bool isNoexcept(const FunctionDecl *FD) { | |||
| return false; | |||
| } | |||
|
|
|||
| /// Checks if the given variable, which is assumed to be a function pointer, is | |||
| /// initialized with a function having 'noreturn' attribute. | |||
| static bool isInitializedWithNoReturn(const VarDecl *VD) { | |||
There was a problem hiding this comment.
Hmm... We are checking the initializer here, but we don't check to make sure it wasn't assigned since then, right?
IMO it seems this is a failure of [[noreturn]] not being part of the type :)
There was a problem hiding this comment.
We are checking the initializer here, but we don't check to make sure it wasn't assigned since then, right?
No, the statements in a basic block are scanned in backward direction. So if the basic block contains an assignment, it will be tested first.
IMO it seems this is a failure of [[noreturn]] not being part of the type :)
You are not alone. For instance, GCC developers have the same opinion: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80495#c1.
| if (auto *ListInit = dyn_cast<InitListExpr>(Init); | ||
| ListInit && ListInit->getNumInits() > 0) | ||
| Init = ListInit->getInit(0); | ||
| if (auto *DeclRef = dyn_cast<DeclRefExpr>(Init->IgnoreParenCasts())) |
There was a problem hiding this comment.
Can we change this to:
return isReferenceToNoReturn(Init)?
| @@ -399,6 +401,145 @@ static bool isNoexcept(const FunctionDecl *FD) { | |||
| return false; | |||
| } | |||
|
|
|||
| /// Checks if the given variable, which is assumed to be a function pointer, is | |||
| /// initialized with a function having 'noreturn' attribute. | |||
| static bool isInitializedWithNoReturn(const VarDecl *VD) { | |||
There was a problem hiding this comment.
Rather than being 'static', I tend to prefer just putting these into the following unnamed namespace.
There was a problem hiding this comment.
LLVM coding style recommends using 'static': https://llvm.org/docs/CodingStandards.html#restrict-visibility.
There was a problem hiding this comment.
Huh, interesting. We've not been following that at all in clang, instead preferring to keep anonymous namespaces in most places. We'll have to discuss that.
|
|
||
| // Checks if all possible values of the given variable are functions with | ||
| // 'noreturn' attribute. | ||
| static bool areAllValuesNoReturn(const VarDecl *VD, const CFGBlock &VarBlk, |
|
Are there anything else that should be made? |
Compiler sometimes issues warnings on exit from 'noreturn' functions, in
the code like:
[[noreturn]] extern void nonreturnable();
void (*func_ptr)();
[[noreturn]] void foo() {
func_ptr = nonreturnable;
(*func_ptr)();
}
where exit cannot take place because the function pointer is actually a
pointer to noreturn function.
This change introduces small data analysis that can remove some of the
warnings in the cases when compiler can prove that the set of reaching
definitions consists of noreturn functions only.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/23366 Here is the relevant piece of the build log for the reference |
This was referenced Jul 23, 2025
|
It looks like this PR causes a regression: #150336 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Compiler sometimes issues warnings on exit from 'noreturn' functions, in the code like:
where exit cannot take place because the function pointer is actually a pointer to noreturn function.
This change introduces small data analysis that can remove some of the warnings in the cases when compiler can prove that the set of reaching definitions consists of noreturn functions only.