[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG762af11d4c5d: [-Wunsafe-buffer-usage] Add a Fixable for
pointer pre-increment (authored by ziqingluo-90).
Changed prior to commit:
https://reviews.llvm.org/D144304?vs=503928=512979#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144304/new/
https://reviews.llvm.org/D144304
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
===
--- /dev/null
+++ 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])) {
+ }
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -139,6 +139,11 @@
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 @@
};
} // 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 )
+: 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 ) 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 @@
FixItHint::CreateReplacement(Node->getSourceRange(), SS.str())};
}
+
+std::optional UPCPreIncrementGadget::getFixits(const Strategy ) 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())) {
+
[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
ziqingluo-90 marked an inline comment as done.
ziqingluo-90 added inline comments.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp:19
bool a = (bool) [5];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"(p.data() + 5)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"()[5]"
bool b = (bool) [x];
NoQ wrote:
> These changes look unrelated, probably accidentally included from another
> patch 樂
oh yes, need a rebase.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144304/new/
https://reviews.llvm.org/D144304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp:19
bool a = (bool) [5];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"(p.data() + 5)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"()[5]"
bool b = (bool) [x];
These changes look unrelated, probably accidentally included from another patch
樂
Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:37
-void testArraySubscripts(int *p, int **pp) {
+void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of
'pp' to 'std::span' to preserve bounds information}}
// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
These also seem unrelated.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144304/new/
https://reviews.llvm.org/D144304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
ziqingluo-90 updated this revision to Diff 503928.
ziqingluo-90 added a comment.
Rebased and addressed comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144304/new/
https://reviews.llvm.org/D144304
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -34,7 +34,7 @@
void * voidPtrCall(void);
char * charPtrCall(void);
-void testArraySubscripts(int *p, int **pp) {
+void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of 'pp' to 'std::span' to preserve bounds information}}
// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
// expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
foo(p[1], // expected-note{{used in buffer access here}}
@@ -108,7 +108,7 @@
sizeof(decltype(p[1]))); // expected-note{{used in buffer access here}}
}
-void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) {
+void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) { // expected-note{{change type of 'a' to 'std::span' to preserve bounds information}}
// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
// expected-warning@-2{{'q' is an unsafe pointer used for buffer access}}
// expected-warning@-3{{'a' is an unsafe pointer used for buffer access}}
@@ -323,7 +323,7 @@
template void fArr(int t[]); // expected-note {{in instantiation of}}
-int testReturn(int t[]) {
+int testReturn(int t[]) { // expected-note{{change type of 't' to 'std::span' to preserve bounds information}}
// expected-warning@-1{{'t' is an unsafe pointer used for buffer access}}
return t[1]; // expected-note{{used in buffer access here}}
}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
===
--- /dev/null
+++ 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])) {
+ }
+}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
@@ -16,17 +16,17 @@
void address_to_bool(int x) {
int * p = new int[10];
bool a = (bool) [5];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"(p.data() + 5)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"()[5]"
bool b = (bool) [x];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"(p.data() + x)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:19-[[@LINE-1]]:24}:"()[x]"
bool a1 = [5];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"(p.data() + 5)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"()[5]"
bool b1 = [x];
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"(p.data() + x)"
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:18}:"()[x]"
if ([5]) {
-// CHECK:
[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
ziqingluo-90 added inline comments. Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:663 +return stmt(isInUnspecifiedPointerContext(expr( + ignoringImpCasts(unaryOperator(isPreInc()).bind(UPCPreIncrementTag); + } NoQ wrote: > 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. 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(Node->getSubExpr()))` statement at line 671 could be a defensive check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144304/new/ https://reviews.llvm.org/D144304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D144304: [-Wunsafe-buffer-usage] Add a Fixable for pointer pre-increment
NoQ added inline comments.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:149-152
+// Matches a `UnaryOperator` whose operator is pre-increment:
+AST_MATCHER(UnaryOperator, isPreInc) {
+ return Node.getOpcode() == UnaryOperator::Opcode::UO_PreInc;
+}
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.
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:663
+return stmt(isInUnspecifiedPointerContext(expr(
+
ignoringImpCasts(unaryOperator(isPreInc()).bind(UPCPreIncrementTag);
+ }
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.
Comment at:
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp:25
+
+ // Don't know how to fix the following cases:
+ // CHECK-NOT: fix-it:"{{.*}}":{
It's probably a good idea to explicitly say `FIXME:` if we think these cases
should eventually be fixed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144304/new/
https://reviews.llvm.org/D144304
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
