[clang] [Clang] Optimize -Wunsafe-buffer-usage. (PR #125492)

[Ilya Biryukov via cfe-commits]

================
@@ -1986,112 +2360,119 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
   }
 };
 
-/// Scan the function and return a list of gadgets found with provided kits.
-static void findGadgets(const Stmt *S, ASTContext &Ctx,
-                        const UnsafeBufferUsageHandler &Handler,
-                        bool EmitSuggestions, FixableGadgetList &FixableGadgets,
-                        WarningGadgetList &WarningGadgets,
-                        DeclUseTracker &Tracker) {
+class EvaluatedStmtMatcher : public FastMatcher {
 
-  struct GadgetFinderCallback : MatchFinder::MatchCallback {
-    GadgetFinderCallback(FixableGadgetList &FixableGadgets,
-                         WarningGadgetList &WarningGadgets,
-                         DeclUseTracker &Tracker)
-        : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets),
-          Tracker(Tracker) {}
-
-    void run(const MatchFinder::MatchResult &Result) override {
-      // In debug mode, assert that we've found exactly one gadget.
-      // This helps us avoid conflicts in .bind() tags.
-#if NDEBUG
-#define NEXT return
-#else
-      [[maybe_unused]] int numFound = 0;
-#define NEXT ++numFound
-#endif
-
-      if (const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("any_dre")) {
-        Tracker.discoverUse(DRE);
-        NEXT;
-      }
+public:
+  EvaluatedStmtMatcher(WarningGadgetList &WarningGadgets)
+      : WarningGadgets(WarningGadgets) {}
 
-      if (const auto *DS = Result.Nodes.getNodeAs<DeclStmt>("any_ds")) {
-        Tracker.discoverDecl(DS);
-        NEXT;
-      }
+  bool matches(const DynTypedNode &DynNode, ASTContext &Ctx,
+               const UnsafeBufferUsageHandler &Handler) override {
+    const Stmt *S = DynNode.get<Stmt>();
+    if (!S)
+      return false;
 
-      // Figure out which matcher we've found, and call the appropriate
-      // subclass constructor.
-      // FIXME: Can we do this more logarithmically?
-#define FIXABLE_GADGET(name)                                                   \
-  if (Result.Nodes.getNodeAs<Stmt>(#name)) {                                   \
-    FixableGadgets.push_back(std::make_unique<name##Gadget>(Result));          \
-    NEXT;                                                                      \
-  }
-#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+    MatchResult Result;
 #define WARNING_GADGET(name)                                                   \
-  if (Result.Nodes.getNodeAs<Stmt>(#name)) {                                   \
+  if (name##Gadget::matches(S, Ctx, Result) &&                                 \
+      notInSafeBufferOptOut(*S, &Handler)) {                                   \
+    WarningGadgets.push_back(std::make_unique<name##Gadget>(Result));          \
+    return true;                                                               \
+  }
+#define WARNING_OPTIONAL_GADGET(name)                                          \
+  if (name##Gadget::matches(S, Ctx, &Handler, Result) &&                       \
+      notInSafeBufferOptOut(*S, &Handler)) {                                   \
     WarningGadgets.push_back(std::make_unique<name##Gadget>(Result));          \
-    NEXT;                                                                      \
+    return true;                                                               \
   }
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
+    return false;
+  }
 
-      assert(numFound >= 1 && "Gadgets not found in match result!");
-      assert(numFound <= 1 && "Conflicting bind tags in gadgets!");
-    }
+private:
+  WarningGadgetList &WarningGadgets;
+};
 
-    FixableGadgetList &FixableGadgets;
-    WarningGadgetList &WarningGadgets;
-    DeclUseTracker &Tracker;
-  };
+class StmtMatcher : public FastMatcher {
 
-  MatchFinder M;
-  GadgetFinderCallback CB{FixableGadgets, WarningGadgets, Tracker};
-
-  // clang-format off
-  M.addMatcher(
-      stmt(
-        forEachDescendantEvaluatedStmt(stmt(anyOf(
-          // Add Gadget::matcher() for every gadget in the registry.
-#define WARNING_GADGET(x)                                                      \
-          allOf(x ## Gadget::matcher().bind(#x),                               \
-                notInSafeBufferOptOut(&Handler)),
-#define WARNING_OPTIONAL_GADGET(x)                                            \
-          allOf(x ## Gadget::matcher(&Handler).bind(#x),                      \
-                notInSafeBufferOptOut(&Handler)),
-#include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-            // Avoid a hanging comma.
-            unless(stmt())
-        )))
-    ),
-    &CB
-  );
-  // clang-format on
+public:
+  StmtMatcher(FixableGadgetList &FixableGadgets, DeclUseTracker &Tracker)
+      : FixableGadgets(FixableGadgets), Tracker(Tracker) {}
+
+  // Match all DeclRefExprs so that to find out
+  // whether there are any uncovered by gadgets.
+  bool matchDeclRefExprs(const Stmt *S, MatchResult &Result) {
+    const auto *DRE = dyn_cast<DeclRefExpr>(S);
+    if (!DRE || (!hasPointerType(*DRE) && !hasArrayType(*DRE)))
+      return false;
+    const Decl *D = DRE->getDecl();
+    if (!D || (!isa<VarDecl>(D) && !isa<BindingDecl>(D)))
+      return false;
+    Result.addNode("any_dre", DynTypedNode::create(*DRE));
+    return true;
+  }
 
-  if (EmitSuggestions) {
-    // clang-format off
-    M.addMatcher(
-        stmt(
-          forEachDescendantStmt(stmt(eachOf(
-#define FIXABLE_GADGET(x)                                                      \
-            x ## Gadget::matcher().bind(#x),
+  // Also match DeclStmts because we'll need them when fixing
+  // their underlying VarDecls that otherwise don't have
+  // any backreferences to DeclStmts.
+  bool matchDeclStmt(const Stmt *S, MatchResult &Result) {
+    const auto *DS = dyn_cast<DeclStmt>(S);
+    if (!DS)
+      return false;
+    Result.addNode("any_ds", DynTypedNode::create(*DS));
+    return true;
+  }
+
+  bool matches(const DynTypedNode &DynNode, ASTContext &Ctx,
+               const UnsafeBufferUsageHandler &Handler) override {
+    bool matchFound = false;
+    const Stmt *S = DynNode.get<Stmt>();
+    if (!S) {
+      return matchFound;
+    }
+
+    llvm::SmallVector<MatchResult> Results;
+#define FIXABLE_GADGET(name)                                                   \
+  if (name##Gadget::matches(S, Results)) {                                     \
+    for (const auto &R : Results) {                                            \
+      FixableGadgets.push_back(std::make_unique<name##Gadget>(R));             \
+      matchFound = true;                                                       \
+    }                                                                          \
+    Results = {};                                                              \
+  }
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
-            // In parallel, match all DeclRefExprs so that to find out
----------------
ilya-biryukov wrote:

Could we bring these comments back to the call sites?
They allow to understand the code a little easier. I think they are not as necessary on the declarations.

https://github.com/llvm/llvm-project/pull/125492