Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Use Strategy to determine whether to fix a …
Browse files Browse the repository at this point in the history
…parameter

- 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

Reviewed by: t-rasmud (Rashmi Mudduluru), NoQ (Artem Dergachev)

Differential revision: https://reviews.llvm.org/D157441
  • Loading branch information
ziqingluo-90 committed Sep 21, 2023
1 parent e0be78b commit 700baeb
Show file tree
Hide file tree
Showing 10 changed files with 117 additions and 136 deletions.
4 changes: 4 additions & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ class VariableGroupsManager {
/// variables, where `Var` is in, contains parameters.
virtual VarGrpRef getGroupOfVar(const VarDecl *Var,
bool *HasParm = nullptr) const =0;

/// 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 =0;
};

/// The interface that lets the caller handle unsafe buffer usage analysis
Expand Down
94 changes: 50 additions & 44 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1634,12 +1634,10 @@ PointerDereferenceGadget::getFixits(const Strategy &S) const {
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;
}
Expand Down Expand Up @@ -1978,18 +1976,9 @@ static bool hasConflictingOverload(const FunctionDecl *FD) {
// 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))
Expand All @@ -1999,21 +1988,33 @@ createOverloadsForFixedParams(const std::vector<bool> &ParmsMask, const Strategy
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])
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:
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
Expand Down Expand Up @@ -2322,28 +2323,18 @@ static FixItList createFunctionOverloadsForParms(
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;
}
Expand Down Expand Up @@ -2448,8 +2439,9 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const Strategy &S,
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);
Expand Down Expand Up @@ -2486,6 +2478,10 @@ class VariableGroupsManagerImpl : public VariableGroupsManager {
return std::nullopt;
return Groups[It->second];
}

VarGrpRef getGroupOfParms() const override {
return GrpsUnionForParms.getArrayRef();
}
};

void clang::checkUnsafeBufferUsage(const Decl *D,
Expand Down Expand Up @@ -2579,6 +2575,14 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
it->first, it->first->getBeginLoc(),
("failed to produce fixit for '" + it->first->getNameAsString() +
"' : neither local nor a parameter"));
#endif
it = FixablesForAllVars.byVar.erase(it);
} else if (it->first->getType().getCanonicalType()->isReferenceType()) {
#ifndef NDEBUG
Handler.addDebugNoteForVar(it->first, it->first->getBeginLoc(),
("failed to produce fixit for '" +
it->first->getNameAsString() +
"' : has a reference type"));
#endif
it = FixablesForAllVars.byVar.erase(it);
} else if (Tracker.hasUnclaimedUses(it->first)) {
Expand All @@ -2605,10 +2609,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}

llvm::SmallVector<const VarDecl *, 16> UnsafeVars;
for (const auto &[VD, ignore] : FixablesForAllVars.byVar)
UnsafeVars.push_back(VD);

// Fixpoint iteration for pointer assignments
using DepMapTy = DenseMap<const VarDecl *, llvm::SetVector<const VarDecl *>>;
DepMapTy DependenciesMap{};
Expand Down Expand Up @@ -2737,7 +2737,13 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
++I;
}

Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars);
// We assign strategies to variables that are 1) in the graph and 2) can be
// fixed. Other variables have the default "Won't fix" strategy.
Strategy NaiveStrategy = getNaiveStrategy(llvm::make_filter_range(
VisitedVars, [&FixablesForAllVars](const VarDecl *V) {
// If a warned variable has no "Fixable", it is considered unfixable:
return FixablesForAllVars.byVar.count(V);
}));
VariableGroupsManagerImpl VarGrpMgr(Groups, VarGrpMap, GrpsUnionForParms);

if (isa<NamedDecl>(D))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,15 @@ void * multiParmAllFix(int *p, int **q, int a[], int * r);

// Fixing local variables implicates fixing parameters
void multiParmLocalAllFix(int *p, int * r) {
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:28-[[@LINE-1]]:34}:"std::span<int> p"
// CHECK: fix-it:{{.*}}:{[[@LINE-2]]:36-[[@LINE-2]]:43}:"std::span<int> r"
// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> x"
int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'x' to 'std::span' to preserve bounds information, and change 'p', 'z', and 'r' to safe types to make function 'multiParmLocalAllFix' bounds-safe}}
// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:3-[[@LINE+1]]:10}:"std::span<int> z"
int * z; // expected-warning{{'z' is an unsafe pointer used for buffer access}} \
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}}
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]:
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
int * x; // expected-warning{{'x' is an unsafe pointer used for buffer access}}
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
int * z; // expected-warning{{'z' is an unsafe pointer used for buffer access}}
int * y;

x = p;
y = x;
y = x; // FIXME: we do not fix `y = x` here as the `.data()` fix-it is not generally correct
// `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;
Expand All @@ -89,33 +86,26 @@ void multiParmLocalAllFix(int *p, int * r) {
// fixing `x` involves fixing all `p`, `r` and `z`. Similar for
// fixing `z`.
}
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void multiParmLocalAllFix(int *p, int * r) {return multiParmLocalAllFix(std::span<int>(p, <# size #>), std::span<int>(r, <# size #>));}\n"
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]:


// Fixing parameters implicates fixing local variables
// CHECK: fix-it:{{.*}}:{[[@LINE+2]]:29-[[@LINE+2]]:35}:"std::span<int> p"
// CHECK: fix-it:{{.*}}:{[[@LINE+1]]:37-[[@LINE+1]]:44}:"std::span<int> r"
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]:
void multiParmLocalAllFix2(int *p, int * r) { // 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 'x', 'r', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}} \
expected-warning{{'r' is an unsafe pointer used for buffer access}} \
expected-note{{change type of 'r' to 'std::span' to preserve bounds information, and change 'p', 'x', and 'z' to safe types to make function 'multiParmLocalAllFix2' bounds-safe}}
expected-warning{{'r' is an unsafe pointer used for buffer access}}
int * x = new int[10];
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> x"
// CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]:
int * z = new int[10];
// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> z"
// CHECK: fix-it:{{.*}}:{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK: fix-it:{{.*}}:{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]:
int * y;

p = x;
y = x;
y = x; // FIXME: we do not fix `y = x` here as the `.data()` fix-it is not generally correct
p[5] = 5; // expected-note{{used in buffer access here}}
r = z;
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"
// CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]:


// No fix emitted for any of the parameter since parameter `r` cannot be fixed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,14 @@

void foo1a() {
int *r = new int[7];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p = new int[4];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
p = r;
int tmp = p[9];
int *q;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
q = r;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
q = r; // FIXME: we do not fix `q = r` here as the `.data()` fix-it is not generally correct
}

void foo1b() {
Expand All @@ -37,15 +33,13 @@ void foo1b() {

void foo1c() {
int *r = new int[7];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p = new int[4];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
p = r;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
p = r; // FIXME: we do not fix `p = r` here as the `.data()` fix-it is not generally correct
int tmp = r[9];
int *q;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
q = r;
tmp = q[9];
}
Expand All @@ -70,18 +64,12 @@ void foo2a() {

void foo2b() {
int *r = new int[7];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p = new int[5];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 5}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *q = new int[4];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
p = q;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
p = q; // FIXME: we do not fix `p = q` here as the `.data()` fix-it is not generally correct
int tmp = q[8];
q = r;
}
Expand All @@ -107,14 +95,12 @@ void foo2c() {

void foo3a() {
int *r = new int[7];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p = new int[5];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 5}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *q = new int[4];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
q = p;
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
q = p; // FIXME: we do not fix `q = p` here as the `.data()` fix-it is not generally correct
int tmp = p[8];
q = r;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,11 @@ void test_grouping() {
int *z = new int[8];
int tmp;
int *y = new int[10];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> y"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
tmp = y[5];

int *x = new int[10];
x = y;
x = y; // FIXME: we do not fix `x = y` here as the `.data()` fix-it is not generally correct

int *w = z;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ void rhs_span3() {
int *q = new int[6];
int *p = q; // expected-warning{{'p' is an unsafe pointer used for buffer access}}
p[5] = 10; // expected-note{{used in buffer access here}}
int *r = q;
int *r = q; // FIXME: we do not fix `int *r = q` here as the `.data()` fix-it is not generally correct
}

void test_grouping() {
int *z = new int[8];
int tmp;
int *y = new int[10]; // expected-warning{{'y' is an unsafe pointer used for buffer access}} expected-note{{change type of 'y' to 'std::span' to preserve bounds information}}
int *y = new int[10]; // expected-warning{{'y' is an unsafe pointer used for buffer access}}
tmp = y[5]; // expected-note{{used in buffer access here}}

int *x = new int[10];
x = y;
x = y; // FIXME: we do not fix `x = y` here as the `.data()` fix-it is not generally correct

int *w = z;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,13 @@ void bar(int * param) {}

void foo1a() {
int *r = new int[7];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> r"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 7}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
int *p = new int[4];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:22-[[@LINE-3]]:22}:", 4}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:
p = r;
int tmp = p[9];
int *q;
q = r;
q = r; // FIXME: we do not fix `q = r` here as the `.data()` fix-it is not generally correct
}

void uuc_if_body() {
Expand Down

0 comments on commit 700baeb

Please sign in to comment.