ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: t-rasmud, NoQ, jkorous, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
- Use `Strategy` to determine whether to fix a parameter.
- Fix the `Strategy` construction so that only variables on the graph are
assigned the `std::span` strategy
- Extend `PointerAssignmentGadget` and `PointerInitGadget` to generate fix-its
for cases where the left-hand side has `won't fix` strategy and the right-hand
side has `std::span` strategy.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157441
Files:
clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.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
@@ -36,9 +36,11 @@
void * voidPtrCall(void);
char * charPtrCall(void);
-void testArraySubscripts(int *p, int **pp) {
-// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
-// expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
+void testArraySubscripts(int *p, int **pp) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-warning{{'pp' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'pp' to safe types to make function 'testArraySubscripts' bounds-safe}} \
+ expected-note{{change type of 'pp' to 'std::span' to preserve bounds information, and change 'p' to safe types to make function 'testArraySubscripts' bounds-safe}}
+
foo(p[1], // expected-note{{used in buffer access here}}
pp[1][1], // expected-note{{used in buffer access here}}
// expected-warning@-1{{unsafe buffer access}}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-warnings.cpp
@@ -65,7 +65,8 @@
void rhs_span2() {
int *q = new int[6];
- int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+ int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' to 'std::span' to propagate bounds information between them}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q;
}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-multi-decl-ptr-init.cpp
@@ -25,7 +25,8 @@
// FIXME: Suggest fixits for p, q, and r since span a valid fixit for r.
void rhs_span3() {
int *q = new int[6];
- int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+ int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information, and change 'q' to 'std::span' to propagate bounds information between them}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q;
}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-multi-parm-span.cpp
@@ -2,11 +2,12 @@
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fblocks -fsafe-buffer-usage-suggestions \
// RUN: %s 2>&1 | FileCheck %s
-// FIXME: what about possible diagnostic message non-determinism?
-
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
void parmsNoFix(int *p, int *q) {
int * a = new int[10];
+ // CHECK: fix-it:{{.*}}:{[[@LINE+3]]:3-[[@LINE+3]]:12}:"std::span<int> b"
+ // CHECK: fix-it:{{.*}}:{[[@LINE+2]]:13-[[@LINE+2]]:13}:"{"
+ // CHECK: fix-it:{{.*}}:{[[@LINE+1]]:24-[[@LINE+1]]:24}:", 10}"
int * b = new int[10]; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'b' to 'std::span' to preserve bounds information}}
@@ -27,7 +28,7 @@
int * b; //expected-warning{{'b' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'b' to 'std::span' to preserve bounds information, and change 'a' to 'std::span' to propagate bounds information between them}}
- b = a;
+ b = a; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
b[5] = 5; // expected-note{{used in buffer access here}}
p[5] = 5; // expected-note{{used in buffer access here}}
}
@@ -78,11 +79,11 @@
expected-note{{change type of 'z' to 'std::span' to preserve bounds information, and change 'x', 'p', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
int * y;
- x = p;
- y = x;
+ x = p; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
+ y = x; // CHECK: fix-it:{{.*}}:{[[@LINE]]:8-[[@LINE]]:8}:".data()"
// `x` needs to be fixed so does the pointer assigned to `x`, i.e.,`p`
x[5] = 5; // expected-note{{used in buffer access here}}
- z = r;
+ z = r; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
// `z` needs to be fixed so does the pointer assigned to `z`, i.e.,`r`
z[5] = 5; // expected-note{{used in buffer access here}}
// Since `p` and `r` are parameters need to be fixed together,
@@ -109,10 +110,10 @@
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
int * y;
- p = x;
- y = x;
+ p = x; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
+ y = x; // CHECK: fix-it:{{.*}}:{[[@LINE]]:8-[[@LINE]]:8}:".data()"
p[5] = 5; // expected-note{{used in buffer access here}}
- r = z;
+ r = z; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
r[5] = 5; // expected-note{{used in buffer access here}}
}
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void multiParmLocalAllFix2(int *p, int * r) {return multiParmLocalAllFix2(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
@@ -285,7 +286,7 @@
// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> y"
int * y; // expected-warning{{'y' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'y' to 'std::span' to preserve bounds information, and change 'x' to 'std::span' to propagate bounds information between them}}
- y = x;
+ y = x; // CHECK-NOT: fix-it:{{.*}}:{[[@LINE]]
tmp = y[5]; // expected-note{{used in buffer access here}}
}
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -13,6 +13,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/iterator_range.h"
#include <memory>
#include <optional>
#include <sstream>
@@ -1218,44 +1219,6 @@
return false;
}
-std::optional<FixItList>
-PointerAssignmentGadget::getFixits(const Strategy &S) const {
- const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
- const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
- switch (S.lookup(LeftVD)) {
- case Strategy::Kind::Span:
- if (S.lookup(RightVD) == Strategy::Kind::Span)
- return FixItList{};
- return std::nullopt;
- case Strategy::Kind::Wontfix:
- return std::nullopt;
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
- llvm_unreachable("unsupported strategies for FixableGadgets");
- }
- return std::nullopt;
-}
-
-std::optional<FixItList>
-PointerInitGadget::getFixits(const Strategy &S) const {
- const auto *LeftVD = PtrInitLHS;
- const auto *RightVD = cast<VarDecl>(PtrInitRHS->getDecl());
- switch (S.lookup(LeftVD)) {
- case Strategy::Kind::Span:
- if (S.lookup(RightVD) == Strategy::Kind::Span)
- return FixItList{};
- return std::nullopt;
- case Strategy::Kind::Wontfix:
- return std::nullopt;
- case Strategy::Kind::Iterator:
- case Strategy::Kind::Array:
- case Strategy::Kind::Vector:
- llvm_unreachable("unsupported strategies for FixableGadgets");
- }
- return std::nullopt;
-}
-
std::optional<FixItList>
ULCArraySubscriptGadget::getFixits(const Strategy &S) const {
if (const auto *DRE =
@@ -1530,6 +1493,59 @@
return SpanOpen + EltTyText.str() + '>';
}
+std::optional<FixItList>
+PointerAssignmentGadget::getFixits(const Strategy &S) const {
+ const auto *LeftVD = cast<VarDecl>(PtrLHS->getDecl());
+ const auto *RightVD = cast<VarDecl>(PtrRHS->getDecl());
+ switch (S.lookup(LeftVD)) {
+ case Strategy::Kind::Span:
+ if (S.lookup(RightVD) == Strategy::Kind::Span)
+ return FixItList{};
+ return std::nullopt;
+ case Strategy::Kind::Wontfix: {
+ if (S.lookup(RightVD) == Strategy::Kind::Span) {
+ const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in!
+ RightVD->getASTContext();
+ if (auto EndLocOfRHS =
+ getPastLoc(PtrRHS, Ctx.getSourceManager(), Ctx.getLangOpts()))
+ return FixItList{FixItHint::CreateInsertion(*EndLocOfRHS, ".data()")};
+ }
+ return std::nullopt;
+ }
+ case Strategy::Kind::Iterator:
+ case Strategy::Kind::Array:
+ case Strategy::Kind::Vector:
+ llvm_unreachable("unsupported strategies for FixableGadgets");
+ }
+ return std::nullopt;
+}
+
+std::optional<FixItList> PointerInitGadget::getFixits(const Strategy &S) const {
+ const auto *LeftVD = PtrInitLHS;
+ const auto *RightVD = cast<VarDecl>(PtrInitRHS->getDecl());
+ switch (S.lookup(LeftVD)) {
+ case Strategy::Kind::Span:
+ if (S.lookup(RightVD) == Strategy::Kind::Span)
+ return FixItList{};
+ return std::nullopt;
+ case Strategy::Kind::Wontfix: {
+ if (S.lookup(RightVD) == Strategy::Kind::Span) {
+ const ASTContext &Ctx = // FIXME: we need ASTContext to be passed in!
+ RightVD->getASTContext();
+ if (auto EndLocOfRHS =
+ getPastLoc(PtrInitRHS, Ctx.getSourceManager(), Ctx.getLangOpts()))
+ return FixItList{FixItHint::CreateInsertion(*EndLocOfRHS, ".data()")};
+ }
+ return std::nullopt;
+ }
+ case Strategy::Kind::Iterator:
+ case Strategy::Kind::Array:
+ case Strategy::Kind::Vector:
+ llvm_unreachable("unsupported strategies for FixableGadgets");
+ }
+ return std::nullopt;
+}
+
std::optional<FixItList>
DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
@@ -1605,12 +1621,10 @@
CharSourceRange derefRange = clang::CharSourceRange::getCharRange(
Op->getBeginLoc(), Op->getBeginLoc().getLocWithOffset(1));
// Inserts the [0]
- std::optional<SourceLocation> EndOfOperand =
- getEndCharLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts());
- if (EndOfOperand) {
+ if (auto LocPastOperand =
+ getPastLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts())) {
return FixItList{{FixItHint::CreateRemoval(derefRange),
- FixItHint::CreateInsertion(
- (*EndOfOperand).getLocWithOffset(1), "[0]")}};
+ FixItHint::CreateInsertion(*LocPastOperand, "[0]")}};
}
break;
}
@@ -1948,19 +1962,9 @@
// [[clang::unsafe_buffer_usage]] void f(int *p) { // added def
// return f(std::span(p, <# size #>));
// }
-//
-// The actual fix-its may contain more details, e.g., the attribute may be guard
-// by a macro
-// #if __has_cpp_attribute(clang::unsafe_buffer_usage)
-// [[clang::unsafe_buffer_usage]]
-// #endif
-//
-// `ParmsMask` is an array of size of `FD->getNumParams()` such
-// that `ParmsMask[i]` is true iff the `i`-th parameter will be fixed with some
-// strategy.
static std::optional<FixItList>
-createOverloadsForFixedParams(const std::vector<bool> &ParmsMask, const Strategy &S,
- const FunctionDecl *FD, const ASTContext &Ctx,
+createOverloadsForFixedParams(const Strategy &S, const FunctionDecl *FD,
+ const ASTContext &Ctx,
UnsafeBufferUsageHandler &Handler) {
// FIXME: need to make this conflict checking better:
if (hasConflictingOverload(FD))
@@ -1970,21 +1974,36 @@
const LangOptions &LangOpts = Ctx.getLangOpts();
const unsigned NumParms = FD->getNumParams();
std::vector<std::string> NewTysTexts(NumParms);
+ std::vector<bool> ParmsMask(NumParms, false);
+ bool AtLeastOneParmToFix = false;
for (unsigned i = 0; i < NumParms; i++) {
- if (!ParmsMask[i])
- continue;
+ const ParmVarDecl *PVD = FD->getParamDecl(i);
+ if (S.lookup(PVD) == Strategy::Kind::Wontfix)
+ continue;
+ if (S.lookup(PVD) != Strategy::Kind::Span) {
+ // Not supported, not suppose to happen:
+ DEBUG_NOTE_UNEXPECTED_ISSUE(PVD,
+ "unsupported strategy kind for parameter " +
+ PVD->getNameAsString());
+ return std::nullopt;
+ }
std::optional<Qualifiers> PteTyQuals = std::nullopt;
std::optional<std::string> PteTyText =
- getPointeeTypeText(FD->getParamDecl(i), SM, LangOpts, &PteTyQuals);
+ getPointeeTypeText(PVD, SM, LangOpts, &PteTyQuals);
if (!PteTyText)
// something wrong in obtaining the text of the pointee type, give up
return std::nullopt;
// FIXME: whether we should create std::span type depends on the Strategy.
NewTysTexts[i] = getSpanTypeText(*PteTyText, PteTyQuals);
+ ParmsMask[i] = true;
+ AtLeastOneParmToFix = true;
}
+ if (!AtLeastOneParmToFix)
+ // No need to create function overloads:
+ return {};
// FIXME Respect indentation of the original code.
// A lambda that creates the text representation of a function declaration
@@ -2287,28 +2306,17 @@
const VariableGroupsManager &VarGrpMgr, const FunctionDecl *FD,
const Strategy &S, ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) {
FixItList FixItsSharedByParms{};
- std::vector<bool> ParmsNeedFixMask(FD->getNumParams(), false);
- const VarDecl *FirstParmNeedsFix = nullptr;
-
- for (unsigned i = 0; i < FD->getNumParams(); i++)
- if (FixItsForVariable.count(FD->getParamDecl(i))) {
- ParmsNeedFixMask[i] = true;
- FirstParmNeedsFix = FD->getParamDecl(i);
- }
- if (FirstParmNeedsFix) {
- // In case at least one parameter needs to be fixed:
- std::optional<FixItList> OverloadFixes =
- createOverloadsForFixedParams(ParmsNeedFixMask, S, FD, Ctx, Handler);
+ std::optional<FixItList> OverloadFixes =
+ createOverloadsForFixedParams(S, FD, Ctx, Handler);
- if (OverloadFixes) {
- FixItsSharedByParms.append(*OverloadFixes);
- } else {
- // Something wrong in generating `OverloadFixes`, need to remove the
- // whole group, where parameters are in, from `FixItsForVariable` (Note
- // that all parameters should be in the same group):
- for (auto *Member : VarGrpMgr.getGroupOfVar(FirstParmNeedsFix))
- FixItsForVariable.erase(Member);
- }
+ if (OverloadFixes) {
+ FixItsSharedByParms.append(*OverloadFixes);
+ } else {
+ // Something wrong in generating `OverloadFixes`, need to remove the
+ // whole group, where parameters are in, from `FixItsForVariable` (Note
+ // that all parameters should be in the same group):
+ for (auto *Member : VarGrpMgr.getGroupOfParms())
+ FixItsForVariable.erase(Member);
}
return FixItsSharedByParms;
}
@@ -2413,8 +2421,9 @@
return FinalFixItsForVariable;
}
+template <typename VarDeclIterTy>
static Strategy
-getNaiveStrategy(const llvm::SmallVectorImpl<const VarDecl *> &UnsafeVars) {
+getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
Strategy S;
for (const VarDecl *VD : UnsafeVars) {
S.set(VD, Strategy::Kind::Span);
@@ -2448,6 +2457,10 @@
return std::nullopt;
return Groups[VarGrpMap.at(Var)];
}
+
+ VarGrpRef getGroupOfParms() const override {
+ return GrpsUnionForParms.getArrayRef();
+ }
};
void clang::checkUnsafeBufferUsage(const Decl *D,
@@ -2700,7 +2713,10 @@
++I;
}
- Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
+ // We assign strategies to variables on the graph. Other variables have the
+ // default "Won't fix" strategy.
+ Strategy NaiveStrategy = getNaiveStrategy(
+ llvm::make_range(VisitedVars.begin(), VisitedVars.end()));
VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);
if (isa<NamedDecl>(D))
Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
===================================================================
--- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -35,6 +35,10 @@
/// variables, where `Var` is in, contains parameters.
virtual VarGrpRef getGroupOfVar(const VarDecl *Var,
bool *HasParm = nullptr) const;
+
+ /// Returns the non-empty group of variables that include parameters of the
+ /// analyzing function, if such a group exists. An empty group, otherwise.
+ virtual VarGrpRef getGroupOfParms() const;
};
/// The interface that lets the caller handle unsafe buffer usage analysis
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
