[Clang] SemaFunctionEffects: When verifying a function, ignore any conditional noexcept expression.

[dougsonos]

Would have been part of my last PR (#142666) if I'd found it a few hours sooner.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

Would have been part of my last PR (#142666) if I'd found it a few hours sooner.

---

Full diff: https://github.com/llvm/llvm-project/pull/115342.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaFunctionEffects.cpp (+17-2)
• (modified) clang/test/Sema/attr-nonblocking-constraints.cpp (+10-2)

diff --git a/clang/lib/Sema/SemaFunctionEffects.cpp b/clang/lib/Sema/SemaFunctionEffects.cpp
index ab728f24d8a271..70f6f9b6784cd8 100644
--- a/clang/lib/Sema/SemaFunctionEffects.cpp
+++ b/clang/lib/Sema/SemaFunctionEffects.cpp
@@ -972,6 +972,7 @@ class Analyzer {
     CallableInfo &CurrentCaller;
     ViolationSite VSite;
     const Expr *TrailingRequiresClause = nullptr;
+    const Expr *NoexceptExpr = nullptr;
 
     FunctionBodyASTVisitor(Analyzer &Outer,
                            PendingFunctionAnalysis &CurrentFunction,
@@ -986,9 +987,22 @@ class Analyzer {
       if (auto *Dtor = dyn_cast<CXXDestructorDecl>(CurrentCaller.CDecl))
         followDestructor(dyn_cast<CXXRecordDecl>(Dtor->getParent()), Dtor);
 
-      if (auto *FD = dyn_cast<FunctionDecl>(CurrentCaller.CDecl))
+      if (auto *FD = dyn_cast<FunctionDecl>(CurrentCaller.CDecl)) {
         TrailingRequiresClause = FD->getTrailingRequiresClause();
 
+        // Note that FD->getType->getAs<FunctionProtoType>() can yield a
+        // noexcept Expr which has been boiled down to a constant expression.
+        // Going through the TypeSourceInfo obtains the actual expression which
+        // will be traversed as part of the function -- unless we capture it
+        // here and have TraverseStmt skip it.
+        if (TypeSourceInfo *TSI = FD->getTypeSourceInfo()) {
+          FunctionProtoTypeLoc TL =
+              TSI->getTypeLoc().getAs<FunctionProtoTypeLoc>();
+          if (const FunctionProtoType *FPT = TL.getTypePtr())
+            NoexceptExpr = FPT->getNoexceptExpr();
+        }
+      }
+
       // Do an AST traversal of the function/block body
       TraverseDecl(const_cast<Decl *>(CurrentCaller.CDecl));
     }
@@ -1269,7 +1283,8 @@ class Analyzer {
       // We skip the traversal of lambdas (beyond their captures, see
       // TraverseLambdaExpr below), so just caching this from our constructor
       // should suffice.
-      if (Statement != TrailingRequiresClause)
+      // The exact same is true for a conditional `noexcept()` clause.
+      if (Statement != TrailingRequiresClause && Statement != NoexceptExpr)
         return Base::TraverseStmt(Statement);
       return true;
     }
diff --git a/clang/test/Sema/attr-nonblocking-constraints.cpp b/clang/test/Sema/attr-nonblocking-constraints.cpp
index 19a4c3b7942b12..169c42ee35fe83 100644
--- a/clang/test/Sema/attr-nonblocking-constraints.cpp
+++ b/clang/test/Sema/attr-nonblocking-constraints.cpp
@@ -388,7 +388,7 @@ void nb26() [[clang::nonblocking]] {
 	abort_wrapper(); // no diagnostic
 }
 
-// --- Make sure we don't traverse a requires clause. ---
+// --- Make sure we don't traverse requires and noexcept clauses. ---
 
 // Apparently some requires clauses are able to be collapsed into a constant before the nonblocking
 // analysis sees any function calls. This example (extracted from a real-world case where
@@ -420,7 +420,9 @@ class expected {
   constexpr expected()
     {}
 
+  // This is a deliberate corruption of the real implementation for simplicity.
   constexpr expected(const expected&)
+    noexcept(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err>)
     requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err>)
   = default;
 };
@@ -428,11 +430,17 @@ class expected {
 void test() [[clang::nonblocking]]
 {
 	expected<int, int> a;
-	auto b = a;
+	auto b = a;            // Copy constructor.
 }
 
 } // namespace ExpectedTest
 
+// Make sure that simple type traits don't cause violations.
+
+void nb27() [[clang::nonblocking]] {
+	bool x = __is_constructible(int, const int&);
+}
+
 // --- nonblocking implies noexcept ---
 #pragma clang diagnostic warning "-Wperf-constraint-implies-noexcept"
 

[Sirraide]

At this point, I wonder if it wouldn’t be easier to just reimplement function traversal and not call TraverseDecl here rather than add more and more members that we then need to explicitly avoid visiting later on. From a cursory look, DEF_TRAVERSE_DECL, TraverseFunctionHelper (which is what’s used for most function-like things), and the traversal code for BlockDecls seem to have a lot of code paths we don’t really care about in this case.

[Sirraide]

The current approach should work too, though. So if you want to explore that approach separately (or I can also take a look at that since I’ve been refactoring AST visitors lately), then that’s also fine imo

[dougsonos]

That's a good thought. Looking at TraverseFunctionHelper:

• the template arguments should be ignorable.
• need to traverse the function's type because it contains the parameters -- e.g. I caught someone passing a vector by value instead of by reference and that showed up first as a call to the vector's destructor, located in the parameter list.
• but the function's type is where the noexcept expression comes from.
• can skip the trailing return clause.
• need to traverse the constructor initializers.
• need to traverse the body of course.

I'm not excited to tear this apart at the moment but maybe the next bug that comes up in this area can drive an improvement.

[Sirraide]

I think something like this would be a simpler, more self-contained test:

constexpr bool foo() [[clang::nonblocking(false)]] { return true; }
void f() noexcept(foo()) [[clang::nonblocking]] {}

This currently causes a warning on godbolt.

[Sirraide]

Speaking of constant-evaluation, this currently causes warnings:

constexpr bool foo() [[clang::nonblocking]] { 
    if consteval {
        auto p = new int;
        delete p;
    }

    return true; 
}

I think simply ignoring the body of a consteval if (or the else clause if there is one of a !consteval if) should be enough; that doesn’t have to be in this pr tho.

[Sirraide]

@dougsonos Oh also, I think I may have mentioned this before, but maybe you might want to email Chris to obtain commit access since you’re the main person maintaining all of the function effects code ;Þ

[dougsonos]

@dougsonos Oh also, I think I may have mentioned this before, but maybe you might want to email Chris to obtain commit access since you’re the main person maintaining all of the function effects code ;Þ

Thanks, I wrote to Chris.