From 27c10337831c94ef59f8790f6ca1c3d1b66b4494 Mon Sep 17 00:00:00 2001 From: Rashmi Mudduluru Date: Wed, 12 Jul 2023 15:04:45 -0700 Subject: [PATCH] [WIP][-Wunsafe-buffer-usage] Handle lambda expressions within a method. Differential Revision: https://reviews.llvm.org/D150386 --- clang/docs/LibASTMatchersReference.html | 37 ++++++++++++ clang/docs/ReleaseNotes.rst | 2 + clang/include/clang/ASTMatchers/ASTMatchers.h | 39 +++++++++++++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp | 4 ++ clang/lib/ASTMatchers/Dynamic/Registry.cpp | 2 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 16 ++++-- ...n-unsafe-buffer-usage-fixits-pointer-access.cpp | 67 ++++++++++++++++++++++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp | 39 ++++++++++++- 8 files changed, 200 insertions(+), 6 deletions(-) diff --git a/clang/docs/LibASTMatchersReference.html b/clang/docs/LibASTMatchersReference.html index bde8ac7d..b4f282f 100644 --- a/clang/docs/LibASTMatchersReference.html +++ b/clang/docs/LibASTMatchersReference.html @@ -1257,6 +1257,43 @@ addrLabelExpr() +Matcher<Stmt>arrayInitIndexExprMatcher<ArrayInitIndexExpr>... +
The arrayInitIndexExpr consists of two subexpressions: a common expression
+(the source array) that is evaluated once up-front, and a per-element initializer
+that runs once for each array element. Within the per-element initializer,
+the current index may be obtained via an ArrayInitIndexExpr.
+
+Given
+  void testStructBinding() {
+    int a[2] = {1, 2};
+    auto [x, y] = a;
+  }
+arrayInitIndexExpr() matches the array index that implicitly iterates
+over the array `a` to copy each element to the anonymous array
+that backs the structured binding `[x, y]` elements of which are
+referred to by their aliases `x` and `y`.
+
+ + +Matcher<Stmt>arrayInitLoopExprMatcher<ArrayInitLoopExpr>... +
Matches a loop initializing the elements of an array in a number of contexts:
+ * in the implicit copy/move constructor for a class with an array member
+ * when a lambda-expression captures an array by value
+ * when a decomposition declaration decomposes an array
+
+Given
+  void testLambdaCapture() {
+    int a[10];
+    auto Lam1 = [a]() {
+      return;
+    };
+  }
+arrayInitLoopExpr() matches the implicit loop that initializes each element of
+the implicit array field inside the lambda object, that represents the array `a`
+captured by value.
+
+ + Matcher<Stmt>arraySubscriptExprMatcher<ArraySubscriptExpr>... 
Matches array subscript expressions.
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ef1cc89..f1d098e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -943,6 +943,8 @@ AST Matchers
 
 - The ``hasBody`` matcher now matches coroutine body nodes in
   ``CoroutineBodyStmts``.
+  
+- Add ``arrayInitIndexExpr`` and ``arrayInitLoopExpr`` matchers.
 
 clang-format
 ------------
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index b698365..a931313 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1971,6 +1971,45 @@ extern const internal::VariadicDynCastAllOfMatcher
 extern const internal::VariadicDynCastAllOfMatcher
     cxxNoexceptExpr;
 
+/// Matches a loop initializing the elements of an array in a number of contexts:
+///  * in the implicit copy/move constructor for a class with an array member
+///  * when a lambda-expression captures an array by value
+///  * when a decomposition declaration decomposes an array
+///
+/// Given
+/// \code
+///   void testLambdaCapture() {
+///     int a[10];
+///     auto Lam1 = [a]() {
+///       return;
+///     };
+///   }
+/// \endcode
+/// arrayInitLoopExpr() matches the implicit loop that initializes each element of
+/// the implicit array field inside the lambda object, that represents the array `a`
+/// captured by value.
+extern const internal::VariadicDynCastAllOfMatcher
+    arrayInitLoopExpr;
+
+/// The arrayInitIndexExpr consists of two subexpressions: a common expression
+/// (the source array) that is evaluated once up-front, and a per-element initializer
+/// that runs once for each array element. Within the per-element initializer,
+/// the current index may be obtained via an ArrayInitIndexExpr.
+///
+/// Given
+/// \code
+///   void testStructBinding() {
+///     int a[2] = {1, 2};
+///     auto [x, y] = a;
+///   }
+/// \endcode
+/// arrayInitIndexExpr() matches the array index that implicitly iterates
+/// over the array `a` to copy each element to the anonymous array
+/// that backs the structured binding `[x, y]` elements of which are
+/// referred to by their aliases `x` and `y`.
+extern const internal::VariadicDynCastAllOfMatcher
+    arrayInitIndexExpr;
+
 /// Matches array subscript expressions.
 ///
 /// Given
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 26e40c4..ad6b20f 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -882,6 +882,10 @@ const internal::VariadicDynCastAllOfMatcher
     cxxNoexceptExpr;
 const internal::VariadicDynCastAllOfMatcher
     arraySubscriptExpr;
+const internal::VariadicDynCastAllOfMatcher
+    arrayInitIndexExpr;
+const internal::VariadicDynCastAllOfMatcher
+    arrayInitLoopExpr;
 const internal::VariadicDynCastAllOfMatcher
     cxxDefaultArgExpr;
 const internal::VariadicDynCastAllOfMatcher
diff --git a/clang/lib/ASTMatchers/Dynamic/Registry.cpp b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
index 995c082..d359445 100644
--- a/clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -134,6 +134,8 @@ RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(allOf);
   REGISTER_MATCHER(anyOf);
   REGISTER_MATCHER(anything);
+  REGISTER_MATCHER(arrayInitIndexExpr);
+  REGISTER_MATCHER(arrayInitLoopExpr);
   REGISTER_MATCHER(argumentCountIs);
   REGISTER_MATCHER(argumentCountAtLeast);
   REGISTER_MATCHER(arraySubscriptExpr);
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 5295d9d..54569b4 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -119,9 +119,6 @@ public:
       return true;
     if (!match(*Node))
       return false;
-    // To skip callables:
-    if (isa(Node))
-      return true;
     return VisitorBase::TraverseStmt(Node);
   }
 
@@ -467,7 +464,9 @@ public:
       return stmt(arraySubscriptExpr(
             hasBase(ignoringParenImpCasts(
               anyOf(hasPointerType(), hasArrayType()))),
-            unless(hasIndex(integerLiteral(equals(0)))))
+            unless(hasIndex(
+                anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+             )))
             .bind(ArraySubscrTag));
     // clang-format on
   }
@@ -2209,6 +2208,13 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler,
                                    bool EmitSuggestions) {
   assert(D && D->getBody());
+  
+  // We do not want to visit a Lambda expression defined inside a method independently.
+  // Instead, it should be visited along with the outer method.
+  if (const auto *fd = dyn_cast(D)) {
+    if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+      return;
+  }
 
   // Do not emit fixit suggestions for functions declared in an
   // extern "C" block.
@@ -2254,7 +2260,7 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
        it != FixablesForAllVars.byVar.cend();) {
       // FIXME: need to deal with global variables later
       if ((!it->first->isLocalVarDecl() && !isa(it->first)) ||
-          Tracker.hasUnclaimedUses(it->first)) {
+          Tracker.hasUnclaimedUses(it->first) || it->first->isInitCapture()) {
       it = FixablesForAllVars.byVar.erase(it);
     } else {
       ++it;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index 0235dce..f1b92ae 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -110,3 +110,70 @@ void unsafe_method_invocation_double_param() {
   m1(q, q, 8);
 }
 
+void unsafe_access_in_lamda() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  auto my_lambda = [&](){
+    p[5] = 10;
+  };
+
+  foo(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+}
+
+void fixits_in_lamda() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  auto my_lambda = [&](){
+    foo(p);
+    // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()"
+  };
+
+  p[5] = 10;
+}
+
+// FIXME: Emit fixits for lambda captured variables
+void fixits_in_lambda_capture() {
+  auto p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  
+  auto my_lambda = [p](){ // No fixits emitted here.
+    foo(p);
+  };
+  
+  p[5] = 10;
+}
+
+void fixits_in_lambda_capture_reference() {
+  auto p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+ 
+  auto my_lambda = [&p](){ // No fixits emitted here.
+    foo(p);
+  };
+
+  p[5] = 10;
+}
+
+void fixits_in_lambda_capture_rename() {
+  auto p = new int[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span p"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  
+  auto my_lambda = [x = p](){ // No fixits emitted here.
+    foo(x);
+  };
+
+  p[5] = 10;
+} 
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index b6f1675..fd3d6cbd 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -166,7 +166,6 @@ int garray[10];     // expected-warning{{'garray' is an unsafe buffer that does
 int * gp = garray;  // expected-warning{{'gp' is an unsafe pointer used for buffer access}}
 int gvar = gp[1];   // FIXME: file scope unsafe buffer access is not warned
 
-// FIXME: Add test for lambda capture with initializer. E. g. auto Lam = [new_p = p]() {...
 void testLambdaCaptureAndGlobal(int * p) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
@@ -175,6 +174,44 @@ void testLambdaCaptureAndGlobal(int * p) {
     return p[1]           // expected-note{{used in buffer access here}}
       + a[1] + garray[1]  // expected-note2{{used in buffer access here}}
       + gp[1];            // expected-note{{used in buffer access here}}
+
+  };
+}
+
+auto file_scope_lambda = [](int *ptr) {
+  // expected-warning@-1{{'ptr' is an unsafe pointer used for buffer access}}
+  
+  ptr[5] = 10;  // expected-note{{used in buffer access here}}
+};
+
+void testLambdaCapture() {
+  int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+  int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+  int c[10];
+
+  auto Lam1 = [a]() {
+    return a[1];           // expected-note{{used in buffer access here}}
+  };
+
+  auto Lam2 = [x = b[3]]() { // expected-note{{used in buffer access here}}
+    return x;
+  };
+
+  auto Lam = [x = c]() { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+    return x[3]; // expected-note{{used in buffer access here}}
+  };
+}
+
+void testLambdaImplicitCapture() {
+  int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+  int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+  
+  auto Lam1 = [=]() {
+    return a[1];           // expected-note{{used in buffer access here}}
+  };
+  
+  auto Lam2 = [&]() {
+    return b[1];           // expected-note{{used in buffer access here}}
   };
 }
 
-- 
2.7.4