From f58b025354ee2d3bcd7ab2399a11429ec940c1e0 Mon Sep 17 00:00:00 2001 From: Ziqing Luo Date: Wed, 4 Jan 2023 17:16:21 -0800 Subject: [PATCH] Revert "[-Wunsafe-buffer-usage] Add a new `forEachDescendant` matcher that skips callable declarations" This reverts commit b2ac5fd724c44cf662caed84bd8f84af574b981d. --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 101 +----------------------- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp | 70 +--------------- 2 files changed, 5 insertions(+), 166 deletions(-) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 29c8dbb..a699d27 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -8,111 +8,12 @@ #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::internal { -// 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 { -public: - typedef RecursiveASTVisitor 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 DynTypedMatcher *Matcher, ASTMatchFinder *Finder, - BoundNodesTreeBuilder *Builder, - 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()) { - TraverseStmt(const_cast(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(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(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 bool match(const T &Node) { - BoundNodesTreeBuilder RecursiveBuilder(*Builder); - - if (Matcher->matches(DynTypedNode::create(Node), Finder, - &RecursiveBuilder)) { - ResultBindings.addMatch(RecursiveBuilder); - Matches = true; - if (Bind != ASTMatchFinder::BK_All) - return false; // Abort as soon as a match is found. - } - return true; - } - - const DynTypedMatcher *const Matcher; - ASTMatchFinder *const Finder; - BoundNodesTreeBuilder *const Builder; - BoundNodesTreeBuilder ResultBindings; - const ASTMatchFinder::BindKind Bind; - bool Matches; -}; - -AST_MATCHER_P(Stmt, forEveryDescendant, Matcher, innerMatcher) { - MatchDescendantVisitor Visitor(new DynTypedMatcher(innerMatcher), Finder, - Builder, ASTMatchFinder::BK_All); - return Visitor.findMatch(DynTypedNode::create(Node)); -} -} // namespace clang::ast_matchers::internal - namespace { // Because the analysis revolves around variables and their types, we'll need to // track uses of variables (aka DeclRefExprs). @@ -497,7 +398,7 @@ static std::pair findGadgets(const Decl *D) { // clang-format off M.addMatcher( - stmt(forEveryDescendant( + stmt(forEachDescendant( stmt(anyOf( // Add Gadget::matcher() for every gadget in the registry. #define GADGET(x) \ diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp index 476ec73..e828414 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fblocks -include %s -verify %s +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -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]; // FIXME: file scope unsafe buffer access is not warned +int gvar = gp[1]; // TODO: this is not warned void testLambdaCaptureAndGlobal(int * p) { int a[10]; auto Lam = [p, a]() { - return p[1] // expected-warning{{unchecked operation on raw buffer in expression}} + return p[1] // expected-warning2{{unchecked operation on raw buffer in expression}} + a[1] + garray[1] - + gp[1]; // expected-warning{{unchecked operation on raw buffer in expression}} + + gp[1]; // expected-warning2{{unchecked operation on raw buffer in expression}} }; } @@ -213,66 +213,4 @@ 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 -- 2.7.4