Skip to content

Commit

Permalink
[-Wunsafe-buffer-usage] Refactor to let local variable fix-its and pa…
Browse files Browse the repository at this point in the history 
…rameter fix-its share common code

Refactor the code for local variable fix-its so that it reuses the
code for parameter fix-its, which is in general better. For example,
cv-qualifiers are supported.

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

Differential revision: https://reviews.llvm.org/D156189
  • Loading branch information
@ziqingluo-90
ziqingluo-90 committed Aug 21, 2023
1 parent d7f3b23 commit 3a67b91
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 109 deletions.
85 changes: 46 additions & 39 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1716,7 +1716,7 @@ std::optional<FixItList> UPCPreIncrementGadget::getFixits(const Strategy &S) con
// `Init` a pointer to the initializer expression
// `Ctx` a reference to the ASTContext
static FixItList
populateInitializerFixItWithSpan(const Expr *Init, ASTContext &Ctx,
FixVarInitializerWithSpan(const Expr *Init, ASTContext &Ctx,
const StringRef UserFillPlaceHolder) {
const SourceManager &SM = Ctx.getSourceManager();
const LangOptions &LangOpts = Ctx.getLangOpts();
Expand Down Expand Up @@ -1827,59 +1827,67 @@ static std::optional<std::string> createSpanTypeForVarDecl(const VarDecl *VD,
}

// 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.
// function generates fix-its that
// 1) replace `T * var` with `std::span<T> var`; and
// 2) change `Init` accordingly to a span constructor, if it exists.
//
// FIXME: support Multi-level pointers
//
// Parameters:
// `D` a pointer the variable declaration node
// `Ctx` a reference to the ASTContext
// `UserFillPlaceHolder` the user-input placeholder text
// Returns:
// the generated fix-it
static FixItList fixVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
const StringRef UserFillPlaceHolder,
UnsafeBufferUsageHandler &Handler) {
(void)Handler; // Suppress unused variable warning in release builds.
const QualType &SpanEltT = D->getType()->getPointeeType();
assert(!SpanEltT.isNull() && "Trying to fix a non-pointer type variable!");

// the non-empty fix-it list, if fix-its are successfuly generated; empty
// list otherwise.
static FixItList fixLocalVarDeclWithSpan(const VarDecl *D, ASTContext &Ctx,
const StringRef UserFillPlaceHolder,
UnsafeBufferUsageHandler &Handler) {
FixItList FixIts{};
std::optional<SourceLocation>
ReplacementLastLoc; // the inclusive end location of the replacement
const SourceManager &SM = Ctx.getSourceManager();
std::optional<std::string> SpanTyText = createSpanTypeForVarDecl(D, Ctx);

if (!SpanTyText) {
DEBUG_NOTE_DECL_FAIL(D, " : failed to generate 'std::span' type");
return {};
}

// Will hold the text for `std::span<T> Ident`:
std::stringstream SS;

SS << *SpanTyText;
// Append qualifiers to the type of `D`, if any:
if (D->getType().hasQualifiers())
SS << " " << D->getType().getQualifiers().getAsString();

// The end of the range of the original source that will be replaced
// by `std::span<T> ident`:
SourceLocation EndLocForReplacement = D->getEndLoc();
std::optional<StringRef> IdentText =
getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());

if (!IdentText) {
DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
return {};
}
// Fix the initializer if it exists:
if (const Expr *Init = D->getInit()) {
FixItList InitFixIts =
populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder);

if (InitFixIts.empty()) {
DEBUG_NOTE_DECL_FAIL(D, " : empty initializer");
FixVarInitializerWithSpan(Init, Ctx, UserFillPlaceHolder);
if (InitFixIts.empty())
return {};
}

// The loc right before the initializer:
ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1);
FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()),
std::make_move_iterator(InitFixIts.end()));
} else
ReplacementLastLoc = getEndCharLoc(D, SM, Ctx.getLangOpts());

// Producing fix-it for variable declaration---make `D` to be of span type:
SmallString<32> Replacement;
raw_svector_ostream OS(Replacement);

OS << "std::span<" << SpanEltT.getAsString() << "> " << D->getName();

if (!ReplacementLastLoc) {
DEBUG_NOTE_DECL_FAIL(D, " : failed to get end char loc (macro)");
// If the declaration has the form `T *ident = init`, we want to replace
// `T *ident = ` with `std::span<T> ident`:
EndLocForReplacement = Init->getBeginLoc().getLocWithOffset(-1);
}
SS << " " << IdentText->str();
if (!EndLocForReplacement.isValid()) {
DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the end of the declaration");
return {};
}

FixIts.push_back(FixItHint::CreateReplacement(
SourceRange{D->getBeginLoc(), *ReplacementLastLoc}, OS.str()));
SourceRange(D->getBeginLoc(), EndLocForReplacement), SS.str()));
return FixIts;
}

Expand Down Expand Up @@ -2139,8 +2147,7 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
(void)DS;

// FIXME: handle cases where DS has multiple declarations
return fixVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(),
Handler);
return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
}

// TODO: we should be consistent to use `std::nullopt` to represent no-fix due
Expand Down
110 changes: 76 additions & 34 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,53 +4,66 @@
typedef int * Int_ptr_t;
typedef int Int_t;

#define DEFINE_PTR(X) int* ptr = (X);

void local_array_subscript_simple() {
int tmp;
int *p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
// 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]]:23-[[@LINE-3]]:23}:", 10}"
const int *q = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span<const int> q"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::span<int const> q"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:18-[[@LINE-2]]:18}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:29-[[@LINE-3]]:29}:", 10}"
tmp = p[5];
tmp = q[5];

// We do not fix the following declaration. Because if the
// definition of `Int_ptr_t` gets changed, the fixed code becomes
// incorrect and may NOT be noticed.
// FIXME: Fix with std::span<std::remove_pointer_t<Int_ptr_t>>?
Int_ptr_t x = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> x"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
Int_ptr_t y = new int;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:16}:"std::span<int> y"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:17-[[@LINE-2]]:17}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 1}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
Int_t * z = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> z"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> z"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
Int_t * w = new Int_t[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> w"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<Int_t> w"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:28-[[@LINE-3]]:28}:", 10}"

tmp = x[5];
tmp = y[5]; // y[5] will crash after being span
tmp = z[5];
tmp = w[5];
}

void local_array_subscript_auto() {
int tmp;
// We do not fix the following declaration because
// that'd cause us to hardcode the element type.
// FIXME: Can we use the C++17 class template argument deduction
// to avoid spelling out the element type?
auto p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
tmp = p[5];
}

void local_variable_qualifiers_specifiers() {
int a[10];
const int * p = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::span<int const> p"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", 10}"
const int * const q = a;
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:24}:"std::span<int const> const q"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:25-[[@LINE-2]]:25}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:26-[[@LINE-3]]:26}:", 10}"
int tmp;
tmp = p[5];
tmp = q[5];
}


void local_array_subscript_variable_extent() {
int n = 10;
int tmp;
Expand Down Expand Up @@ -102,8 +115,8 @@ void decl_without_init() {
int * p;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:10}:"std::span<int> p"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
Int_ptr_t q;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:14}:"std::span<int> q"
Int_t * q;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<Int_t> q"
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}}
tmp = p[5];
tmp = q[5];
Expand Down Expand Up @@ -168,15 +181,42 @@ void null_init() {

void unsupported_multi_decl(int * x) {
int * p = x, * q = new int[10];
// CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
*p = q[5];
}

void macroVariableIdentifier() {
#define MY_NAME p
#define MY_NAME_ARG(x) q

// Although fix-its include macros, the macros do not overlap with
// the bounds of the source range of these fix-its. So these fix-its
// are valid.

int * MY_NAME = new int[10];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:18}:"std::span<int> MY_NAME"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:30-[[@LINE-3]]:30}:", 10}"
int * MY_NAME_ARG( 'x' ) = new int[10];
// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:29}:"std::span<int> MY_NAME_ARG( 'x' )"
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:30-[[@LINE-2]]:30}:"{"
// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:41-[[@LINE-3]]:41}:", 10}"
p[5] = 5;
q[5] = 5;
#undef MY_NAME
#undef MY_NAME_ARG
}

void unsupported_fixit_overlapping_macro(int * x) {
int tmp;
// In the case below, a tentative fix-it replaces `MY_INT * p =` with `std::span<MY_INT> p `.
// It overlaps with the use of the macro `MY_INT`. The fix-it is
// discarded then.
// The bounds of the source range of the fix-it overlap with the use of the macro
// `MY_INT`. The fix-it is discarded then.

// FIXME: we do not have to discard a fix-it if its begin location
// overlaps with the begin location of a macro. Similar for end
// locations.

#define MY_INT int
MY_INT * p = new int[10];
// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
Expand Down Expand Up @@ -227,6 +267,8 @@ void unsupported_subscript_negative(int i, unsigned j, unsigned long k) {
tmp = r[(unsigned int)-1]; // a cast-to-unsigned-expression is also non-negative
}

#define DEFINE_PTR(X) int* ptr = (X);

void all_vars_in_macro() {
int* local;
DEFINE_PTR(local)
Expand All @@ -239,10 +281,10 @@ void few_vars_in_macro() {
ptr[1] = 0;
int tmp;
ptr[2] = 30;
auto p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
int * p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", 10}"
tmp = p[5];
int val = *p;
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:""
Expand Down
18 changes: 9 additions & 9 deletions clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ void m1(int* v1, int* v2, int) {
}

void condition_check_nullptr() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand All @@ -21,7 +21,7 @@ void condition_check_nullptr() {
}

void condition_check() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand Down Expand Up @@ -53,12 +53,12 @@ void condition_check() {
}

void condition_check_two_usafe_buffers() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"

auto q = new int[10];
int* q = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> q"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand All @@ -72,7 +72,7 @@ void condition_check_two_usafe_buffers() {
}

void unsafe_method_invocation_single_param() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand All @@ -84,12 +84,12 @@ void unsafe_method_invocation_single_param() {
}

void safe_method_invocation_single_param() {
auto p = new int[10];
int* p = new int[10];
foo(p);
}

void unsafe_method_invocation_double_param() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand All @@ -111,7 +111,7 @@ void unsafe_method_invocation_double_param() {
}

void unsafe_access_in_lamda() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand All @@ -125,7 +125,7 @@ void unsafe_access_in_lamda() {
}

void fixits_in_lamda() {
auto p = new int[10];
int* p = new int[10];
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
Expand Down

0 comments on commit 3a67b91

Please sign in to comment.