[-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

149–152

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.

663

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

25

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

663

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(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 
 FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalue Context
 FIXABLE_GADGET(PointerDereference)
 FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context
+FIXABLE_GADGET(UPCPreIncrement)            // '++Ptr' in an Unspecified Pointer Context
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
 FIXABLE_GADGET(PointerCtxAccess)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "clang/Sema/Lookup.h"
 #include "llvm/ADT/SmallVector.h"
 #include <memory>
 #include <optional>
+#include <sstream>
 
 using namespace llvm;
 using namespace clang;
 using namespace ast_matchers;
 
 namespace clang::ast_matchers {
 // A `RecursiveASTVisitor` that traverses all descendants of a given node "n"
 // except for those belonging to a different callable of "n".
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(eachOf(
       implicitCastExpr(
         hasCastKind(CastKind::CK_LValueToRValue),
if (const auto *ArraySubst =
             dyn_cast<ArraySubscriptExpr>(Node->getSubExpr()))
       if (const auto *DRE =
               dyn_cast<DeclRefExpr>(ArraySubst->getBase()->IgnoreImpCasts())) {
         return {DRE};
       }
     return {};
   }
 };
+
+// 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()).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 {
+    if (auto DRE = dyn_cast<DeclRefExpr>(Node->getSubExpr())) {
+      return {DRE};
+    }
+    return {};
+  }
+};
 } // namespace
 
 namespace {
 // An auxiliary tracking facility for the fixit analysis. It helps connect
 // declarations to its and make sure we've covered all uses with our analysis
 // before we try to fix the declaration.
 class DeclUseTracker {
   using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
       case Strategy::Kind::Array:
       case Strategy::Kind::Vector:
         llvm_unreachable("unsupported strategies for FixableGadgets");
       }
     }
   return std::nullopt;
 }
 
+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()"
+
+  // 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])) {
+  }
+}