[-Wunsafe-buffer-usage] FixableGadget for handling stand alone pointe…

…rs under UPC

This patch introduces UPCStandalonePointerGadget, a FixableGadget that emits fixits to
handle cases where a pointer identified as unsafe is simply referenced. An example of
such a case is when the pointer is input as an argument to a method call, where we can
not change the type of the argument. For cases where the strategy for the unsafe pointer is
to use std::span, the idea is to extract the underlying pointer by invoking the "data()"
method on the span instance.

For example, the gadget emits a fixit for S3, where S1, S2 are handled by other gadgets:
  S1: int *ptr = new int[10];
  S2: int val1 = ptr[k]; // Unsafe operation on ptr
  S3: foo(ptr); // Some method that accepts raw pointer => FIXIT: foo(ptr.data());

Reviewed by: NoQ, ziqingluo-90, jkorous

Differential revision: https://reviews.llvm.org/D143676

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

@@ -34,6 +34,7 @@ FIXABLE_GADGET(ULCArraySubscript)          // `DRE[any]` in an Unspecified Lvalu
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
 FIXABLE_GADGET(PointerDereference)
 FIXABLE_GADGET(UPCAddressofArraySubscript) // '&DRE[any]' in an Unspecified Pointer Context
+FIXABLE_GADGET(UPCStandalonePointer)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -165,17 +165,22 @@ isInUnspecifiedPointerContext(internal::Matcher<Stmt> InnerMatcher) {
   // 1. an argument of a function call (except the callee has [[unsafe_...]]
   // attribute), or
   // 2. the operand of a cast operation; or
-  // ...
+  // 3. the operand of a comparator operation; or
   auto CallArgMatcher =
-      callExpr(hasAnyArgument(allOf(
-                   hasPointerType() /* array also decays to pointer type*/,
-                   InnerMatcher)),
-               unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
+      callExpr(forEachArgumentWithParam(InnerMatcher,
+                  hasPointerType() /* array also decays to pointer type*/),
+          unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
+
   auto CastOperandMatcher =
       explicitCastExpr(hasCastKind(CastKind::CK_PointerToIntegral),
                        castSubExpr(allOf(hasPointerType(), InnerMatcher)));
 
- return stmt(anyOf(CallArgMatcher, CastOperandMatcher));
+  auto CompOperandMatcher =
+      binaryOperator(hasAnyOperatorName("!=", "==", "<", "<=", ">", ">="),
+                     eachOf(hasLHS(allOf(hasPointerType(), InnerMatcher)),
+                            hasRHS(allOf(hasPointerType(), InnerMatcher))));
+
+  return stmt(anyOf(CallArgMatcher, CastOperandMatcher, CompOperandMatcher));
   // FIXME: any more cases? (UPC excludes the RHS of an assignment.  For now we
   // don't have to check that.)
 }
@@ -489,6 +494,41 @@ class ULCArraySubscriptGadget : public FixableGadget {
   }
 };
 
+// Fixable gadget to handle stand alone pointers of the form `UPC(DRE)` in the
+// unspecified pointer context (isInUnspecifiedPointerContext). The gadget emits
+// fixit of the form `UPC(DRE.data())`.
+class UPCStandalonePointerGadget : public FixableGadget {
+private:
+  static constexpr const char *const DeclRefExprTag = "StandalonePointer";
+  const DeclRefExpr *Node;
+
+public:
+  UPCStandalonePointerGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::UPCStandalonePointer),
+        Node(Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefExprTag)) {
+    assert(Node != nullptr && "Expecting a non-null matching result");
+  }
+
+  static bool classof(const Gadget *G) {
+    return G->getKind() == Kind::UPCStandalonePointer;
+  }
+
+  static Matcher matcher() {
+    auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType());
+    auto target = expr(
+        ignoringParenImpCasts(declRefExpr(allOf(ArrayOrPtr, to(varDecl()))).bind(DeclRefExprTag)));
+    return stmt(isInUnspecifiedPointerContext(target));
+  }
+
+  virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+
+  virtual const Stmt *getBaseStmt() const override { return Node; }
+
+  virtual DeclUseList getClaimedVarUseSites() const override {
+    return {Node};
+  }
+};
+
 class PointerDereferenceGadget : public FixableGadget {
   static constexpr const char *const BaseDeclRefExprTag = "BaseDRE";
   static constexpr const char *const OperatorTag = "op";
@@ -1070,6 +1110,31 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
   return std::nullopt;
 }
 
+// Generates fix-its replacing an expression of the form UPC(DRE) with
+// `DRE.data()`
+std::optional<FixItList> UPCStandalonePointerGadget::getFixits(const Strategy &S)
+      const {
+  const auto VD = cast<VarDecl>(Node->getDecl());
+  switch (S.lookup(VD)) {
+    case Strategy::Kind::Span: {
+      ASTContext &Ctx = VD->getASTContext();
+      SourceManager &SM = Ctx.getSourceManager();
+      // Inserts the .data() after the DRE
+      SourceLocation endOfOperand = getEndCharLoc(Node, SM, Ctx.getLangOpts());
+
+      return FixItList{{FixItHint::CreateInsertion(
+          endOfOperand.getLocWithOffset(1), ".data()")}};
+    }
+    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;
+}
+
 // Generates fix-its replacing an expression of the form `&DRE[e]` with
 // `&DRE.data()[e]`:
 static std::optional<FixItList>

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp

@@ -0,0 +1,110 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void foo(int* v) {
+}
+
+void m1(int* v1, int* v2, int) {
+
+}
+
+void condition_check_nullptr() {
+  auto 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}"
+
+  int tmp = p[5];
+  if(p != nullptr) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+}
+
+void condition_check() {
+  auto 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}"
+
+  auto q = new int[10];
+
+  int tmp = p[5];
+  if(p == q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  if(q != p){}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:12}:".data()"
+
+  if(p < q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  if(p <= q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  if(p > q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  if(p >= q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  if( p == q && p != nullptr) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:".data()"
+}
+
+void condition_check_two_usafe_buffers() {
+  auto 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}"
+
+  auto 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}"
+
+  int tmp = p[5];
+  tmp = q[5];
+
+  if(p == q) {}
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:".data()"
+}
+
+void unsafe_method_invocation_single_param() {
+  auto 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}"
+
+  int tmp = p[5];
+  foo(p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
+
+}
+
+void safe_method_invocation_single_param() {
+  auto p = new int[10];
+  foo(p);
+}
+
+void unsafe_method_invocation_double_param() {
+  auto 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}"
+
+  int tmp = p[5];
+  m1(p, p, 10);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:10-[[@LINE-2]]:10}:".data()"
+
+  auto q = new int[10];
+
+  m1(p, q, 4);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
+
+  m1(q, p, 9);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:10}:".data()"
+
+  m1(q, q, 8);
+}
+