[clang] 41279e8 - [-Wunsafe-buffer-usage] Refactor and improve for parameter fix-its
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 17 15:27:57 PDT 2023
Author: Ziqing Luo
Date: 2023-08-17T15:27:38-07:00
New Revision: 41279e870fa577f8a7f7030b8e45da250d0def9e
URL: https://github.com/llvm/llvm-project/commit/41279e870fa577f8a7f7030b8e45da250d0def9e
DIFF: https://github.com/llvm/llvm-project/commit/41279e870fa577f8a7f7030b8e45da250d0def9e.diff
LOG: [-Wunsafe-buffer-usage] Refactor and improve for parameter fix-its
- Factor out the code that will be shared by both parameter and local variable fix-its
- Add a check to ensure that a TypeLoc::isNull is false before using the TypeLoc
- Remove the special check for whether a fixing variable involves unnamed types. This check is unnecessary now.
- Move tests for cv-qualified parameters and unnamed types out of the "...-unsupported.cpp" test file.
Reviewed by: NoQ (Artem Dergachev)
Differential revision: https://reviews.llvm.org/D156188
Added:
Modified:
clang/lib/Analysis/UnsafeBufferUsage.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 87087686171347..41820c80da6b4b 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1380,13 +1380,35 @@ static std::optional<StringRef> getRangeText(SourceRange SR,
return std::nullopt;
}
+// Returns the begin location of the identifier of the given variable
+// declaration.
+static SourceLocation getVarDeclIdentifierLoc(const VarDecl *VD) {
+ // According to the implementation of `VarDecl`, `VD->getLocation()` actually
+ // returns the begin location of the identifier of the declaration:
+ return VD->getLocation();
+}
+
+// Returns the literal text of the identifier of the given variable declaration.
+static std::optional<StringRef>
+getVarDeclIdentifierText(const VarDecl *VD, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ SourceLocation ParmIdentBeginLoc = getVarDeclIdentifierLoc(VD);
+ SourceLocation ParmIdentEndLoc =
+ Lexer::getLocForEndOfToken(ParmIdentBeginLoc, 0, SM, LangOpts);
+
+ if (ParmIdentEndLoc.isMacroID() &&
+ !Lexer::isAtEndOfMacroExpansion(ParmIdentEndLoc, SM, LangOpts))
+ return std::nullopt;
+ return getRangeText({ParmIdentBeginLoc, ParmIdentEndLoc}, SM, LangOpts);
+}
+
// Returns the text of the pointee type of `T` from a `VarDecl` of a pointer
// type. The text is obtained through from `TypeLoc`s. Since `TypeLoc` does not
// have source ranges of qualifiers ( The `QualifiedTypeLoc` looks hacky too me
// :( ), `Qualifiers` of the pointee type is returned separately through the
// output parameter `QualifiersToAppend`.
static std::optional<std::string>
-getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
+getPointeeTypeText(const VarDecl *VD, const SourceManager &SM,
const LangOptions &LangOpts,
std::optional<Qualifiers> *QualifiersToAppend) {
QualType Ty = VD->getType();
@@ -1395,15 +1417,36 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
assert(Ty->isPointerType() && !Ty->isFunctionPointerType() &&
"Expecting a VarDecl of type of pointer to object type");
PteTy = Ty->getPointeeType();
- if (PteTy->hasUnnamedOrLocalType())
- // If the pointee type is unnamed, we can't refer to it
+
+ TypeLoc TyLoc = VD->getTypeSourceInfo()->getTypeLoc().getUnqualifiedLoc();
+ TypeLoc PteTyLoc;
+
+ // We only deal with the cases that we know `TypeLoc::getNextTypeLoc` returns
+ // the `TypeLoc` of the pointee type:
+ switch (TyLoc.getTypeLocClass()) {
+ case TypeLoc::ConstantArray:
+ case TypeLoc::IncompleteArray:
+ case TypeLoc::VariableArray:
+ case TypeLoc::DependentSizedArray:
+ case TypeLoc::Decayed:
+ assert(isa<ParmVarDecl>(VD) && "An array type shall not be treated as a "
+ "pointer type unless it decays.");
+ PteTyLoc = TyLoc.getNextTypeLoc();
+ break;
+ case TypeLoc::Pointer:
+ PteTyLoc = TyLoc.castAs<PointerTypeLoc>().getPointeeLoc();
+ break;
+ default:
+ return std::nullopt;
+ }
+ if (PteTyLoc.isNull())
+ // Sometimes we cannot get a useful `TypeLoc` for the pointee type, e.g.,
+ // when the pointer type is `auto`.
return std::nullopt;
- TypeLoc TyLoc = VD->getTypeSourceInfo()->getTypeLoc();
- TypeLoc PteTyLoc = TyLoc.getUnqualifiedLoc().getNextTypeLoc();
- SourceLocation VDNameStartLoc = VD->getLocation();
+ SourceLocation IdentLoc = getVarDeclIdentifierLoc(VD);
- if (!(VDNameStartLoc.isValid() && PteTyLoc.getSourceRange().isValid())) {
+ if (!(IdentLoc.isValid() && PteTyLoc.getSourceRange().isValid())) {
// We are expecting these locations to be valid. But in some cases, they are
// not all valid. It is a Clang bug to me and we are not responsible for
// fixing it. So we will just give up for now when it happens.
@@ -1414,7 +1457,11 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
SourceLocation PteEndOfTokenLoc =
Lexer::getLocForEndOfToken(PteTyLoc.getEndLoc(), 0, SM, LangOpts);
- if (!SM.isBeforeInTranslationUnit(PteEndOfTokenLoc, VDNameStartLoc)) {
+ if (!PteEndOfTokenLoc.isValid())
+ // Sometimes we cannot get the end location of the pointee type, e.g., when
+ // there are macros involved.
+ return std::nullopt;
+ if (!SM.isBeforeInTranslationUnit(PteEndOfTokenLoc, IdentLoc)) {
// We only deal with the cases where the source text of the pointee type
// appears on the left-hand side of the variable identifier completely,
// including the following forms:
@@ -1739,6 +1786,32 @@ Handler.addDebugNoteForVar((D), (D)->getBeginLoc(), "failed to produce fixit for
#define DEBUG_NOTE_DECL_FAIL(D, Msg)
#endif
+// For the given variable declaration with a pointer-to-T type, returns the text
+// `std::span<T>`. If it is unable to generate the text, returns
+// `std::nullopt`.
+static std::optional<std::string> createSpanTypeForVarDecl(const VarDecl *VD,
+ const ASTContext &Ctx) {
+ assert(VD->getType()->isPointerType());
+
+ std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
+ std::optional<std::string> PteTyText = getPointeeTypeText(
+ VD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
+
+ if (!PteTyText)
+ return std::nullopt;
+
+ std::string SpanTyText = "std::span<";
+
+ SpanTyText.append(*PteTyText);
+ // Append qualifiers to span element type if any:
+ if (PteTyQualifiers) {
+ SpanTyText.append(" ");
+ SpanTyText.append(PteTyQualifiers->getAsString());
+ }
+ SpanTyText.append(">");
+ return SpanTyText;
+}
+
// 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
@@ -1995,39 +2068,22 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
return {};
}
- std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
- std::optional<std::string> PteTyText = getPointeeTypeText(
- PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
-
- if (!PteTyText) {
- DEBUG_NOTE_DECL_FAIL(PVD, " : invalid pointee type");
- return {};
- }
-
- std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
-
- if (!PVDNameText) {
- DEBUG_NOTE_DECL_FAIL(PVD, " : invalid identifier name");
- return {};
- }
-
- std::string SpanOpen = "std::span<";
- std::string SpanClose = ">";
- std::string SpanTyText;
std::stringstream SS;
+ std::optional<std::string> SpanTyText = createSpanTypeForVarDecl(PVD, Ctx);
+ std::optional<StringRef> ParmIdentText;
- SS << SpanOpen << *PteTyText;
- // Append qualifiers to span element type:
- if (PteTyQualifiers)
- SS << " " << PteTyQualifiers->getAsString();
- SS << SpanClose;
- // Save the Span type text:
- SpanTyText = SS.str();
+ if (!SpanTyText)
+ return {};
+ SS << *SpanTyText;
// Append qualifiers to the type of the parameter:
if (PVD->getType().hasQualifiers())
SS << " " << PVD->getType().getQualifiers().getAsString();
+ ParmIdentText =
+ getVarDeclIdentifierText(PVD, Ctx.getSourceManager(), Ctx.getLangOpts());
+ if (!ParmIdentText)
+ return {};
// Append parameter's name:
- SS << " " << PVDNameText->str();
+ SS << " " << ParmIdentText->str();
FixItList Fixes;
unsigned ParmIdx = 0;
@@ -2040,7 +2096,7 @@ static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx,
ParmIdx++;
}
if (ParmIdx < FD->getNumParams())
- if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText,
+ if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, *SpanTyText,
FD, Ctx, Handler)) {
Fixes.append(*OverloadFix);
return Fixes;
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
index 85210dd4d337bd..4d3fac80b804e3 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
@@ -147,18 +147,78 @@ void complexDeclarator(int * (*a[10])[10]) {
if (++a){}
}
-// Make sure we do not generate fixes for the following cases:
+// Tests parameters with cv-qualifiers
-#define MACRO_NAME MyName
+void const_ptr(int * const 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}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:16-[[@LINE-1]]:29}:"std::span<int> const x"
+ int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr(int * const x) {return const_ptr(std::span<int>(x, <# size #>));}\n"
-void macroIdentifier(int *MACRO_NAME) { // The fix-it ends with a macro. It will be discarded due to overlap with macros.
- // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]]
- if (++MyName){}
+void const_ptr_to_const(const int * const 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}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:44}:"std::span<int const> const x"
+ int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr_to_const(const int * const x) {return const_ptr_to_const(std::span<int const>(x, <# size #>));}\n"
+
+void const_volatile_ptr(int * const volatile 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}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:47}:"std::span<int> const volatile x"
+ int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_volatile_ptr(int * const volatile x) {return const_volatile_ptr(std::span<int>(x, <# size #>));}\n"
+
+void volatile_const_ptr(int * volatile const 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}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:47}:"std::span<int> const volatile x"
+ int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void volatile_const_ptr(int * volatile const x) {return volatile_const_ptr(std::span<int>(x, <# size #>));}\n"
+
+void const_volatile_ptr_to_const_volatile(const volatile int * const volatile 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}}
+ // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:43-[[@LINE-1]]:80}:"std::span<int const volatile> const volatile x"
+ int tmp = x[5]; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_volatile_ptr_to_const_volatile(const volatile int * const volatile x) {return const_volatile_ptr_to_const_volatile(std::span<int const volatile>(x, <# size #>));}\n"
+
+
+// Test if function declaration specifiers are handled correctly:
+
+static void static_f(int *p) {
+ p[5] = 5;
}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} static void static_f(int *p) {return static_f(std::span<int>(p, <# size #>));}\n"
-// CHECK-NOT: fix-it:{{.*}}:
-void parmHasNoName(int *p, int *) { // cannot fix the function because there is one parameter has no name.
+static inline void static_inline_f(int *p) {
p[5] = 5;
}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} static inline void static_inline_f(int *p) {return static_inline_f(std::span<int>(p, <# size #>));}\n"
+
+inline void static static_inline_f2(int *p) {
+ p[5] = 5;
+}
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} inline void static static_inline_f2(int *p) {return static_inline_f2(std::span<int>(p, <# size #>));}\n"
+
+
+// Test when unnamed types are involved:
+
+typedef struct {int x;} UNNAMED_STRUCT;
+struct {int x;} VarOfUnnamedType;
+
+void useUnnamedType(UNNAMED_STRUCT * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:21-[[@LINE-2]]:39}:"std::span<UNNAMED_STRUCT> p"
+ if (++p) { // expected-note{{used in pointer arithmetic here}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+ }
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void useUnnamedType(UNNAMED_STRUCT * p) {return useUnnamedType(std::span<UNNAMED_STRUCT>(p, <# size #>));}\n"
+
+void useUnnamedType2(decltype(VarOfUnnamedType) * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} \
+ expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:22-[[@LINE-2]]:52}:"std::span<decltype(VarOfUnnamedType)> p"
+ if (++p) { // expected-note{{used in pointer arithmetic here}}
+ // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
+ }
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void useUnnamedType2(decltype(VarOfUnnamedType) * p) {return useUnnamedType2(std::span<decltype(VarOfUnnamedType)>(p, <# size #>));}\n"
#endif
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
index 44f44d0e077e11..71350098613d19 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
@@ -1,34 +1,21 @@
// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fcxx-exceptions -fsafe-buffer-usage-suggestions -verify %s
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fcxx-exceptions -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions %s 2>&1 | FileCheck %s
-void const_ptr(int * const 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}}
- // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:16-[[@LINE-1]]:29}:"std::span<int> const x"
- int tmp = x[5]; // expected-note{{used in buffer access here}}
-}
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr(int * const x) {return const_ptr(std::span<int>(x, <# size #>));}\n"
-
-void const_ptr_to_const(const int * const 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}}
- // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:44}:"std::span<int const> const x"
- int tmp = x[5]; // expected-note{{used in buffer access here}}
-}
-// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void const_ptr_to_const(const int * const x) {return const_ptr_to_const(std::span<int const>(x, <# size #>));}\n"
+typedef int * TYPEDEF_PTR;
+#define MACRO_PTR int*
-typedef struct {int x;} NAMED_UNNAMED_STRUCT; // an unnamed struct type named by a typedef
-typedef struct {int x;} * PTR_TO_ANON; // pointer to an unnamed struct
-typedef NAMED_UNNAMED_STRUCT * PTR_TO_NAMED; // pointer to a named type
+// We CANNOT fix a pointer whose type is defined in a typedef or a
+// macro. Because if the typedef is changed after the fix, the fix
+// becomes incorrect and may not be noticed.
-// We can fix a pointer to a named type
-void namedPointeeType(NAMED_UNNAMED_STRUCT * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}\ expected-note{{change type of 'p' to 'std::span' to preserve bounds information}}
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:23-[[@LINE-1]]:47}:"std::span<NAMED_UNNAMED_STRUCT> p"
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+1]]
+void typedefPointer(TYPEDEF_PTR p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
if (++p) { // expected-note{{used in pointer arithmetic here}}
- // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:10}:"(p = p.subspan(1)).data()"
}
}
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void namedPointeeType(NAMED_UNNAMED_STRUCT * p) {return namedPointeeType(std::span<NAMED_UNNAMED_STRUCT>(p, <# size #>));}\n"
-// We CANNOT fix a pointer to an unnamed type
-// CHECK-NOT: fix-it:
-void unnamedPointeeType(PTR_TO_ANON p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE+1]]
+void macroPointer(MACRO_PTR p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
if (++p) { // expected-note{{used in pointer arithmetic here}}
}
}
@@ -148,3 +135,17 @@ void parmWithDefaultValueDecl(int * x) {
int tmp;
tmp = x[5]; // expected-note{{used in buffer access here}}
}
+
+#define MACRO_NAME MyName
+
+// The fix-it ends with a macro. It will be discarded due to overlap with macros.
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void macroIdentifier(int * MACRO_NAME) { // expected-warning{{'MyName' is an unsafe pointer used for buffer access}}
+ if (++MyName){} // expected-note{{used in pointer arithmetic here}}
+}
+
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void parmHasNoName(int *p, int *) { // cannot fix the function because there is one parameter has no name. \
+ expected-warning{{'p' is an unsafe pointer used for buffer access}}
+ p[5] = 5; // expected-note{{used in buffer access here}}
+}
More information about the cfe-commits
mailing list