ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: jkorous, NoQ, t-rasmud, malavikasamak,
aaron.ballman, xazax.hun, gribozavr.
Herald added a subscriber: rnkovacs.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Use clang fix-its to transform declarations of local variables, which are used
for buffer access , to be of `std::span` type.
We placed a few limitations to keep the solution simple:
1. it only transforms local variable declarations (no parameter declaration)
2. it only considers single level pointers, i.e., pointers of type `T *`
regardless of whether `T` is again a pointer.
3. it only transforms to `std::span` types (no `std::array`, or
`std::span::iterator`, or ...)
4. it can only transform a `VarDecl` that belongs to a `DeclStmt` whose has a
single child.
One of the purposes of keeping this patch simple enough is to first evaluate if
fix-it is an appropriate approach to do the transformation.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D139737
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.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
@@ -69,19 +69,19 @@
auto ap1 = a; // expected-warning{{variable 'ap1' participates in unchecked buffer operations}}
- foo(ap1[1]); // expected-warning{{unchecked operation on raw buffer in expression}}
+ foo(ap1[1]);
auto ap2 = p; // expected-warning{{variable 'ap2' participates in unchecked buffer operations}}
- foo(ap2[1]); // expected-warning{{unchecked operation on raw buffer in expression}}
+ foo(ap2[1]);
- auto ap3 = pp;
+ auto ap3 = pp; // expected-warning{{variable 'ap3' participates in unchecked buffer operations}}
- foo(ap3[1][1]); // expected-warning2{{unchecked operation on raw buffer in expression}}
+ foo(ap3[1][1]); // expected-warning{{unchecked operation on raw buffer in expression}}
auto ap4 = *pp; // expected-warning{{variable 'ap4' participates in unchecked buffer operations}}
- foo(ap4[1]); // expected-warning{{unchecked operation on raw buffer in expression}}
+ foo(ap4[1]);
}
void testQualifiedParameters(const int * p, const int * const q,
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp
@@ -3,16 +3,71 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fixit %t.cpp
// RUN: grep -v CHECK %t.cpp | FileCheck %s
+void foo(...);
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
void local_array_subscript_simple() {
-// CHECK: std::span<int, {{.*}}> p = new int[10];
+// CHECK: std::span<int> p{new int [10], 10};
// CHECK: p[5] = 5;
int *p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
p[5] = 5;
+
+// CHECK: std::span<const int> q{new int [10], 10};
+// CHECK: std::span<int> x{new int [10], 10};
+// CHECK: std::span<int> y{new int, 1};
+// CHECK: std::span<Int_t> z{new int [10], 10};
+// CHECK: std::span<Int_t> w{new Int_t [10], 10};
+// CHECK: foo(q[5], x[5], y[5], z[5], w[5]);
+ const int *q = new int[10]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+ Int_ptr_t x = new int[10]; // expected-warning{{variable 'x' participates in unchecked buffer operations}}
+ Int_ptr_t y = new int; // expected-warning{{variable 'y' participates in unchecked buffer operations}}
+ Int_t * z = new int[10]; // expected-warning{{variable 'z' participates in unchecked buffer operations}}
+ Int_t * w = new Int_t[10]; // expected-warning{{variable 'w' participates in unchecked buffer operations}}
+ foo(q[5], x[5], y[5], z[5], w[5]);
+ // y[5] will crash after being span
}
void local_array_subscript_auto() {
-// CHECK: std::span<int, {{.*}}> p = new int[10];
+// CHECK: std::span<int> p{new int [10], 10};
// CHECK: p[5] = 5;
auto p = new int[10]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
p[5] = 5;
}
+
+void local_array_subscript_variable_extent() {
+ int n = 10;
+// CHECK: std::span<int> p{new int [n], n};
+// CHECK: std::span<int> q{new int [n++], ...};
+// CHECK: foo(p[5], q[5]);
+ int *p = new int[n]; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+ // If the extent expression does not have a constant value, we cannot fill the extent for users...
+ int *q = new int[n++]; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+ foo(p[5], q[5]);
+}
+
+
+void local_ptr_to_array() {
+ int n = 10;
+ int a[10];
+ int b[n]; // If the extent expression does not have a constant value, we cannot fill the extent for users...
+// CHECK: std::span<int> p{a, 10};
+// CHECK: std::span<int> q{b, ...};
+// CHECK: foo(p[5], q[5]);
+ int *p = a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+ int *q = b; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+ foo(p[5], q[5]);
+}
+
+void local_ptr_addrof_init() {
+ int a[10];
+ int var;
+// CHECK: std::span<int[10]> p{&a, 1};
+// CHECK: std::span<int> q{&var, 1};
+// CHECK: foo(p[5], q[5]);
+ int (*p)[10] = &a; // expected-warning{{variable 'p' participates in unchecked buffer operations}}
+ int * q = &var; // expected-warning{{variable 'q' participates in unchecked buffer operations}}
+ // These two expressions involve unsafe buffer accesses, which will
+ // crash at runtime after applying the fix-it,
+ foo(p[5], q[5]);
+}
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -610,8 +610,120 @@
return {std::move(CB.Gadgets), std::move(CB.Tracker)};
}
+// For a non-null initializer `Init` of `T *` type, this function writes
+// `{Init, S}` as a part of a fix-it to output stream . The list-initializer
+// `{Init, S}` belongs to a span constructor, where `Init` specifies the
+// address of the data and `S` is the extent. 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 span object being constructed
+// will have type `span<T, S>`.
+//
+// FIXME: This function so far only works for spanify "single-level" pointers.
+// When it comes to multi-level pointers, the data address is not necessarily
+// identical to `Init` and the element type of the constructing span object is
+// not necessarily `T`.
+//
+// Parameters:
+// `Init` a pointer to the initializer expression
+// `Ctx` a reference to the ASTContext
+// `OS` the output stream where fix-it texts being written to
+static void populateInitializerFixItWithSpan(const Expr *Init,
+ const ASTContext &Ctx, raw_ostream &OS) {
+ const PrintingPolicy &PP = Ctx.getPrintingPolicy();
+
+ // Prints the begin address of the span constructor:
+ OS << "{";
+ Init->printPretty(OS, nullptr, PP);
+ OS << ", ";
+
+ bool ExtKnown = false; // true iff the extent can be obtained
+ // Printers that print extent into OS and sets ExtKnown to true:
+ std::function PrintExpr = [&ExtKnown, &OS, &PP](const Expr *Size) {
+ Size->printPretty(OS, nullptr, PP);
+ ExtKnown = true;
+ };
+ std::function PrintAPInt = [&ExtKnown, &OS](const APInt &Size) {
+ Size.print(OS, false);
+ ExtKnown = true;
+ };
+ std::function PrintOne = [&ExtKnown, &OS](void) {
+ OS << "1";
+ ExtKnown = true;
+ };
+
+ // Try to get the data extent. Break into different cases:
+ if (auto CxxNew = dyn_cast<CXXNewExpr>(Init->IgnoreImpCasts())) {
+ // In cases `Init` is `new T[n]` and there is no explicit cast over
+ // `Init`, we know that `Init` must evaluates to a pointer to `n` objects
+ // of `T`. So the extent is `n` unless `n` has side effects. Similar but
+ // simpler for `Init` is `new T`.
+ if (const Expr *Ext = CxxNew->getArraySize().value_or(nullptr)) {
+ if (!Ext->HasSideEffects(Ctx))
+ PrintExpr(Ext);
+ } else if (!CxxNew->isArray())
+ PrintOne();
+ } else if (auto CArrTy = dyn_cast<ConstantArrayType>(
+ Init->IgnoreImpCasts()->getType().getTypePtr())) {
+ // In cases `Init` is of an array type after stripping off implicit casts,
+ // `PointeeTy` must besimilar to the element type of `CArrTy`, so the
+ // extent is the array size of `CArrTy`. Note that if the array size is
+ // not a constant, we cannot use it as the extent.
+ PrintAPInt(CArrTy->getSize());
+ // FIXME: local static array may be converted to std:array. Is there any
+ // dependency among these fix-its?
+ } else {
+ // In cases `Init` is of the form `&Var` after stripping of implicit
+ // casts, where `&` is the built-in operator, `PointeeTy` must be similar
+ // to the type of `Var`, so the extent is 1.
+ if (auto AddrOfExpr = dyn_cast<UnaryOperator>(Init->IgnoreImpCasts()))
+ if (AddrOfExpr->getOpcode() == UnaryOperatorKind::UO_AddrOf &&
+ isa_and_present<DeclRefExpr>(AddrOfExpr->getSubExpr()))
+ PrintOne();
+ // TODO: we can handle more cases, e.g., `&a[0]`, `&a`, std::addressof,...
+ // etc.
+ }
+ if (!ExtKnown)
+ OS << UserFillPlaceHolder;
+ OS << "}";
+}
+
+// For a `VarDecl` of the form `T * var (= Init)?`, this
+// function generates a fix-it for the declaration, which re-declares `var` to
+// be of `span<T>` type and transforms the initializer, if present, to a span
+// constructor---`span<T> var {Init, Extent}`, where `Extent` may need the user
+// to fill in.
+//
+// FIXME:
+// For now, we only consider single level pointer types, i.e., given `T` is
+// `int **`, the converted type will be `span<int*>`.
+// We need to handle multi-level pointers correctly later.
+//
+// Parameters:
+// `D` a pointer the variable declaration node
+// `Ctx` a reference to the ASTContext
+// Returns:
+// the generated fix-it
+static FixItHint fixVarDeclWithSpan(const VarDecl *D,
+ const ASTContext &Ctx) {
+ const QualType &T = D->getType().getDesugaredType(Ctx);
+ const QualType &SpanEltT = T->getPointeeType();
+ assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");
+
+ SmallString<32> Replacement;
+ raw_svector_ostream OS(Replacement);
+
+ // Simply make `D` to be of span type:
+ OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();
+ if (const Expr *Init = D->getInit())
+ populateInitializerFixItWithSpan(Init, Ctx, OS);
+
+ return FixItHint::CreateReplacement(
+ SourceRange{D->getInnerLocStart(), D->getEndLoc()}, OS.str());
+}
+
static FixItList fixVariableWithSpan(const VarDecl *VD,
- const DeclUseTracker &Tracker) {
+ const DeclUseTracker &Tracker,
+ const ASTContext &Ctx) {
const DeclStmt *DS = Tracker.lookupDecl(VD);
assert(DS && "Fixing non-local variables not implemented yet!");
assert(DS->getSingleDecl() == VD &&
@@ -621,27 +733,17 @@
// and the '*' are spread around the place.
(void)DS;
- QualType T = VD->getType()->getPointeeType();
- assert(!T.isNull() && "Trying to fix a non-pointer type variable!");
-
- SmallString<32> Replacement;
- raw_svector_ostream OS(Replacement);
- OS << "std::span<" << T.getAsString() << ", ...>";
- if (!VD->getType()->getAs<AutoType>())
- OS << " ";
-
- FixItHint Fix = FixItHint::CreateReplacement(
- SourceRange{VD->getTypeSpecStartLoc(), VD->getTypeSpecEndLoc()},
- OS.str());
-
+ // FIXME: handle cases where DS has multiple declarations
+ FixItHint Fix = fixVarDeclWithSpan(VD, Ctx);
return {Fix};
}
static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K,
- const DeclUseTracker &Tracker) {
+ const DeclUseTracker &Tracker,
+ const ASTContext &Ctx) {
switch (K) {
case Strategy::Kind::Span:
- return fixVariableWithSpan(VD, Tracker);
+ return fixVariableWithSpan(VD, Tracker, Ctx);
case Strategy::Kind::Iterator:
case Strategy::Kind::Array:
case Strategy::Kind::Vector:
@@ -683,6 +785,7 @@
}
Strategy S;
+ const ASTContext &Ctx = D->getASTContext();
for (const auto &Item : Map) {
const VarDecl *VD = Item.first;
@@ -724,7 +827,7 @@
if (Fixes) {
// If we reach this point, the strategy is applicable.
// Add a fixit for the variable itself and emit all fixes.
- FixItList VarF = fixVariable(VD, S.lookup(VD), Tracker);
+ FixItList VarF = fixVariable(VD, S.lookup(VD), Tracker, Ctx);
for (auto &&Fixit : VarF)
Fixes->push_back(std::move(Fixit));
@@ -740,3 +843,4 @@
}
}
}
+
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -41,6 +41,7 @@
// through the handler class.
void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler);
+static std::string UserFillPlaceHolder = "...";
} // end namespace clang
#endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
