[clang] [-Wunsafe-buffer-usage] Add fixits for array to pointer assignment (PR #81343)

via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 14 19:00:47 PST 2024


https://github.com/jkorous-apple updated https://github.com/llvm/llvm-project/pull/81343

>From 791130c5c5de31084c168db33531a5d856104506 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:30:20 -0800
Subject: [PATCH 1/5] [-Wunsafe-buffer-usage][NFC] Factor out .data() fixit to
 a function

---
 clang/lib/Analysis/UnsafeBufferUsage.cpp | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a74c113e29f1cf..cc49876779ece2 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1490,6 +1490,9 @@ PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
   return std::nullopt;
 }
 
+/// \returns fixit that adds .data() call after \DRE.
+static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE);
+
 std::optional<FixItList>
 PointerInitGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = PtrInitLHS;
@@ -1907,6 +1910,18 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
   return std::nullopt;
 }
 
+static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE) {
+  const SourceManager &SM = Ctx.getSourceManager();
+  // Inserts the .data() after the DRE
+  std::optional<SourceLocation> EndOfOperand =
+      getPastLoc(DRE, SM, Ctx.getLangOpts());
+
+  if (EndOfOperand)
+    return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+
+  return std::nullopt;
+}
+
 // Generates fix-its replacing an expression of the form UPC(DRE) with
 // `DRE.data()`
 std::optional<FixItList>
@@ -1915,14 +1930,7 @@ UPCStandalonePointerGadget::getFixits(const FixitStrategy &S) const {
   switch (S.lookup(VD)) {
   case FixitStrategy::Kind::Array:
   case FixitStrategy::Kind::Span: {
-    ASTContext &Ctx = VD->getASTContext();
-    SourceManager &SM = Ctx.getSourceManager();
-    // Inserts the .data() after the DRE
-    std::optional<SourceLocation> EndOfOperand =
-        getPastLoc(Node, SM, Ctx.getLangOpts());
-
-    if (EndOfOperand)
-      return FixItList{{FixItHint::CreateInsertion(*EndOfOperand, ".data()")}};
+    return createDataFixit(VD->getASTContext(), Node);
     // FIXME: Points inside a macro expansion.
     break;
   }

>From 1b12f7413288b01d1806b98fb90c2d44e53b7437 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:33:03 -0800
Subject: [PATCH 2/5] [-Wunsafe-buffer-usage][NFC] Rename PointerAssignment
 gadget to PtrToPtrAssignment

---
 .../Analysis/Analyses/UnsafeBufferUsageGadgets.def    |  2 +-
 clang/lib/Analysis/UnsafeBufferUsage.cpp              | 11 ++++++-----
 clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 07f805ebb11013..2babc1df93d515 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -45,7 +45,7 @@ FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Poin
 FIXABLE_GADGET(UPCStandalonePointer)
 FIXABLE_GADGET(UPCPreIncrement)            // '++Ptr' in an Unspecified Pointer Context
 FIXABLE_GADGET(UUCAddAssign)            // 'Ptr += n' in an Unspecified Untyped Context
-FIXABLE_GADGET(PointerAssignment)
+FIXABLE_GADGET(PtrToPtrAssignment)
 FIXABLE_GADGET(PointerInit)
 
 #undef FIXABLE_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index cc49876779ece2..927baef2fffa39 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -799,7 +799,8 @@ class PointerInitGadget : public FixableGadget {
 ///  \code
 ///  p = q;
 ///  \endcode
-class PointerAssignmentGadget : public FixableGadget {
+/// where both `p` and `q` are pointers.
+class PtrToPtrAssignmentGadget : public FixableGadget {
 private:
   static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
   static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
@@ -807,13 +808,13 @@ class PointerAssignmentGadget : public FixableGadget {
   const DeclRefExpr * PtrRHS;         // the RHS pointer expression in `PA`
 
 public:
-  PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
-      : FixableGadget(Kind::PointerAssignment),
+  PtrToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::PtrToPtrAssignment),
     PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
     PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 
   static bool classof(const Gadget *G) {
-    return G->getKind() == Kind::PointerAssignment;
+    return G->getKind() == Kind::PtrToPtrAssignment;
   }
 
   static Matcher matcher() {
@@ -1471,7 +1472,7 @@ bool clang::internal::anyConflict(const SmallVectorImpl<FixItHint> &FixIts,
 }
 
 std::optional<FixItList>
-PointerAssignmentGadget::getFixits(const FixitStrategy &S) const {
+PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
   const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
   switch (S.lookup(LeftVD)) {
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
index a5b578b98d4e5b..4cc1948f28a773 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -53,7 +53,7 @@ void unclaimed_use() {
 void implied_unclaimed_var(int *b) {  // expected-warning{{'b' is an unsafe pointer used for buffer access}}
   int *a = new int[3];  // expected-warning{{'a' is an unsafe pointer used for buffer access}}
   a[4] = 7;  // expected-note{{used in buffer access here}}
-  a = b;  // debug-note{{safe buffers debug: gadget 'PointerAssignment' refused to produce a fix}}
+  a = b;  // debug-note{{safe buffers debug: gadget 'PtrToPtrAssignment' refused to produce a fix}}
   b++;  // expected-note{{used in pointer arithmetic here}} \
         // debug-note{{safe buffers debug: failed to produce fixit for 'b' : has an unclaimed use}}
 }

>From c2411fa5665e0dab496a61faff6156bebc2198ba Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Thu, 8 Feb 2024 14:47:13 -0800
Subject: [PATCH 3/5] [-Wunsafe-buffer-usage] Implement fixits for const size
 array assigned to pointer

Introducing CArrayToPtrAssignment gadget and implementing fixits for some cases
of array being assigned to pointer.

Key observations:
- const size array can be assigned to std::span and bounds are propagated
- const size array can't be on LHS of assignment
This means array to pointer assignment has no strategy implications.

Fixits are implemented for cases where one of the variables in the assignment is
safe. For assignment of a safe array to unsafe pointer we know that the RHS will
never be transformed since it's safe and can immediately emit the optimal fixit.
Similarly for assignment of unsafe array to safe pointer.
(Obviously this is not and can't be future-proof in regards to what
variables we consider unsafe and that is fine.)

Fixits for assignment from unsafe array to unsafe pointer (from Array to Span
strategy) are not implemented in this patch as that needs to be properly designed
first - we might possibly implement optimal fixits for partially transformed
cases, put both variables in a single fixit group or do something else.
---
 .../Analyses/UnsafeBufferUsageGadgets.def     |  1 +
 clang/lib/Analysis/UnsafeBufferUsage.cpp      | 72 +++++++++++++++++++
 ...uffer-usage-fixits-array-assign-to-ptr.cpp | 43 +++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp

diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 2babc1df93d515..3273c642eed517 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -46,6 +46,7 @@ FIXABLE_GADGET(UPCStandalonePointer)
 FIXABLE_GADGET(UPCPreIncrement)            // '++Ptr' in an Unspecified Pointer Context
 FIXABLE_GADGET(UUCAddAssign)            // 'Ptr += n' in an Unspecified Untyped Context
 FIXABLE_GADGET(PtrToPtrAssignment)
+FIXABLE_GADGET(CArrayToPtrAssignment)
 FIXABLE_GADGET(PointerInit)
 
 #undef FIXABLE_GADGET
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 927baef2fffa39..66e85969f359a9 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -7,11 +7,14 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
 #include "clang/AST/StmtVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Lexer.h"
@@ -849,6 +852,59 @@ class PtrToPtrAssignmentGadget : public FixableGadget {
   }
 };
 
+/// An assignment expression of the form:
+///  \code
+///  ptr = array;
+///  \endcode
+/// where `p` is a pointer and `array` is a constant size array.
+class CArrayToPtrAssignmentGadget : public FixableGadget {
+private:
+  static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
+  static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
+  const DeclRefExpr * PtrLHS;         // the LHS pointer expression in `PA`
+  const DeclRefExpr * PtrRHS;         // the RHS pointer expression in `PA`
+
+public:
+  CArrayToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::CArrayToPtrAssignment),
+    PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+    PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::CArrayToPtrAssignment;
+  }
+
+  static Matcher matcher() {
+    auto PtrAssignExpr = binaryOperator(allOf(hasOperatorName("="),
+      hasRHS(ignoringParenImpCasts(declRefExpr(hasType(hasCanonicalType(constantArrayType())),
+                                               toSupportedVariable()).
+                                   bind(PointerAssignRHSTag))),
+                                   hasLHS(declRefExpr(hasPointerType(),
+                                                      toSupportedVariable()).
+                                          bind(PointerAssignLHSTag))));
+
+    return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
+  }
+
+  virtual std::optional<FixItList>
+  getFixits(const FixitStrategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override {
+    // FIXME: This should be the binary operator, assuming that this method
+    // makes sense at all on a FixableGadget.
+    return PtrLHS;
+  }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    return DeclUseList{PtrLHS, PtrRHS};
+  }
+
+  virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
+  getStrategyImplications() const override {
+    return {};
+  }
+};
+
 /// A call of a function or method that performs unchecked buffer operations
 /// over one of its pointer parameters.
 class UnsafeBufferUsageAttrGadget : public WarningGadget {
@@ -1494,6 +1550,22 @@ PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
 /// \returns fixit that adds .data() call after \DRE.
 static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE);
 
+std::optional<FixItList>
+CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
+  const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
+  const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
+  if (S.lookup(LeftVD) == FixitStrategy::Kind::Span) {
+    if (S.lookup(RightVD) == FixitStrategy::Kind::Wontfix) {
+      return FixItList{};
+    }
+  } else if (S.lookup(LeftVD) == FixitStrategy::Kind::Wontfix) {
+    if (S.lookup(RightVD) == FixitStrategy::Kind::Array) {
+      return createDataFixit(RightVD->getASTContext(), PtrRHS);
+    }
+  }
+  return std::nullopt;
+}
+
 std::optional<FixItList>
 PointerInitGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = PtrInitLHS;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
new file mode 100644
index 00000000000000..020a4520e2ccb4
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void safe_array_assigned_to_safe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+}
+
+void safe_array_assigned_to_unsafe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  int* ptr;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> ptr"
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr[idx] = 0;
+}
+
+void unsafe_array_assigned_to_safe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
+  ptr = buffer;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:15-[[@LINE-1]]:15}:".data()"
+  buffer[idx] = 0;
+}
+
+void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) {
+  int buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  int* ptr;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  ptr = buffer;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}
+  buffer[idx] = 0;
+  ptr[idx] = 0;
+}

>From 5591e8e711dfdc32186e492b22e2be3e169c74b3 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 14 Feb 2024 14:50:59 -0800
Subject: [PATCH 4/5] [-Wunsafe-buffer-usage][NFC] clang-format the PR

---
 clang/lib/Analysis/UnsafeBufferUsage.cpp | 33 +++++++++++++-----------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 66e85969f359a9..8e810876950c81 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -813,8 +813,8 @@ class PtrToPtrAssignmentGadget : public FixableGadget {
 public:
   PtrToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
       : FixableGadget(Kind::PtrToPtrAssignment),
-    PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
-    PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+        PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+        PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PtrToPtrAssignment;
@@ -861,27 +861,28 @@ class CArrayToPtrAssignmentGadget : public FixableGadget {
 private:
   static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
   static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
-  const DeclRefExpr * PtrLHS;         // the LHS pointer expression in `PA`
-  const DeclRefExpr * PtrRHS;         // the RHS pointer expression in `PA`
+  const DeclRefExpr *PtrLHS; // the LHS pointer expression in `PA`
+  const DeclRefExpr *PtrRHS; // the RHS pointer expression in `PA`
 
 public:
   CArrayToPtrAssignmentGadget(const MatchFinder::MatchResult &Result)
       : FixableGadget(Kind::CArrayToPtrAssignment),
-    PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
-    PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
+        PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+        PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {}
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::CArrayToPtrAssignment;
   }
 
   static Matcher matcher() {
-    auto PtrAssignExpr = binaryOperator(allOf(hasOperatorName("="),
-      hasRHS(ignoringParenImpCasts(declRefExpr(hasType(hasCanonicalType(constantArrayType())),
-                                               toSupportedVariable()).
-                                   bind(PointerAssignRHSTag))),
-                                   hasLHS(declRefExpr(hasPointerType(),
-                                                      toSupportedVariable()).
-                                          bind(PointerAssignLHSTag))));
+    auto PtrAssignExpr = binaryOperator(
+        allOf(hasOperatorName("="),
+              hasRHS(ignoringParenImpCasts(
+                  declRefExpr(hasType(hasCanonicalType(constantArrayType())),
+                              toSupportedVariable())
+                      .bind(PointerAssignRHSTag))),
+              hasLHS(declRefExpr(hasPointerType(), toSupportedVariable())
+                         .bind(PointerAssignLHSTag))));
 
     return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
   }
@@ -1548,7 +1549,8 @@ PtrToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
 }
 
 /// \returns fixit that adds .data() call after \DRE.
-static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE);
+static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx,
+                                                       const DeclRefExpr *DRE);
 
 std::optional<FixItList>
 CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
@@ -1983,7 +1985,8 @@ PointerDereferenceGadget::getFixits(const FixitStrategy &S) const {
   return std::nullopt;
 }
 
-static inline std::optional<FixItList> createDataFixit(const ASTContext& Ctx, const DeclRefExpr * DRE) {
+static inline std::optional<FixItList> createDataFixit(const ASTContext &Ctx,
+                                                       const DeclRefExpr *DRE) {
   const SourceManager &SM = Ctx.getSourceManager();
   // Inserts the .data() after the DRE
   std::optional<SourceLocation> EndOfOperand =

>From 3260557f895fcefb0e740666d69a4dd86f97ab52 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 14 Feb 2024 18:52:29 -0800
Subject: [PATCH 5/5] [-Wunsafe-buffer-usage][NFC] Comment on trickiness of
 array to ptr assignment

---
 clang/lib/Analysis/UnsafeBufferUsage.cpp         | 16 ++++++++++++++++
 ...e-buffer-usage-fixits-array-assign-to-ptr.cpp |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 8e810876950c81..769c6d9ebefaa5 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1556,6 +1556,22 @@ std::optional<FixItList>
 CArrayToPtrAssignmentGadget::getFixits(const FixitStrategy &S) const {
   const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
   const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
+  // TLDR: Implementing fixits for non-Wontfix strategy on both LHS and RHS is
+  // non-trivial.
+  //
+  // CArrayToPtrAssignmentGadget doesn't have strategy implications because
+  // constant size array propagates its bounds. Because of that LHS and RHS are
+  // addressed by two different fixits.
+  //
+  // At the same time FixitStrategy S doesn't reflect what group a fixit belongs
+  // to and can't be generally relied on in multi-variable Fixables!
+  //
+  // E. g. If an instance of this gadget is fixing variable on LHS then the
+  // variable on RHS is fixed by a different fixit and its strategy for LHS
+  // fixit is as if Wontfix.
+  //
+  // The only exception is Wontfix strategy for a given variable as that is
+  // valid for any fixit produced for the given input source code.
   if (S.lookup(LeftVD) == FixitStrategy::Kind::Span) {
     if (S.lookup(RightVD) == FixitStrategy::Kind::Wontfix) {
       return FixItList{};
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
index 020a4520e2ccb4..ff91e32bdc1ccf 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-array-assign-to-ptr.cpp
@@ -31,6 +31,8 @@ void unsafe_array_assigned_to_safe_ptr(unsigned idx) {
   buffer[idx] = 0;
 }
 
+// FIXME: Implement fixit/s for this case.
+// See comment in CArrayToPtrAssignmentGadget::getFixits to learn why this hasn't been implemented.
 void unsafe_array_assigned_to_unsafe_ptr(unsigned idx) {
   int buffer[10];
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}



More information about the cfe-commits mailing list