diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1380,13 +1380,35 @@ 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 +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 -getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM, +getPointeeTypeText(const VarDecl *VD, const SourceManager &SM, const LangOptions &LangOpts, std::optional *QualifiersToAppend) { QualType Ty = VD->getType(); @@ -1395,15 +1417,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(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().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 @@ 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 @@ #define DEBUG_NOTE_DECL_FAIL(D, Msg) #endif +// For the given variable declaration with a pointer-to-T type, returns the text +// `std::span`. If it is unable to generate the text, returns +// `std::nullopt`. +static std::optional createSpanTypeForVarDecl(const VarDecl *VD, + const ASTContext &Ctx) { + assert(VD->getType()->isPointerType()); + + std::optional PteTyQualifiers = std::nullopt; + std::optional 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` type and transforms the initializer, if present, to a span @@ -1995,39 +2068,22 @@ return {}; } - std::optional PteTyQualifiers = std::nullopt; - std::optional PteTyText = getPointeeTypeText( - PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers); - - if (!PteTyText) { - DEBUG_NOTE_DECL_FAIL(PVD, " : invalid pointee type"); - return {}; - } - - std::optional 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 SpanTyText = createSpanTypeForVarDecl(PVD, Ctx); + std::optional 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 @@ 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 --- 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 @@ 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 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(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 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(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 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(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 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(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 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(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(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(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(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 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(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 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(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 --- 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 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(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 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(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 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(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 + +// 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}} +}