[-Wunsafe-buffer-usage][WIP] Add fixit for variables that have an assignment from another spanified pointer

[NoQ]

Ooo, that's a big one. Our first fixable that can connect multiple variables together. IIUC this patch doesn't yet teach the strategy machine how to pick related strategies for related variables, it just teaches the fixit machine what to do when the strategy is already picked.

clang/lib/Analysis/UnsafeBufferUsage.cpp

463–465

Shouldn't it also claim the RHS? Otherwise, how would the machine know that it's covered?

761

I suspect you want to check the strategy for LHS here as well. Just because the method was called, doesn't mean we plan to use span over it specifically.

1046

Oh, that's probably why it worked without the other claim!

[NoQ]

Wait, I completely misread, you *were* trying to mutate the strategy. I think it makes sense to split this work the way I thought it's already split: first get fixits when the strategy is already set (which this patch almost has), then try to figure out how to mutate and fixpoint-iterate-over the strategy.

[t-rasmud]

WIP: Fixable for ptr1 = ptr2 when both LHS and RHS have their fixit strategies set as std::span.

[t-rasmud]

• Fixes crash caused by FixableGadgetSets and WarningGadgetSets being unique pointers in the context of multiple declaration gadgets.

[NoQ]

Aha, great! Looks like our fancy ownership model couldn't handle shared ownership for multi-variable gadgets, so we're back to raw pointers. Works for me, it's not like they're buffers or something. I have minor nitpicks and then I think we can land!

clang/lib/Analysis/UnsafeBufferUsage.cpp

452–453

I think this overruns the 80-column limit.

463–464

I think these checks should be included in the matcher(). There could be a different Fixable that takes care of these non-variable cases, and if there indeed is one, we really don't want to it to overlap with this Fixable.

691

clang-format will probably recommend a space there.

1041

Now that they're in the same scope, the move doesn't really accomplish anything. You can probably drop the Tracker variable and rename TrackerRes to Tracker.

[t-rasmud]

Address feedback.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

467

Since the matcher only requires PtrLHS (or PtrRHS) to be Expr instead of specifically DeclRefExpr, we could have LDRE (or RDRE respectively) be null here.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

467

I am wondering if we should simply move the logic related to DeclRefExpr and IgnoreParenImpCasts to the matcher() method (expressed as AST matcher).

[t-rasmud]

Friendly Ping @jkorous @ziqingluo-90 @NoQ

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

425

Nit: assignment seems to have only one form :)

758

Isn't LeftDRE identical to PtrLHS and RightDRE identical to PtrRHS?

[NoQ]

We've noticed a subtle problem offline yesterday: sounds like if fixits for both sides are desired (like in the primary test case) but one of them can't be emitted (eg., due to unsupported variable use) then the other fixit alone will be incorrect: it'll act as if both p and q are spanified, so it'll emit no change for p = q. Also generally, we really don't want users to apply fixits for p and q independently; that won't lead to correct code. So it sounds like we cannot land this patch until we learn how to group related variables together.

[t-rasmud]

Abandoning patch in favor of D145739

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

 #endif
 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 FIXABLE_GADGET(ULCArraySubscript)
+FIXABLE_GADGET(PointerAssignment)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

public:
   virtual DeclUseList getClaimedVarUseSites() const override {
     if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
       return {DRE};
     }
     return {};
   }
 };
 
+/// A pointer assignment expression of one of the forms:
+///  \code
+///  p = q;
+///  \endcode
+class PointerAssignmentGadget : public FixableGadget {
+private:
+    static constexpr const char *const PointerAssignemntTag = "ptrAssign";
+    static constexpr const char *const PointerAssignLHSTag = "ptrLHS";
+    static constexpr const char *const PointerAssignRHSTag = "ptrRHS";
+    const BinaryOperator *PA;    // pointer arithmetic expression
+    const DeclRefExpr * PtrLHS;         // the LHS pointer expression in `PA`
+    const DeclRefExpr * PtrRHS;         // the RHS pointer expression in `PA`
+
+public:
+    PointerAssignmentGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::PointerAssignment),
+    PA(Result.Nodes.getNodeAs<BinaryOperator>(PointerAssignemntTag)),
+    PtrLHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignLHSTag)),
+    PtrRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerAssignRHSTag)) {
+    assert(PA != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::PointerAssignment;
+  }
+
+  static Matcher matcher() {
+    auto PtrAtRight = allOf(hasOperatorName("="),
+      hasRHS(ignoringParenImpCasts(declRefExpr(hasPointerType()).bind(PointerAssignRHSTag))));
+    auto PtrAtLeft = allOf(hasOperatorName("="),
+      hasLHS(declRefExpr(hasPointerType()).bind(PointerAssignLHSTag)));
+
+    return stmt(binaryOperator(allOf(PtrAtLeft, PtrAtRight)).bind(PointerAssignemntTag));
+  }
+
+  virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override { return PA; }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    return DeclUseList{PtrLHS, PtrRHS};
+  }
+};
 
 /// A call of a function or method that performs unchecked buffer operations
 /// over one of its pointer parameters.
 class UnsafeBufferUsageAttrGadget : public WarningGadget {
     constexpr static const char *const OpTag = "call_expr";
     const CallExpr *Op;
 
 public:
for (const auto *DRE : G->getClaimedVarUseSites()) {
       CB.Tracker.claimUse(DRE);
     }
   }
 
   return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)};
 }
 
 struct WarningGadgetSets {
-  std::map<const VarDecl *, std::set<std::unique_ptr<WarningGadget>>> byVar;
+  std::map<const VarDecl *, std::set<const WarningGadget *>> byVar;
   // These Gadgets are not related to pointer variables (e. g. temporaries).
-  llvm::SmallVector<std::unique_ptr<WarningGadget>, 16> noVar;
+  llvm::SmallVector<const WarningGadget *, 16> noVar;
 };
 
 static WarningGadgetSets
-groupWarningGadgetsByVar(WarningGadgetList &&AllUnsafeOperations) {
+groupWarningGadgetsByVar(const WarningGadgetList &AllUnsafeOperations) {
   WarningGadgetSets result;
   // If some gadgets cover more than one
   // variable, they'll appear more than once in the map.
   for (auto &G : AllUnsafeOperations) {
     DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites();
 
     bool AssociatedWithVarDecl = false;
     for (const DeclRefExpr *DRE : ClaimedVarUseSites) {
       if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-        result.byVar[VD].emplace(std::move(G));
+        result.byVar[VD].insert(G.get());
         AssociatedWithVarDecl = true;
       }
     }
 
     if (!AssociatedWithVarDecl) {
-      result.noVar.emplace_back(std::move(G));
+      result.noVar.push_back(G.get());
       continue;
     }
   }
   return result;
 }
 
 struct FixableGadgetSets {
-  std::map<const VarDecl *, std::set<std::unique_ptr<FixableGadget>>> byVar;
+  std::map<const VarDecl *, std::set<const FixableGadget *>> byVar;
 };
 
 static FixableGadgetSets
 groupFixablesByVar(FixableGadgetList &&AllFixableOperations) {
   FixableGadgetSets FixablesForUnsafeVars;
   for (auto &F : AllFixableOperations) {
     DeclUseList DREs = F->getClaimedVarUseSites();
 
     for (const DeclRefExpr *DRE : DREs) {
       if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-        FixablesForUnsafeVars.byVar[VD].emplace(std::move(F));
+        FixablesForUnsafeVars.byVar[VD].insert(F.get());
       }
     }
   }
   return FixablesForUnsafeVars;
 }
 
 std::optional<FixItList>
 ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
   if (const auto *DRE = dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts()))
     if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
       switch (S.lookup(VD)) {
       case Strategy::Kind::Span:
         return FixItList{};
       case Strategy::Kind::Wontfix:
       case Strategy::Kind::Iterator:
       case Strategy::Kind::Array:
       case Strategy::Kind::Vector:
         llvm_unreachable("unsupported strategies for FixableGadgets");
       }
     }
   return std::nullopt;
 }
 
+std::optional<FixItList>
+PointerAssignmentGadget::getFixits(const Strategy &S) const {
+  if (const auto *LeftDRE = dyn_cast<DeclRefExpr>(PA->getLHS()->IgnoreImpCasts()))
+    if (const VarDecl *LeftVD = dyn_cast<VarDecl>(LeftDRE->getDecl()))
+      if (const auto *RightDRE = dyn_cast<DeclRefExpr>(PA->getRHS()->IgnoreImpCasts()))
+        if (const VarDecl *RightVD = dyn_cast<VarDecl>(RightDRE->getDecl())) {
+            switch (S.lookup(LeftVD)) {
+              case Strategy::Kind::Span:
+                if (S.lookup(RightVD) == Strategy::Kind::Span)
+                    return FixItList{};
+                return std::nullopt;
+              case Strategy::Kind::Wontfix:
+              case Strategy::Kind::Iterator:
+              case Strategy::Kind::Array:
+              case Strategy::Kind::Vector:
+                  llvm_unreachable("unsupported strategies for FixableGadgets");
+            }
+         }
+  return std::nullopt;
+}
+
 // Return the text representation of the given `APInt Val`:
 static std::string getAPIntText(APInt Val) {
   SmallVector<char> Txt;
   Val.toString(Txt, 10, true);
   // APInt::toString does not add '\0' to the end of the string for us:
   Txt.push_back('\0');
   return Txt.data();
 }

 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
                                    UnsafeBufferUsageHandler &Handler) {
   assert(D && D->getBody());
 
   WarningGadgetSets UnsafeOps;
   FixableGadgetSets FixablesForUnsafeVars;
-  DeclUseTracker Tracker;
 
-  {
-    auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D);
-    UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
-    FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets));
-    Tracker = std::move(TrackerRes);
-  }
+  auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D);
 
   // Filter out non-local vars and vars with unclaimed DeclRefExpr-s.
   for (auto it = FixablesForUnsafeVars.byVar.cbegin();
        it != FixablesForUnsafeVars.byVar.cend();) {
     // FIXME: Support ParmVarDecl as well.
     if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) {
       it = FixablesForUnsafeVars.byVar.erase(it);
     } else {
       ++it;
     }
   }
 
   llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
   for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar)

clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-fixits.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+
+void local_assign_both_span() {
+  int tmp;
+  int* p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  tmp = p[4];
+    
+  int* q = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  tmp = q[4];
+    
+  q = p;
+}
+
+void local_assign_rhs_span() {
+  int tmp;
+  int* p = new int[10];
+  int* q = new int[10];
+  tmp = q[4];
+  p = q;
+}
+
+void local_assign_no_span() {
+  int tmp;
+  int* p = new int[10];
+  int* q = new int[10];
+  p = q;
+}
+
+void local_assign_lhs_span() {
+  int tmp;
+  int* p = new int[10];
+  tmp = p[4];
+  int* q = new int[10];
+    
+  p = q;
+}