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

[Artem Dergachev via cfe-commits]

================
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     }
   }
 }
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+                                   UnsafeBufferUsageHandler &Handler,
+                                   bool EmitSuggestions) {
+#ifndef NDEBUG
+  Handler.clearDebugNotes();
+#endif
+
+  assert(D);
+
+  SmallVector<Stmt *> Stmts;
+
+  // 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;
+      }
+    }
+
+    Stmts.push_back(FD->getBody());
+
+    if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
+      for (const CXXCtorInitializer *CI : ID->inits()) {
+        Stmts.push_back(CI->getInit());
+      }
+    }
+  }
+
+  if (const auto *FD = dyn_cast<FieldDecl>(D)) {
----------------
haoNoQ wrote:

Ok here's how I think this should go:
```diff
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index bfbf84a03b2b..cf0194d29f8c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -170,6 +170,12 @@ public:
     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;
@@ -3343,16 +3349,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
         Stmts.push_back(CI->getInit());
       }
     }
-  } else if (const auto *FlD = dyn_cast<FieldDecl>(D)) {
-    // Visit in-class initializers for fields.
-    if (!FlD->hasInClassInitializer())
-      return;
-    Stmts.push_back(FlD->getInClassInitializer());
-    // In a FieldDecl there is no function body, there is only a single
-    // statement, and the suggestions machinery is not set up to handle that
-    // kind of structure yet and would give poor suggestions or likely even hit
-    // asserts.
-    EmitSuggestions = false;
   } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
     Stmts.push_back(D->getBody());
   }
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index e356c06ac8c4..b9d0b59ef1db 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2471,38 +2471,6 @@ public:
   CallableVisitor(llvm::function_ref<void(const Decl *)> Callback)
       : Callback(Callback) {}
 
-  bool VisitFieldDecl(FieldDecl *Node) {
-    DeclContext *DeclCtx;
-    if (auto *ID = dyn_cast<ObjCIvarDecl>(Node)) {
-      DeclCtx = cast<DeclContext>(ID->getContainingInterface());
-    } else {
-      RecordDecl *RD = Node->getParent();
-      if (auto *CCRD = dyn_cast<CXXRecordDecl>(RD);
-          CCRD && CCRD->isAggregate()) {
-        // Aggregates have implicitly generated constructors which are not
-        // traversed by our AST visitors at this time. The field initializers in
-        // an aggregate may never be used if the callers always explicitly
-        // initialize them, in which case we do not need to warn on unsafe
-        // buffer usage in the initializer.
-        //
-        // FIXME: We should go through the implicit aggregate initialization
-        // (either an implicit CXXConstructExpr for value-init or an implicit
-        // ListInitExpr for aggregate-init) to determine if a field's default
-        // initializer (CXXDefaultInitExpr) is actually used. If it is, then we
-        // should visit it, while retaining a reference to the caller for
-        // showing the path to the use of the default initializer in the
-        // warning.
-        return true;
-      }
-      DeclCtx = cast<DeclContext>(RD);
-    }
-    if (DeclCtx->isDependentContext())
-      return true; // Not to analyze dependent decl
-    if (Node->hasInClassInitializer())
-      Callback(Node);
-    return true;
-  }
-
   bool VisitFunctionDecl(FunctionDecl *Node) {
     if (cast<DeclContext>(Node)->isDependentContext())
       return true; // Not to analyze dependent decl
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
index b9e517a35de7..724d444638b5 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -128,7 +128,7 @@ struct HoldsUnsafeMembers {
 
     UnsafeMembers FromCtor;
     UnsafeMembers FromCtor2;
-    UnsafeMembers FromField{3};  // expected-warning{{function introduces unsafe buffer manipulation}}
+    UnsafeMembers FromField{3};  // expected-warning 2{{function introduces unsafe buffer manipulation}}
 };
 
 struct SubclassUnsafeMembers : public UnsafeMembers {
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
index 945a4e06d6af..22daac83b879 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
@@ -158,9 +158,19 @@ namespace test_flag {
 struct HoldsStdSpan {
   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}}
+  std::span<char> Span{Ptr, Size}; // no-warning (this code is unreachable)
 
   HoldsStdSpan(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 HoldsStdSpan2 {
+  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}}
+
+  HoldsStdSpan2(char* P, unsigned S)
+      : Ptr(P), Size(S)
+  {}
+};
```

This passes all tests except two. In one of those we aren't getting a warning but that's ok because the code is obviously dead. I added another test that demonstrates that we do get a warning when it actually matters.

In the other failure we're getting a duplicate warning because we're visiting the same field twice (from two different constructors). This isn't ideal and ultimately we'd need to either get rid of one of them or at least add notes to connect them back to the constructor, but that's not a deal breaker. We may also occasionally emit conflicting fixits because of that (attached to separate warnings) but that's probably still ok; eventually we'll get them deduplicated.

So I basically think that all of this is still better than analyzing field initializers separately; we'd never want to analyze them separately. Also more concise.

The visitor is a bit confusing; the `match(*Node)` invocation should have been put into the `Visit()` methods, not into `Traverse()` methods. Traverse methods are hard to override because their default implementation has tons of logic.

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