Re-land "[-Wunsafe-buffer-usage] Add a new forEachDescendant matcher that skips callable declarations"

This reverts commit 22df4549a3718dcd8b387ba8246978349e4be50c.

After a quick investigation, realizing that the Sanitizer test
failures caused by this patch is not likely to block other
contributors. I re-land this patch before taking a closer look at
those tests so that it won't block the [-Wunsafe-buffer-usage]
development.

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -8,12 +8,112 @@
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "llvm/ADT/SmallVector.h"
 
 using namespace llvm;
 using namespace clang;
 using namespace ast_matchers;
 
+namespace clang::ast_matchers {
+// A `RecursiveASTVisitor` that traverses all descendants of a given node "n"
+// except for those belonging to a different callable of "n".
+class MatchDescendantVisitor
+    : public RecursiveASTVisitor<MatchDescendantVisitor> {
+public:
+  typedef RecursiveASTVisitor<MatchDescendantVisitor> VisitorBase;
+
+  // Creates an AST visitor that matches `Matcher` on all
+  // descendants of a given node "n" except for the ones
+  // belonging to a different callable of "n".
+  MatchDescendantVisitor(const internal::DynTypedMatcher *Matcher,
+                         internal::ASTMatchFinder *Finder,
+                         internal::BoundNodesTreeBuilder *Builder,
+                         internal::ASTMatchFinder::BindKind Bind)
+      : Matcher(Matcher), Finder(Finder), Builder(Builder), Bind(Bind),
+        Matches(false) {}
+
+  // Returns true if a match is found in a subtree of `DynNode`, which belongs
+  // to the same callable of `DynNode`.
+  bool findMatch(const DynTypedNode &DynNode) {
+    Matches = false;
+    if (const Stmt *StmtNode = DynNode.get<Stmt>()) {
+      TraverseStmt(const_cast<Stmt *>(StmtNode));
+      *Builder = ResultBindings;
+      return Matches;
+    }
+    return false;
+  }
+
+  // The following are overriding methods from the base visitor class.
+  // They are public only to allow CRTP to work. They are *not *part
+  // of the public API of this class.
+
+  // For the matchers so far used in safe buffers, we only need to match
+  // `Stmt`s.  To override more as needed.
+
+  bool TraverseDecl(Decl *Node) {
+    if (!Node)
+      return true;
+    if (!match(*Node))
+      return false;
+    // To skip callables:
+    if (isa<FunctionDecl, BlockDecl, ObjCMethodDecl>(Node))
+      return true;
+    // Traverse descendants
+    return VisitorBase::TraverseDecl(Node);
+  }
+
+  bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
+    if (!Node)
+      return true;
+    if (!match(*Node))
+      return false;
+    // To skip callables:
+    if (isa<LambdaExpr>(Node))
+      return true;
+    return VisitorBase::TraverseStmt(Node);
+  }
+
+  bool shouldVisitTemplateInstantiations() const { return true; }
+  bool shouldVisitImplicitCode() const {
+    // TODO: let's ignore implicit code for now
+    return false;
+  }
+
+private:
+  // Sets 'Matched' to true if 'Matcher' matches 'Node'
+  //
+  // Returns 'true' if traversal should continue after this function
+  // returns, i.e. if no match is found or 'Bind' is 'BK_All'.
+  template <typename T> bool match(const T &Node) {
+    internal::BoundNodesTreeBuilder RecursiveBuilder(*Builder);
+
+    if (Matcher->matches(DynTypedNode::create(Node), Finder,
+                         &RecursiveBuilder)) {
+      ResultBindings.addMatch(RecursiveBuilder);
+      Matches = true;
+      if (Bind != internal::ASTMatchFinder::BK_All)
+        return false; // Abort as soon as a match is found.
+    }
+    return true;
+  }
+
+  const internal::DynTypedMatcher *const Matcher;
+  internal::ASTMatchFinder *const Finder;
+  internal::BoundNodesTreeBuilder *const Builder;
+  internal::BoundNodesTreeBuilder ResultBindings;
+  const internal::ASTMatchFinder::BindKind Bind;
+  bool Matches;
+};
+
+AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher<Stmt>, innerMatcher) {
+  MatchDescendantVisitor Visitor(new DynTypedMatcher(innerMatcher), Finder,
+                                 Builder, ASTMatchFinder::BK_All);
+  return Visitor.findMatch(DynTypedNode::create(Node));
+}
+} // namespace clang::ast_matchers
+
 namespace {
 // Because the analysis revolves around variables and their types, we'll need to
 // track uses of variables (aka DeclRefExprs).
@@ -398,7 +498,7 @@ static std::pair<GadgetList, DeclUseTracker> findGadgets(const Decl *D) {
 
   // clang-format off
   M.addMatcher(
-    stmt(forEachDescendant(
+    stmt(forEveryDescendant(
       stmt(anyOf(
         // Add Gadget::matcher() for every gadget in the registry.
 #define GADGET(x)                                                              \

clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fblocks -include %s -verify %s
 #ifndef INCLUDED
 #define INCLUDED
 #pragma clang system_header
@@ -141,15 +141,15 @@ void testStructMembers(struct T * sp, struct T s, T_t * sp2, T_t s2) {
 
 int garray[10];
 int * gp = garray;
-int gvar = gp[1];  // TODO: this is not warned
+int gvar = gp[1];  // FIXME: file scope unsafe buffer access is not warned
 
 void testLambdaCaptureAndGlobal(int * p) {
   int a[10];
 
   auto Lam = [p, a]() {
-    return p[1] // expected-warning2{{unchecked operation on raw buffer in expression}}
+    return p[1] // expected-warning{{unchecked operation on raw buffer in expression}}
       + a[1] + garray[1]
-      + gp[1];  // expected-warning2{{unchecked operation on raw buffer in expression}}
+      + gp[1];  // expected-warning{{unchecked operation on raw buffer in expression}}
   };
 }
 
@@ -213,4 +213,66 @@ void testPointerToMember() {
   foo(S.*p,
       (S.*q)[1]);  // expected-warning{{unchecked operation on raw buffer in expression}}
 }
+
+// test that nested callable definitions are scanned only once
+void testNestedCallableDefinition(int * p) {
+  class A {
+    void inner(int * p) {
+      p++; // expected-warning{{unchecked operation on raw buffer in expression}}
+    }
+
+    static void innerStatic(int * p) {
+      p++; // expected-warning{{unchecked operation on raw buffer in expression}}
+    }
+
+    void innerInner(int * p) {
+      auto Lam = [p]() {
+        int * q = p;
+        q++;   // expected-warning{{unchecked operation on raw buffer in expression}}
+        return *q;
+      };
+    }
+  };
+
+  auto Lam = [p]() {
+    int * q = p;
+    q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+    return *q;
+  };
+
+  auto LamLam = [p]() {
+    auto Lam = [p]() {
+      int * q = p;
+      q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+      return *q;
+    };
+  };
+
+  void (^Blk)(int*) = ^(int *p) {
+    p++;   // expected-warning{{unchecked operation on raw buffer in expression}}
+  };
+
+  void (^BlkBlk)(int*) = ^(int *p) {
+    void (^Blk)(int*) = ^(int *p) {
+      p++;   // expected-warning{{unchecked operation on raw buffer in expression}}
+    };
+    Blk(p);
+  };
+
+  // lambda and block as call arguments...
+  foo( [p]() { int * q = p;
+              q++;  // expected-warning{{unchecked operation on raw buffer in expression}}
+              return *q;
+       },
+       ^(int *p) { p++;   // expected-warning{{unchecked operation on raw buffer in expression}}
+       }
+     );
+}
+
+void testVariableDecls(int * p) {
+  int * q = p++;      // expected-warning{{unchecked operation on raw buffer in expression}}
+  int a[p[1]];        // expected-warning{{unchecked operation on raw buffer in expression}}
+  int b = p[1];       // expected-warning{{unchecked operation on raw buffer in expression}}
+}
+
 #endif