ziqingluo-90 created this revision.
ziqingluo-90 added reviewers: NoQ, jkorous, t-rasmud, malavikasamak.
Herald added a project: All.
ziqingluo-90 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
- 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. Because we will give up on fixing a
pointer type variable `v`, if we cannot obtain the source range of the pointee
type of `v`. Using unnamed types is just one of the many causes of such
scenarios. Besides, in some cases, we have no problem in referring to unnamed
types. Therefore, we remove the special handling since we have a more general
solution already.
For example,
typedef struct {int x;} UNNAMED_T;
UNNAMED_T * x; // we can refer to the unnamed struct using `UNNAMED_T`
typedef struct {int x;} * UNNAMED_PTR;
UNNAMED_PTR y; // we cannot obtain the source range of the pointee type of
`y`, so we will give up
- Move tests for cv-qualified parameters and unnamed types out of the
"...-unsupported.cpp" test file.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156188
Files:
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
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
+++ 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 @@
int tmp;
tmp = x[5]; // expected-note{{used in buffer access here}}
}
+
+#define MACRO_NAME MyName
+
+// CHECK-NOT: fix-it:{{.*}}:{[[@LINE+1]]
+void macroIdentifier(int * MACRO_NAME) { // The fix-it ends with a macro. It will be discarded due to overlap with macros. \
+ 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}}
+}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
===================================================================
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp
@@ -147,18 +147,78 @@
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
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===================================================================
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1362,13 +1362,32 @@
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);
+
+ 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();
@@ -1377,15 +1396,36 @@
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.
@@ -1396,7 +1436,11 @@
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:
@@ -1713,6 +1757,34 @@
return FixIts;
}
+// 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;
+
+ StringRef SpanOpen = "std::span<";
+ StringRef SpanClose = ">";
+ std::string SpanTyText = SpanOpen.str();
+
+ SpanTyText.append(*PteTyText);
+ // Append qualifiers to span element type if any:
+ if (PteTyQualifiers) {
+ SpanTyText.append(" ");
+ SpanTyText.append(PteTyQualifiers->getAsString());
+ }
+ SpanTyText.append(SpanClose.str());
+ 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
@@ -1958,35 +2030,22 @@
if (!FD)
return {};
- std::optional<Qualifiers> PteTyQualifiers = std::nullopt;
- std::optional<std::string> PteTyText = getPointeeTypeText(
- PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers);
-
- if (!PteTyText)
- return {};
-
- std::optional<StringRef> PVDNameText = PVD->getIdentifier()->getName();
-
- if (!PVDNameText)
- 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;
@@ -1999,7 +2058,7 @@
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
