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.