[clang] Thread safety analysis: Fix substitution for operator calls (PR #116487)

[Aaron Puchert via cfe-commits]

https://github.com/aaronpuchert created https://github.com/llvm/llvm-project/pull/116487

For operator calls that go to methods we need to substitute the first parameter for "this" and the following parameters into the function parameters, instead of substituting all of them into the parameters.

This revealed an issue about lambdas. An existing test accidentally worked because the substitution bug was covered by a speciality of lambdas: a CXXThisExpr in a lambda CXXMethodDecl does not refer to the implicit this argument of the method, but to a captured "this" from the context the lambda was created in. This can happen for operator calls, where it worked due to the substitution bug (we treated the implicit this argument incorrectly as parameter), and for regular calls (i.e. obj.operator()(args) instead of obj(args)), where it didn't work.

The correct fix seems to be to clear the self-argument on a lambda call. Lambdas can only capture "this" inside methods, and calls to the lambda in that scope cannot substitute anything for "this".

>From 685e1b00ade1e4b27a2b4e505107f4c15d3b6c21 Mon Sep 17 00:00:00 2001
From: Aaron Puchert <aaron.puchert at sap.com>
Date: Fri, 15 Nov 2024 23:23:07 +0100
Subject: [PATCH] Thread safety analysis: Fix substitution for operator calls

For operator calls that go to methods we need to substitute the first
parameter for "this" and the following parameters into the function
parameters, instead of substituting all of them into the parameters.

This revealed an issue about lambdas. An existing test accidentally
worked because the substitution bug was covered by a speciality of
lambdas: a CXXThisExpr in a lambda CXXMethodDecl does not refer to the
implicit this argument of the method, but to a captured "this" from the
context the lambda was created in. This can happen for operator calls,
where it worked due to the substitution bug (we treated the implicit
this argument incorrectly as parameter), and for regular calls (i.e.
obj.operator()(args) instead of obj(args)), where it didn't work.

The correct fix seems to be to clear the self-argument on a lambda call.
Lambdas can only capture "this" inside methods, and calls to the lambda
in that scope cannot substitute anything for "this".
---
 clang/lib/Analysis/ThreadSafetyCommon.cpp     | 20 +++++++++++--
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 29 +++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index cbcfefdc525490..1df89802baa9ce 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -135,14 +135,30 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
     Ctx.NumArgs   = CE->getNumArgs();
     Ctx.FunArgs   = CE->getArgs();
   } else if (const auto *CE = dyn_cast<CallExpr>(DeclExp)) {
-    Ctx.NumArgs = CE->getNumArgs();
-    Ctx.FunArgs = CE->getArgs();
+    // Calls to operators that are members need to be treated like member calls.
+    if (isa<CXXOperatorCallExpr>(CE) && isa<CXXMethodDecl>(D)) {
+      Ctx.SelfArg = CE->getArg(0);
+      Ctx.SelfArrow = false;
+      Ctx.NumArgs = CE->getNumArgs() - 1;
+      Ctx.FunArgs = CE->getArgs() + 1;
+    } else {
+      Ctx.NumArgs = CE->getNumArgs();
+      Ctx.FunArgs = CE->getArgs();
+    }
   } else if (const auto *CE = dyn_cast<CXXConstructExpr>(DeclExp)) {
     Ctx.SelfArg = nullptr;  // Will be set below
     Ctx.NumArgs = CE->getNumArgs();
     Ctx.FunArgs = CE->getArgs();
   }
 
+  // Usually we want to substitute the self-argument for "this", but lambdas
+  // are an exception: "this" on or in a lambda call operator doesn't refer
+  // to the lambda, but to captured "this" in the context it was created in.
+  // This can happen for operator calls and member calls, so fix it up here.
+  if (const auto *CMD = dyn_cast<CXXMethodDecl>(D))
+    if (CMD->getParent()->isLambda())
+      Ctx.SelfArg = nullptr;
+
   if (Self) {
     assert(!Ctx.SelfArg && "Ambiguous self argument");
     assert(isa<FunctionDecl>(D) && "Self argument requires function");
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 8477200456d985..3c52c8165d8529 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1593,8 +1593,12 @@ namespace substitution_test {
     void unlockData()  UNLOCK_FUNCTION(mu);
 
     void doSomething() EXCLUSIVE_LOCKS_REQUIRED(mu)  { }
+    void operator()()  EXCLUSIVE_LOCKS_REQUIRED(mu)  { }
+
+    MyData operator+(const MyData& other) const SHARED_LOCKS_REQUIRED(mu, other.mu);
   };
 
+  MyData operator-(const MyData& a, const MyData& b) SHARED_LOCKS_REQUIRED(a.mu, b.mu);
 
   class DataLocker {
   public:
@@ -1607,6 +1611,27 @@ namespace substitution_test {
   public:
     void foo(MyData* d) EXCLUSIVE_LOCKS_REQUIRED(d->mu) { }
 
+    void subst(MyData& d) {
+      d.doSomething(); // expected-warning {{calling function 'doSomething' requires holding mutex 'd.mu' exclusively}}
+      d();             // expected-warning {{calling function 'operator()' requires holding mutex 'd.mu' exclusively}}
+      d.operator()();  // expected-warning {{calling function 'operator()' requires holding mutex 'd.mu' exclusively}}
+
+      d.lockData();
+      d.doSomething();
+      d();
+      d.operator()();
+      d.unlockData();
+    }
+
+    void binop(MyData& a, MyData& b) EXCLUSIVE_LOCKS_REQUIRED(a.mu) {
+      a + b; // expected-warning {{calling function 'operator+' requires holding mutex 'b.mu'}}
+             // expected-note at -1 {{found near match 'a.mu'}}
+      b + a; // expected-warning {{calling function 'operator+' requires holding mutex 'b.mu'}}
+             // expected-note at -1 {{found near match 'a.mu'}}
+      a - b; // expected-warning {{calling function 'operator-' requires holding mutex 'b.mu'}}
+             // expected-note at -1 {{found near match 'a.mu'}}
+    }
+
     void bar1(MyData* d) {
       d->lockData();
       foo(d);
@@ -5172,9 +5197,13 @@ class Foo {
     };
 
     func1();  // expected-warning {{calling function 'operator()' requires holding mutex 'mu_' exclusively}}
+    func1.operator()();  // expected-warning {{calling function 'operator()' requires holding mutex 'mu_' exclusively}}
     func2();
+    func2.operator()();
     func3();
     mu_.Unlock();
+    func3.operator()();
+    mu_.Unlock();
   }
 };