Skip to content

[webkit.UncountedLambdaCapturesChecker] Fix a regression that [[noescape]] on a member function no longer works. #126016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented Feb 6, 2025

We should skip the first argument for CXXOperatorCallExpr, not other kinds of calls.

…ape]] on a member function no longer works.

We should skip the first argument for CXXOperatorCallExpr, not other kinds of calls.
@rniwa rniwa requested a review from t-rasmud February 6, 2025 06:55
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Feb 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

We should skip the first argument for CXXOperatorCallExpr, not other kinds of calls.


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp (+1-3)
  • (modified) clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp (+23)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 53ef423bd82e7e2..a56f48c83c660a9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -109,9 +109,7 @@ class UncountedLambdaCapturesChecker
       bool VisitCallExpr(CallExpr *CE) override {
         checkCalleeLambda(CE);
         if (auto *Callee = CE->getDirectCallee()) {
-          unsigned ArgIndex = 0;
-          if (auto *CXXCallee = dyn_cast<CXXMethodDecl>(Callee))
-            ArgIndex = CXXCallee->isInstance();
+          unsigned ArgIndex = isa<CXXOperatorCallExpr>(CE);
           bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee);
           for (auto *Param : Callee->parameters()) {
             if (ArgIndex >= CE->getNumArgs())
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
index 2a1a164557cdbe7..b6e84769fdf7247 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -63,6 +63,18 @@ template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::Callab
     return Function<Out(In...)>(impl, Function<Out(In...)>::Adopt);
 }
 
+template <typename KeyType, typename ValueType>
+class HashMap {
+public:
+  HashMap();
+  HashMap([[clang::noescape]] const Function<ValueType()>&);
+  void ensure(const KeyType&, [[clang::noescape]] const Function<ValueType()>&);
+  bool operator+([[clang::noescape]] const Function<ValueType()>&) const;
+
+private:
+  ValueType* m_table { nullptr };
+};
+
 } // namespace WTF
 
 struct A {
@@ -268,6 +280,17 @@ struct RefCountableWithLambdaCapturingThis {
       nonTrivial();
     });
   }
+
+  void method_captures_this_in_template_method() {
+    RefCountable* obj = make_obj();
+    WTF::HashMap<int, RefPtr<RefCountable>> nextMap;
+    nextMap.ensure(3, [&] {
+      return obj->next();
+    });
+    nextMap+[&] {
+      return obj->next();
+    };
+  }
 };
 
 struct NonRefCountableWithLambdaCapturingThis {

class HashMap {
public:
HashMap();
HashMap([[clang::noescape]] const Function<ValueType()>&);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have a similar test case for a non-member function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good point! Added a test case for static function & non-member function.

…and a non-member function.
Copy link
Contributor

@t-rasmud t-rasmud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rniwa rniwa merged commit 6f50849 into llvm:main Feb 7, 2025
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Feb 7, 2025

LLVM Buildbot has detected a new failure on builder clang-cmake-x86_64-avx512-win running on avx512-intel64-win while building clang at step 4 "cmake stage 1".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/81/builds/4482

Here is the relevant piece of the build log for the reference
Step 4 (cmake stage 1) failure: 'cmake -G ...' (failure)
'cmake' is not recognized as an internal or external command,
operable program or batch file.

Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025

Verified

This commit was signed with the committer’s verified signature.
Icohedron Deric C.
…ape]] on a member function no longer works. (llvm#126016)
rniwa added a commit to rniwa/llvm-project that referenced this pull request Feb 13, 2025
@rniwa rniwa deleted the fix-lambda-noescape-regression branch February 15, 2025 09:40
devincoughlin pushed a commit to swiftlang/llvm-project that referenced this pull request Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants