[-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment

[ziqingluo-90]

Summary

For a pointer type expression e of the form ++DRE, if e is under an Unspecified Pointer Context (UPC) and DRE is suppose to be transformed to have span type,
we generate fix-its that transform e to (DRE = DRE.subspan(1)).data().

For reference, the UPC is currently defined as

• 1. an argument of a function call (except the callee has [[unsafe_buffer_usage]] attribute), or
• 2. the operand of a cast-to-(Integer or Boolean) operation; or
• 3. the operand of a pointer subtraction operation; or
• 4. the operand of a pointer comparison operation;

We may extend the definition of UPC by adding more cases later.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

142–145

Oh interesting, so hasOperatorName wasn't sufficient, because operators ++ and ++ have the same "name"?

Yeah, sounds like a universally useful matcher, we should commit it to ASTMatchers.h for everybody to use.

657

You're not checking whether the operand is a variable. This isn't incorrect, given that the fixable will be thrown away if it doesn't claim any variables. However, for performance it makes sense to never match things that don't act on variables, so that to never construct the fixable object in the first place. The check for the variable being an actual VarDecl is also useful and can be made here instead of the getFixit method.

I think this is something we should do consistently: if it's not the right code, stop doing things with it as early as possible. Premature optimizations are evil, but in these cases it's not much of an optimization really, it's just easier to write code this way from the start.

It also makes the code easier to read: you can easily see which patterns the gadget covers, by just reading the matcher() method.

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp

26

It's probably a good idea to explicitly say FIXME: if we think these cases should eventually be fixed.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

657

aha, I agree with you and I like your last sentence the most! Treating the matcher() methods as documents, i.e., we are guaranteed that a matched node conforms to the "description" of the matcher(). So that we can get rid of defensive checks. For example, the
if (auto DRE = dyn_cast<DeclRefExpr>(Node->getSubExpr())) statement at line 671 could be a defensive check.

[ziqingluo-90]

Rebased and addressed comments.

[NoQ]

LGTM!

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

19 ↗

(On Diff #503928)

These changes look unrelated, probably accidentally included from another patch 🤔

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

37 ↗

(On Diff #503928)

These also seem unrelated.

[ziqingluo-90]

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp

19 ↗

(On Diff #503928)

oh yes, need a rebase.

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

 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalue Context
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
 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
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

AST_MATCHER_P(Stmt, notInSafeBufferOptOut, const UnsafeBufferUsageHandler *,
               Handler) {
   return !Handler->isSafeBufferOptOut(Node.getBeginLoc());
 }
 
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
   return innerMatcher.matches(*Node.getSubExpr(), Finder, Builder);
 }
 
+// Matches a `UnaryOperator` whose operator is pre-increment:
+AST_MATCHER(UnaryOperator, isPreInc) {
+  return Node.getOpcode() == UnaryOperator::Opcode::UO_PreInc;
+}  
+
 // Returns a matcher that matches any expression 'e' such that `innerMatcher`
 // matches 'e' and 'e' is in an Unspecified Lvalue Context.
 static auto isInUnspecifiedLvalueContext(internal::Matcher<Expr> innerMatcher) {
   // clang-format off
   return
     expr(anyOf(
       implicitCastExpr(
         hasCastKind(CastKind::CK_LValueToRValue),
public:
   DeclUseTracker &operator=(DeclUseTracker &&) = default;
 
   // Start tracking a freshly discovered DRE.
   void discoverUse(const DeclRefExpr *DRE) { Uses->insert(DRE); }
 
   // Stop tracking the DRE as it's been fully figured out.
   void claimUse(const DeclRefExpr *DRE) {
     assert(Uses->count(DRE) &&
            "DRE not found or claimed by multiple matchers!");
     Uses->erase(DRE);
   }
 
   // A variable is unclaimed if at least one use is unclaimed.
   bool hasUnclaimedUses(const VarDecl *VD) const {
     // FIXME: Can this be less linear? Maybe maintain a map from VDs to DREs?
     return any_of(*Uses, [VD](const DeclRefExpr *DRE) {
       return DRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl();
Kind lookup(const VarDecl *VD) const {
     if (I == Map.end())
       return Kind::Wontfix;
 
     return I->second;
   }
 };
 } // namespace
 
+
+// Representing a pointer type expression of the form `++Ptr` in an Unspecified
+// Pointer Context (UPC):
+class UPCPreIncrementGadget : public FixableGadget {
+private:
+  static constexpr const char *const UPCPreIncrementTag =
+    "PointerPreIncrementUnderUPC";
+  const UnaryOperator *Node; // the `++Ptr` node
+
+public:
+  UPCPreIncrementGadget(const MatchFinder::MatchResult &Result)
+    : FixableGadget(Kind::UPCPreIncrement),
+      Node(Result.Nodes.getNodeAs<UnaryOperator>(UPCPreIncrementTag)) {
+    assert(Node != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::UPCPreIncrement;
+  }
+
+  static Matcher matcher() {
+    // Note here we match `++Ptr` for any expression `Ptr` of pointer type.
+    // Although currently we can only provide fix-its when `Ptr` is a DRE, we
+    // can have the matcher be general, so long as `getClaimedVarUseSites` does
+    // things right.
+    return stmt(isInUnspecifiedPointerContext(expr(ignoringImpCasts(
+                    unaryOperator(isPreInc(),
+                      hasUnaryOperand(declRefExpr())
+                      ).bind(UPCPreIncrementTag)))));
+  }
+
+  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->getSubExpr())};
+  }
+};
+
 // Representing a fixable expression of the form `*(ptr + 123)` or `*(123 +
 // ptr)`:
 class DerefSimplePtrArithFixableGadget : public FixableGadget {
   static constexpr const char *const BaseDeclRefExprTag = "BaseDRE";
   static constexpr const char *const DerefOpTag = "DerefOp";
   static constexpr const char *const AddOpTag = "AddOp";
   static constexpr const char *const OffsetTag = "Offset";
 
fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) {
   } else {
     SS << "&" << getExprText(DRE, SM, LangOpts).str() << ".data()"
        << "[" << getExprText(Idx, SM, LangOpts).str() << "]";
   }
   return FixItList{
       FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())};
 }
 
+
+std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) const {
+  DeclUseList DREs = getClaimedVarUseSites();
+
+  if (DREs.size() != 1)
+    return std::nullopt; // In cases of `++Ptr` 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 *PreIncNode = getBaseStmt();
+      StringRef varName = VD->getName();
+      const ASTContext &Ctx = VD->getASTContext();
+
+      // To transform UPC(++p) to UPC((p = p.subspan(1)).data()):
+      SS << "(" << varName.data() << " = " << varName.data()
+         << ".subspan(1)).data()";
+      Fixes.push_back(FixItHint::CreateReplacement(
+          SourceRange(PreIncNode->getBeginLoc(),
+                      getEndCharLoc(PreIncNode, Ctx.getSourceManager(),
+                                    Ctx.getLangOpts())),
+          SS.str()));
+      return Fixes;
+    }
+  }
+  return std::nullopt; // Not in the cases that we can handle for now, give up.
+}
+
+
 // For a non-null initializer `Init` of `T *` type, this function returns
 // `FixItHint`s producing a list initializer `{Init,  S}` as a part of a fix-it
 // to output stream.
 // In many cases, this function cannot figure out the actual extent `S`.  It
 // then will use a place holder to replace `S` to ask users to fill `S` in.  The
 // initializer shall be used to initialize a variable of type `std::span<T>`.
 //
 // FIXME: Support multi-level pointers

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void foo(int * , int *);
+
+void simple() {
+  int * p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
+  bool b = ++p;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:15}:"(p = p.subspan(1)).data()"
+  unsigned long n = (unsigned long) ++p;
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:37-[[@LINE-1]]:40}:"(p = p.subspan(1)).data()"
+  if (++p) {
+    // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+  }
+  if (++p - ++p) {
+    // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+    // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:16}:"(p = p.subspan(1)).data()"
+  }
+  foo(++p, p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:".data()"
+
+  // FIXME: Don't know how to fix the following cases:
+  // CHECK-NOT: fix-it:"{{.*}}":{
+  int * g = new int[10];
+  int * a[10];
+  a[0] = ++g;
+  foo(g++, g);
+  if (++(a[0])) {
+  }
+}