clang-tidy does not understand `analyzer_noreturn` attribute

[MikeWeller]

https://clang-analyzer.llvm.org/annotations.html#attr_analyzer_noreturn describes the analyzer_noreturn attribute:

The Clang-specific 'analyzer_noreturn' attribute is almost identical to 'noreturn' except that it is ignored by the compiler for the purposes of code generation.

This attribute is useful for annotating assertion handlers that actually can return, but for the purpose of using the analyzer we want to pretend that such functions do not return.

This attribute is not recognized/supported by some clang-tidy checks, at least the bugprone-unchecked-optional-access check where I have run into this:

#include <optional>

void myAssertFailedHandler() __attribute__((analyzer_noreturn));

#define MY_ASSERT(EXPR) \
  if (!(EXPR)) { \
    myAssertFailedHandler(); \
  }

void foo(const std::optional<int>& o)
{
    MY_ASSERT(o);
    // error: unchecked access to optional value [bugprone-unchecked-optional
    *o;
}

Changing the attribute to noreturn will cause the check to pass.

Should clang-tidy's data-flow analysis understand/respect this attribute?

[steakhal]

I think it would make sense if it would trust that attribute.

[ymand]

We can take a look. But, the complication is that dataflow doesn't understand noreturn directly -- it relies on the CFG for understanding it. So, you'd need to modify the CFG accordingly, which may be more controversial (I don't know offhand how it's used by other projects). But, enabling this at least optionally in the CFG seems like a good idea.

[MikeWeller]

I can't claim to be very experienced with the LLVM code base (I have done a bit of stuff based on lib tooling) but I am happy to help out here if needed, and assuming we can come to an agreement on what changes are required and where etc.

[steakhal]

We already handle the noreturn attribute in clang/lib/Analysis/CFG.cpp. I guess, we should just do the same for analyzer_noreturn.

[MikeWeller]

Thanks, I'll take a look in the next few days and see if I can prepare a draft change/PR for further comment/discussion.

[MikeWeller]

Well, the smallest change to get my use case working is something like this:

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 13a3ff52f3eb..5f4ddecd0595 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
@@ -200,3 +200,14 @@ void foo() {
   if (!vec.empty())
     vec[0].x = 0;
 }
+
+void myAssertFailureHandler() __attribute__((analyzer_noreturn));
+
+void foo2(const absl::optional<int>& opt)
+{
+  if (!opt) {
+    myAssertFailureHandler();
+  }
+
+  *opt;
+}
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 0a9c9e17d3f9..282b3cae30a0 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2586,6 +2586,10 @@ public:
   /// an attribute on its declaration or its type.
   bool isNoReturn() const;
 
+  /// Determines whether this function is 'noreturn' via the '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 1b99c435aebb..c3e9fe769601 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3533,6 +3533,19 @@ bool FunctionDecl::isNoReturn() const {
   return false;
 }
 
+bool FunctionDecl::isAnalyzerNoReturn() const {
+  if (hasAttr<AnalyzerNoReturnAttr>()) {
+    return true;
+  }
+
+// NOCHECKIN: do we need to do something similar to 'isNoReturn'?
+//
+//  if (auto *FnTy = getType()->getAs<FunctionType>())
+//    return FnTy->getNoReturnAttr();
+
+  return false;
+}
+
 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 64e6155de090..6fe0003ca7bb 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -2725,7 +2725,7 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
     if (!FD->isVariadic())
       findConstructionContextsForArguments(C);
 
-    if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context))
+    if (FD->isNoReturn() || FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context))
       NoReturn = true;
     if (FD->hasAttr<NoThrowAttr>())
       AddEHEdge = false;

However the main issue/question here is that the regular noreturn attribute also propagates through a bitmask in FunctionType's getExtInfo(). It's unclear to me whether something similar would be required for analyzer_noreturn.

I'm also not familiar with what components use CFG.cpp and therefore what might be affected generally.

[MikeWeller]

Coming back to this some time later... this is still something I would like to push forward.

The issue, from my last comment, is that I'm unsure what other functionality uses Analysis/CFG.cpp though I assume it is a lot. Ideally I'd like to find somebody who knows this area of the code and can help figure out where else we need to handle analyzer_noreturn, make sure it is consistently supported, and test any other checks/components that may need testing since unchecked-optional-access.cpp is very particular to my use case.

[steakhal]

Coming back to this some time later... this is still something I would like to push forward.

The issue, from my last comment, is that I'm unsure what other functionality uses Analysis/CFG.cpp though I assume it is a lot. Ideally I'd like to find somebody who knows this area of the code and can help figure out where else we need to handle analyzer_noreturn, make sure it is consistently supported, and test any other checks/components that may need testing since unchecked-optional-access.cpp is very particular to my use case.

• clang/lib/Sema/AnalysisBasedWarnings.cpp
• clang/include/clang/Sema/AnalysisBasedWarnings.h
• clang/lib/Analysis/FlowSensitive/* and their corresponding headers
• maybe clang-tidy also refers to the CFG via different things, like ExprSequence.

I'd suggest grepping for the clang CFG header includes to see where it may be used. Or alternatively, just break the factory fn of CFG to see all the callsites.

[MikeWeller]

One thing I noticed while looking around: for more fundamental checks like dereferencing a null pointer, this already seems to handle analyzer_noreturn via

llvm-project/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp

Line 45 in 9889de8

BuildSinks = FD->hasAttr<AnalyzerNoReturnAttr>() || FD->isNoReturn();

and related code.

More interestingly there is a hardcoded list of functions here which are known to be noreturn but aren't declared as such:

llvm-project/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp

Lines 60 to 80 in 9889de8

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

The CFG.cpp-based stuff like the null optional checker will not be using this list of functions. Would be nice if this knowledge could be unified somehow.

[steakhal]

That would make sense.
The idea behing the checker is that at thr time we didn't want to change the CFG for any other users, hence we just patched the checker instead.
I think it makes sense to have this apply globally for all CFG users, thus have a broader impact.
We should ensure we have a way to opt out this though. Who knows what downstream users may use the clang cfg for.

[MikeWeller]

Just sharing one other subtle difference I've found. I'm essentially comparing the behaviour of bugprone-unchecked-optional-access with clang-analyzer-core.NullDereference since they are very similar kinds of checks, but based on different implementations (CFG.cpp vs. NoReturnFunctionChecker.cpp (from what I can tell by deliberately breaking stuff)).

In addition to the knowledge of exit and those other hardcoded functions, clang-analyzer-core.NullDereference also propagates noreturn/analyzer_noreturn information better:

void myAssertFailureHandler() __attribute__((analyzer_noreturn));
void noAttributeFailureHandler()
{
	myAssertFailureHandler();
}

void foo3(int *i)
{
  if (!i) {
    noAttributeFailureHandler();
  }

  *i = 123;
}

noAttributeFailureHandler is correctly treated as noreturn/analyzer_noreturn. However with bugprone-unchecked-optional-access the equivalent test:

void noAttributeFailureHandler() {
  myAssertFailureHandler();
}

void foo3(const absl::optional<int>& o)
{
  if (!o) {
    noAttributeFailureHandler();
  }

  *o;
}

Will not understand that noAttributeFailureHandler is noreturn and will flag up the *o as unchecked.

I think my general approach going forward will be:

• Introduce some minimal changes to CFG.cpp to support analyzer_noreturn without dealing with the above inconsistencies. This will probably be enabled with some kind of setTreatAnalyzerNoReturnAsNoReturn (naming TBD) and turn it on in the appropriate places for clang-tidy checks and anywhere else that should have it, while leaving the behaviour unchanged by default.
• Try to get a better understanding of the code base and how some of the noreturn logic could be unified/reused better between these different checks.

[steakhal]

You have a CallGraph. You could traverse it in reverse-post-order, and deduce from the CFG of every function if it never returns. And if that's the case, you can also propagate the analyzer_noreturn.

That would help bridging the gap between the static analyzer implementation and the dataflow-based implementation.

But be aware of the fact that these two are distinct. They are useful for different objectives. Neither is superior to the other.