Revert "Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (#91991)"

This reverts commit a518ed2 because it causes regression. See #91991 (comment) for detail.

Author: ZequanWu

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -171,12 +171,6 @@ class MatchDescendantVisitor
     return VisitorBase::TraverseCXXTypeidExpr(Node);
   }
 
-  bool TraverseCXXDefaultInitExpr(CXXDefaultInitExpr *Node) {
-    if (!TraverseStmt(Node->getExpr()))
-      return false;
-    return VisitorBase::TraverseCXXDefaultInitExpr(Node);
-  }
-
   bool TraverseStmt(Stmt *Node, DataRecursionQueue *Queue = nullptr) {
     if (!Node)
       return true;
@@ -1998,18 +1992,14 @@ 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) {
+static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
+findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
+            bool EmitSuggestions) {
 
   struct GadgetFinderCallback : MatchFinder::MatchCallback {
-    GadgetFinderCallback(FixableGadgetList &FixableGadgets,
-                         WarningGadgetList &WarningGadgets,
-                         DeclUseTracker &Tracker)
-        : FixableGadgets(FixableGadgets), WarningGadgets(WarningGadgets),
-          Tracker(Tracker) {}
+    FixableGadgetList FixableGadgets;
+    WarningGadgetList WarningGadgets;
+    DeclUseTracker Tracker;
 
     void run(const MatchFinder::MatchResult &Result) override {
       // In debug mode, assert that we've found exactly one gadget.
@@ -2050,14 +2040,10 @@ static void findGadgets(const Stmt *S, ASTContext &Ctx,
       assert(numFound >= 1 && "Gadgets not found in match result!");
       assert(numFound <= 1 && "Conflicting bind tags in gadgets!");
     }
-
-    FixableGadgetList &FixableGadgets;
-    WarningGadgetList &WarningGadgets;
-    DeclUseTracker &Tracker;
   };
 
   MatchFinder M;
-  GadgetFinderCallback CB{FixableGadgets, WarningGadgets, Tracker};
+  GadgetFinderCallback CB;
 
   // clang-format off
   M.addMatcher(
@@ -2102,7 +2088,9 @@ static void findGadgets(const Stmt *S, ASTContext &Ctx,
     // clang-format on
   }
 
-  M.match(*S, Ctx);
+  M.match(*D->getBody(), D->getASTContext());
+  return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
+          std::move(CB.Tracker)};
 }
 
 // Compares AST nodes by source locations.
@@ -3646,9 +3634,39 @@ class VariableGroupsManagerImpl : public VariableGroupsManager {
   }
 };
 
-void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
-                  WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
-                  UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
+void clang::checkUnsafeBufferUsage(const Decl *D,
+                                   UnsafeBufferUsageHandler &Handler,
+                                   bool EmitSuggestions) {
+#ifndef NDEBUG
+  Handler.clearDebugNotes();
+#endif
+
+  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.
+  // FIXME: do we want to do the same thing for `BlockDecl`s?
+  if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
+    if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+      return;
+  }
+
+  // Do not emit fixit suggestions for functions declared in an
+  // extern "C" block.
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    for (FunctionDecl *FReDecl : FD->redecls()) {
+      if (FReDecl->isExternC()) {
+        EmitSuggestions = false;
+        break;
+      }
+    }
+  }
+
+  WarningGadgetSets UnsafeOps;
+  FixableGadgetSets FixablesForAllVars;
+
+  auto [FixableGadgets, WarningGadgets, Tracker] =
+      findGadgets(D, Handler, EmitSuggestions);
+
   if (!EmitSuggestions) {
     // Our job is very easy without suggestions. Just warn about
     // every problematic operation and consider it done. No need to deal
@@ -3692,10 +3710,8 @@ void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
   if (WarningGadgets.empty())
     return;
 
-  WarningGadgetSets UnsafeOps =
-      groupWarningGadgetsByVar(std::move(WarningGadgets));
-  FixableGadgetSets FixablesForAllVars =
-      groupFixablesByVar(std::move(FixableGadgets));
+  UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
+  FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
 
   std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
 
@@ -3916,56 +3932,3 @@ void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
     }
   }
 }
-
-void clang::checkUnsafeBufferUsage(const Decl *D,
-                                   UnsafeBufferUsageHandler &Handler,
-                                   bool EmitSuggestions) {
-#ifndef NDEBUG
-  Handler.clearDebugNotes();
-#endif
-
-  assert(D);
-
-  SmallVector<Stmt *> Stmts;
-
-  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
-    // We do not want to visit a Lambda expression defined inside a method
-    // independently. Instead, it should be visited along with the outer method.
-    // FIXME: do we want to do the same thing for `BlockDecl`s?
-    if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
-      if (MD->getParent()->isLambda() && MD->getParent()->isLocalClass())
-        return;
-    }
-
-    for (FunctionDecl *FReDecl : FD->redecls()) {
-      if (FReDecl->isExternC()) {
-        // Do not emit fixit suggestions for functions declared in an
-        // extern "C" block.
-        EmitSuggestions = false;
-        break;
-      }
-    }
-
-    Stmts.push_back(FD->getBody());
-
-    if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
-      for (const CXXCtorInitializer *CI : ID->inits()) {
-        Stmts.push_back(CI->getInit());
-      }
-    }
-  } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
-    Stmts.push_back(D->getBody());
-  }
-
-  assert(!Stmts.empty());
-
-  FixableGadgetList FixableGadgets;
-  WarningGadgetList WarningGadgets;
-  DeclUseTracker Tracker;
-  for (Stmt *S : Stmts) {
-    findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets,
-                WarningGadgets, Tracker);
-  }
-  applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
-               std::move(Tracker), Handler, EmitSuggestions);
-}

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

@@ -111,37 +111,6 @@ int testFoldExpression(Vs&&... v) {
     return (... + v);  // expected-warning{{function introduces unsafe buffer manipulation}}
 }
 
-struct HoldsUnsafeMembers {
-    HoldsUnsafeMembers()
-        : FromCtor(3),  // expected-warning{{function introduces unsafe buffer manipulation}}
-          FromCtor2{3}  // expected-warning{{function introduces unsafe buffer manipulation}}
-    {}
-
-    [[clang::unsafe_buffer_usage]]
-    HoldsUnsafeMembers(int i)
-        : FromCtor(i),  // expected-warning{{function introduces unsafe buffer manipulation}}
-          FromCtor2{i}  // expected-warning{{function introduces unsafe buffer manipulation}}
-    {}
-
-    HoldsUnsafeMembers(float f)
-        : HoldsUnsafeMembers(0) {}  // expected-warning{{function introduces unsafe buffer manipulation}}
-
-    UnsafeMembers FromCtor;
-    UnsafeMembers FromCtor2;
-    UnsafeMembers FromField{3};  // expected-warning 2{{function introduces unsafe buffer manipulation}}
-};
-
-struct SubclassUnsafeMembers : public UnsafeMembers {
-    SubclassUnsafeMembers()
-        : UnsafeMembers(3)  // expected-warning{{function introduces unsafe buffer manipulation}}
-    {}
-
-    [[clang::unsafe_buffer_usage]]
-    SubclassUnsafeMembers(int i)
-        : UnsafeMembers(i)  // expected-warning{{function introduces unsafe buffer manipulation}}
-    {}
-};
-
 // https://github.com/llvm/llvm-project/issues/80482
 void testClassMembers() {
     UnsafeMembers(3);  // expected-warning{{function introduces unsafe buffer manipulation}}
@@ -153,95 +122,4 @@ void testClassMembers() {
     UnsafeMembers()();  // expected-warning{{function introduces unsafe buffer manipulation}}
 
     testFoldExpression(UnsafeMembers(), UnsafeMembers());
-
-    HoldsUnsafeMembers();
-    HoldsUnsafeMembers(3);  // expected-warning{{function introduces unsafe buffer manipulation}}
-
-    SubclassUnsafeMembers();
-    SubclassUnsafeMembers(3);  // expected-warning{{function introduces unsafe buffer manipulation}}
-}
-
-// Not an aggregate, so its constructor is not implicit code and will be
-// visited/checked for warnings.
-struct NotCalledHoldsUnsafeMembers {
-    NotCalledHoldsUnsafeMembers()
-        : FromCtor(3),  // expected-warning{{function introduces unsafe buffer manipulation}}
-          FromCtor2{3}  // expected-warning{{function introduces unsafe buffer manipulation}}
-    {}
-
-    UnsafeMembers FromCtor;
-    UnsafeMembers FromCtor2;
-    UnsafeMembers FromField{3};  // expected-warning{{function introduces unsafe buffer manipulation}}
-};
-
-// An aggregate, so its constructor is implicit code. Since it's not called, it
-// is never generated.
-struct AggregateUnused {
-    UnsafeMembers f1;
-    // While this field would trigger the warning during initialization, since
-    // it's unused, there's no code generated that does the initialization, so
-    // no warning.
-    UnsafeMembers f2{3};
-};
-
-struct AggregateExplicitlyInitializedSafe {
-    UnsafeMembers f1;
-    // The warning is not fired as the field is always explicltly initialized
-    // elsewhere. This initializer is never used.
-    UnsafeMembers f2{3};
-};
-
-void testAggregateExplicitlyInitializedSafe() {
-    AggregateExplicitlyInitializedSafe A{
-        .f2 = UnsafeMembers(),  // A safe constructor.
-    };
 }
-
-struct AggregateExplicitlyInitializedUnsafe {
-    UnsafeMembers f1;
-    // The warning is not fired as the field is always explicltly initialized
-    // elsewhere. This initializer is never used.
-    UnsafeMembers f2{3};
-};
-
-void testAggregateExplicitlyInitializedUnsafe() {
-    AggregateExplicitlyInitializedUnsafe A{
-        .f2 = UnsafeMembers(3),  // expected-warning{{function introduces unsafe buffer manipulation}}
-    };
-}
-
-struct AggregateViaAggregateInit {
-    UnsafeMembers f1;
-    // FIXME: A construction of this class does initialize the field through
-    // this initializer, so it should warn. Ideally it should also point to
-    // where the site of the construction is in testAggregateViaAggregateInit().
-    UnsafeMembers f2{3};
-};
-
-void testAggregateViaAggregateInit() {
-    AggregateViaAggregateInit A{};
-};
-
-struct AggregateViaValueInit {
-    UnsafeMembers f1;
-    // FIXME: A construction of this class does initialize the field through
-    // this initializer, so it should warn. Ideally it should also point to
-    // where the site of the construction is in testAggregateViaValueInit().
-    UnsafeMembers f2{3};
-};
-
-void testAggregateViaValueInit() {
-    auto A = AggregateViaValueInit();
-};
-
-struct AggregateViaDefaultInit {
-    UnsafeMembers f1;
-    // FIXME: A construction of this class does initialize the field through
-    // this initializer, so it should warn. Ideally it should also point to
-    // where the site of the construction is in testAggregateViaValueInit().
-    UnsafeMembers f2{3};
-};
-
-void testAggregateViaDefaultInit() {
-    AggregateViaDefaultInit A;
-};

clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp

@@ -174,23 +174,3 @@ namespace test_flag {
 
   }
 } //namespace test_flag
-
-struct HoldsStdSpanAndInitializedInCtor {
-  char* Ptr;
-  unsigned Size;
-  std::span<char> Span{Ptr, Size};  // no-warning (this code is unreachable)
-
-  HoldsStdSpanAndInitializedInCtor(char* P, unsigned S)
-      : Span(P, S)  // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
-  {}
-};
-
-struct HoldsStdSpanAndNotInitializedInCtor {
-  char* Ptr;
-  unsigned Size;
-  std::span<char> Span{Ptr, Size}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
-
-  HoldsStdSpanAndNotInitializedInCtor(char* P, unsigned S)
-      : Ptr(P), Size(S)
-  {}
-};