diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def index a52b00bf8d4ce..a112b6d105025 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def @@ -35,6 +35,7 @@ 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 diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 6a597c8851002..ff818140b2d89 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -139,6 +139,11 @@ AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher, 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 innerMatcher) { @@ -719,6 +724,46 @@ class Strategy { }; } // 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(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 getFixits(const Strategy &S) const override; + + virtual const Stmt *getBaseStmt() const override { return Node; } + + virtual DeclUseList getClaimedVarUseSites() const override { + return {dyn_cast(Node->getSubExpr())}; + } +}; + // Representing a fixable expression of the form `*(ptr + 123)` or `*(123 + // ptr)`: class DerefSimplePtrArithFixableGadget : public FixableGadget { @@ -1196,6 +1241,36 @@ fixUPCAddressofArraySubscriptWithSpan(const UnaryOperator *Node) { FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())}; } + +std::optional 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(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. diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp new file mode 100644 index 0000000000000..9bad7cb558352 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp @@ -0,0 +1,33 @@ +// 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 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])) { + } +}