[Clang] FunctionEffect analysis was missing a CXXBindTemporaryExpr's implicit call to a destructor.

[dougsonos]

This example is reduced from a discovery: resetting a shared pointer from a nonblocking function is not diagnosed.

void nb23()
{
	struct X {
		int *ptr = nullptr;
		X() {}
		~X() { delete ptr; }
	};

	auto inner = []() [[clang::nonblocking]] {
		X();
	};
}

shared_ptr<T>::reset() creates a temporary shared_ptr and swaps it with its current state. The temporary shared_ptr constructor is nonblocking but its destructor potentially deallocates memory and is unsafe.

Analysis was ignoring the implicit call in the AST to destroy the temporary.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

This example is reduced from a discovery: resetting a shared pointer from a nonblocking function is not diagnosed.

void nb23()
{
	struct X {
		int *ptr = nullptr;
		X() {}
		~X() { delete ptr; }
	};

	auto inner = []() [[clang::nonblocking]] {
		X();
	};
}

shared_ptr<T>::reset() creates a temporary shared_ptr and swaps it with its current state. The temporary shared_ptr constructor is nonblocking but its destructor potentially deallocates memory and is unsafe.

Analysis was ignoring the implicit call in the AST to destroy the temporary.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaFunctionEffects.cpp (+8)
• (modified) clang/test/Sema/attr-nonblocking-constraints.cpp (+14)

diff --git a/clang/lib/Sema/SemaFunctionEffects.cpp b/clang/lib/Sema/SemaFunctionEffects.cpp
index 8590ee831084f..468f157f2e0bd 100644
--- a/clang/lib/Sema/SemaFunctionEffects.cpp
+++ b/clang/lib/Sema/SemaFunctionEffects.cpp
@@ -1271,7 +1271,15 @@ class Analyzer {
       const CXXConstructorDecl *Ctor = Construct->getConstructor();
       CallableInfo CI(*Ctor);
       followCall(CI, Construct->getLocation());
+      return true;
+    }
 
+    bool VisitCXXBindTemporaryExpr(CXXBindTemporaryExpr *BTE) override {
+      const CXXDestructorDecl* Dtor = BTE->getTemporary()->getDestructor();
+      if (Dtor != nullptr) {
+        CallableInfo CI(*Dtor);
+        followCall(CI, BTE->getBeginLoc());
+      }
       return true;
     }
 
diff --git a/clang/test/Sema/attr-nonblocking-constraints.cpp b/clang/test/Sema/attr-nonblocking-constraints.cpp
index b26a945843696..4b831c0a6be09 100644
--- a/clang/test/Sema/attr-nonblocking-constraints.cpp
+++ b/clang/test/Sema/attr-nonblocking-constraints.cpp
@@ -354,6 +354,20 @@ struct Unsafe {
   Unsafe(float y) [[clang::nonblocking]] : Unsafe(int(y)) {} // expected-warning {{constructor with 'nonblocking' attribute must not call non-'nonblocking' constructor 'Unsafe::Unsafe'}}
 };
 
+// Exercise the case of a temporary with a safe constructor and unsafe destructor.
+void nb23()
+{
+	struct X {
+		int *ptr = nullptr;
+		X() {}
+		~X() { delete ptr; } // expected-note {{destructor cannot be inferred 'nonblocking' because it allocates or deallocates memory}}
+	};
+
+	auto inner = []() [[clang::nonblocking]] {
+		X(); // expected-warning {{lambda with 'nonblocking' attribute must not call non-'nonblocking' destructor 'nb23()::X::~X'}}
+	};
+}
+
 struct DerivedFromUnsafe : public Unsafe {
   DerivedFromUnsafe() [[clang::nonblocking]] {} // expected-warning {{constructor with 'nonblocking' attribute must not call non-'nonblocking' constructor 'Unsafe::Unsafe'}}
   DerivedFromUnsafe(int x) [[clang::nonblocking]] : Unsafe(x) {} // expected-warning {{constructor with 'nonblocking' attribute must not call non-'nonblocking' constructor 'Unsafe::Unsafe'}}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dougsonos]

TODO: Consider auto x = []() { /* lambda body */ };
which becomes VarDecl holding ExprWithCleanups holding CXXBindTemporaryExpr.

We currently look for a destructor on a VarDecl but I suspect that becomes a redundant check with this change. Need to verify.

[Sirraide]

TODO: Consider auto x = []() { /* lambda body */ }; which becomes VarDecl holding ExprWithCleanups holding CXXBindTemporaryExpr.

Visiting CXXBindTemporaryExpr looks like the right approach. Speaking of lambdas, while looking into this, I also found some more issues with lambda captures I think:

struct S { ~S(); };
void f() {
    S s;
    [&]() [[clang::nonblocking]] {
        [s]{ auto x = &s; }();
        [=]{ auto x = &s; }();
    }();
}

These are all by-value captures, meaning that we need to call ~S() after evaluating the two inner lambdas, but we don't seem to diagnose that at the moment (https://godbolt.org/z/815hj1bKM). This might well be a different issue though.

We currently look for a destructor on a VarDecl but I suspect that becomes a redundant check with this change. Need to verify.

That might no longer be necessary yeah.

[dougsonos]

struct S { ~S(); };
void f() {
    S s;
    [&]() [[clang::nonblocking]] {
        [s]{ auto x = &s; }();
        [=]{ auto x = &s; }();
    }();
}

With this patch, here we get two warnings that S's destructor is being called from the nonblocking lambda.

[Sirraide]

With this patch, here we get two warnings that S's destructor is being called from the nonblocking lambda.

Oh, nice; add that as a test case then

[dougsonos]

Added that as test nb24(), thanks.

The possible redundancy between CXXBindTemporaryExpr and VarDecl with a destructor is tricky. Most of the time temporaries seem to be orthogonal to VarDecls -- it's clearly not as simple as removing the check on VarDecl.

[Sirraide]

The possible redundancy between CXXBindTemporaryExpr and VarDecl with a destructor is tricky. Most of the time temporaries seem to be orthogonal to VarDecls -- it's clearly not as simple as removing the check on VarDecl

Can we simply keep both checks? Or does that cause too many duplicate diagnostics?

[Sirraide]

The possible redundancy between CXXBindTemporaryExpr and VarDecl with a destructor is tricky. Most of the time temporaries seem to be orthogonal to VarDecls -- it's clearly not as simple as removing the check on VarDecl

Can we simply keep both checks? Or does that cause too many duplicate diagnostics?

Alternatively, can we skip checking the type of the VarDecl if the initialiser is a CXXBindTemporaryExpr?

[dougsonos]

Yeah, we can keep both checks (and that's how things stand at the moment on the branch). I'm having trouble finding an example where they are redundant.

[Sirraide]

Yeah, we can keep both checks (and that's how things stand at the moment on the branch). I'm having trouble finding an example where they are redundant.

Sgtm; we can come back to this if/when someone finds an example that causes duplicate warnings

[dougsonos]

Aha, I dumped a big AST to JSON, searched for a CXXBindTemporaryExpr inside a VarDecl and came up with this example:

/Users/doug/Desktop/makestring.cpp:10:7: warning: function with 'nonblocking' attribute must not call non-'nonblocking' destructor 'String::~String' [-Wfunction-effects]
   10 |         auto str = String::make();
      |              ^
/Users/doug/Desktop/makestring.cpp:10:13: warning: function with 'nonblocking' attribute must not call non-'nonblocking' destructor 'String::~String' [-Wfunction-effects]
   10 |         auto str = String::make();
      |                    ^

It may be a little annoying to get two warnings but technically I think there really are two implicit calls to the destructor. (right? the result of String::make() might be getting moved-from but there's still an implicit destructor.)

[Sirraide]

the result of String::make() might be getting moved-from

Only if it returns an xvalue; if it returns a prvalue, it’s directly constructed into str, so we only get a single destructor call.

[Sirraide]

the result of String::make() might be getting moved-from

Only if it returns an xvalue; if it returns a prvalue, it’s directly constructed into str, so we only get a single destructor call.

In this case, the initialiser contains a CXXBindTemporaryExpr if it’s a prvalue; I wonder if it would suffice to check if the initialiser of the VarDecl is an ExprWithCleanups that wraps a CXXBindTemporaryExpr and not visit the type of the VarDecl in that case.

[Sirraide]

In this case, the initialiser contains a CXXBindTemporaryExpr if it’s a prvalue; I wonder if it would suffice to check if the initialiser of the VarDecl is an ExprWithCleanups that wraps a CXXBindTemporaryExpr and not visit the type of the VarDecl in that case.

Actually, that doesn’t work too well if someone does weird things like:

struct S { ~S(); };
S s{S(S())};

Obviously no-one writes code like this, but even if you remove some of the unnecessary parts here, the AST is still sufficiently complex to where checking all possible patterns becomes annoying.

[dougsonos]

Obviously no-one writes code like this, but even if you remove some of the unnecessary parts here, the AST is still sufficiently complex to where checking all possible patterns becomes annoying.

OK, I'm inclined to think the rare duplicate warnings are tolerable for now (they're at least at different source locations), even if they're not correct.

[Sirraide]

LGTM

One thing I forgot to mention with the other function effects prs: we should add a release note for them. It doesn’t have to be anything too specific; just something along the lines of ‘Fixed a number of false positives and false negatives around -Wfunction-effects’ is enough imo.

[Sirraide]

(forgot to select ‘approve’ instead of ‘comment’)

[dougsonos]

Thanks! I added a release note with this PR. (There's one more, #166078 )