[-Wunsafe-buffer-usage] Add FixableGadget for AddAssign in UnspecifiedUntypedContext

[t-rasmud]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Rashmi Mudduluru (t-rasmud)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/71862.diff

3 Files Affected:

• (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+1)
• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+92)
• (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp (+40)

diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index ff687a0d178bdea..757ee452ced7488 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ FIXABLE_GADGET(PointerDereference)
 FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context
 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(PointerInit)
 
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index e332a3609290aac..54923620274c0d5 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1028,6 +1028,52 @@ class UPCPreIncrementGadget : public FixableGadget {
   }
 };
 
+// Representing a pointer type expression of the form `Ptr += n` in an
+// Unspecified Untyped Context (UUC):
+class UUCAddAssignGadget : public FixableGadget {
+private:
+  static constexpr const char *const UUCAddAssignTag =
+    "PointerAddAssignUnderUUC";
+  static constexpr const char *const IntOffsetTag = "IntOffset";
+  static constexpr const char *const OffsetTag = "Offset";
+  
+  const BinaryOperator *Node; // the `Ptr += n` node
+  const IntegerLiteral *IntOffset = nullptr;
+  const DeclRefExpr *Offset = nullptr;
+
+public:
+  UUCAddAssignGadget(const MatchFinder::MatchResult &Result)
+    : FixableGadget(Kind::UUCAddAssign),
+      Node(Result.Nodes.getNodeAs<BinaryOperator>(UUCAddAssignTag)),
+      IntOffset(Result.Nodes.getNodeAs<IntegerLiteral>(IntOffsetTag)),
+      Offset(Result.Nodes.getNodeAs<DeclRefExpr>(OffsetTag)) {
+    assert(Node != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::UUCAddAssign;
+  }
+
+  static Matcher matcher() {
+    return stmt(isInUnspecifiedUntypedContext(expr(ignoringImpCasts(
+                    binaryOperator(hasOperatorName("+="),
+                      hasLHS(declRefExpr(
+                                                    toSupportedVariable())),
+                      hasRHS(expr(anyOf(
+                                        ignoringImpCasts(declRefExpr().bind(OffsetTag)),
+                                        integerLiteral().bind(IntOffsetTag))))
+                      ).bind(UUCAddAssignTag)))));
+  }
+
+  virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override { return Node; }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    return {dyn_cast<DeclRefExpr>(Node->getLHS())};
+  }
+};
+
 // Representing a fixable expression of the form `*(ptr + 123)` or `*(123 +
 // ptr)`:
 class DerefSimplePtrArithFixableGadget : public FixableGadget {
@@ -1766,6 +1812,52 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
       FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())};
 }
 
+std::optional<FixItList> UUCAddAssignGadget::getFixits(const Strategy &S) const {
+  DeclUseList DREs = getClaimedVarUseSites();
+
+  if (DREs.size() != 1)
+    return std::nullopt; // In cases of `Ptr += n` where `Ptr` is not a DRE, we
+                         // give up
+  if (const VarDecl *VD = dyn_cast<VarDecl>(DREs.front()->getDecl())) {
+    if (S.lookup(VD) == Strategy::Kind::Span) {
+      FixItList Fixes;
+      std::stringstream SS;
+      const Stmt *AddAssignNode = getBaseStmt();
+      StringRef varName = VD->getName();
+      const ASTContext &Ctx = VD->getASTContext();
+      
+      std::string SubSpanOffset;
+      if (IntOffset) {
+        auto ConstVal = IntOffset->getIntegerConstantExpr(Ctx);
+        if (ConstVal->isNegative())
+          return std::nullopt;
+        
+        SmallString<256> OffsetStr;
+        ConstVal->toString(OffsetStr);
+        SubSpanOffset = OffsetStr.c_str();
+        // To transform UUC(p += IntegerLiteral) to UUC(p = p.subspan(IntegerLiteral)):
+        SubSpanOffset = OffsetStr.c_str();
+      }
+      else {
+        SubSpanOffset = Offset->getDecl()->getName().str();
+      }
+      
+      // To transform UUC(p += n) to UUC(p = p.subspan(..)):
+      SS << varName.data() << " = " << varName.data()
+         << ".subspan(" << SubSpanOffset << ")";
+      
+      std::optional<SourceLocation> AddAssignLocation =
+          getEndCharLoc(AddAssignNode, Ctx.getSourceManager(), Ctx.getLangOpts());
+      if (!AddAssignLocation)
+        return std::nullopt;
+
+      Fixes.push_back(FixItHint::CreateReplacement(
+          SourceRange(AddAssignNode->getBeginLoc(), *AddAssignLocation), SS.str()));
+      return Fixes;
+    }
+  }
+  return std::nullopt; // Not in the cases that we can handle for now, give up.
+}
 
 std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) const {
   DeclUseList DREs = getClaimedVarUseSites();
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
new file mode 100644
index 000000000000000..30c587b2110d19b
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-add-assign.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+void foo(int * , int *);
+
+void add_assign_test(int n, int *a) {
+  int *p = new int[10];
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  p += 2;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:9}:"p = p.subspan(2)"
+  
+  int *r = p;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:13-[[@LINE-3]]:13}:", <# placeholder #>}"
+  while (*r != 0) {
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"[0]"
+    r += 2;
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:11}:"r = r.subspan(2)"
+  }
+  
+  if (*p == 0) {
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]"
+    p += n;
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:11}:"p = p.subspan(n)"
+  }
+  
+  if (*p == 1)
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]"
+    p += 3;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:11}:"p = p.subspan(3)"
+  
+  a += -9;
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:5-[[@LINE-1]]:11}:"p = p.subspan(-9)"
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[haoNoQ]

Very nice!!

[haoNoQ]

Yay I like this!

[haoNoQ]

I'm starting to like how these checks live in getFixits(), as opposed to the matcher. It's not like we'll ever be able to fix the signed case, but this way they'll show up in debug statistics as "gadget refused to produce a fix", which is better than the generic "it never matched". We could even make a dedicated debug message for this failure specifically!

[haoNoQ]

Aha, yes, this is exactly what I had in mind with the fix-around-it thing!

I found one more nitpick, but I think everything else looks great!

[haoNoQ]

Perfect, thanks!!