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 @@ -1224,6 +1224,12 @@ return std::nullopt; // something went wrong, no fix-it } +// FIXME: this function should be customizable through format +static StringRef getEndOfLine() { + static const char *const EOL = "\n"; + return EOL; +} + // Return the text representation of the given `APInt Val`: static std::string getAPIntText(APInt Val) { SmallVector Txt; @@ -1275,6 +1281,83 @@ return std::nullopt; } +// Returns the literal text in `SourceRange SR`, if `SR` is a valid range. +static std::optional getRangeText(SourceRange SR, + const SourceManager &SM, + const LangOptions &LangOpts) { + bool Invalid = false; + CharSourceRange CSR = CharSourceRange::getCharRange(SR.getBegin(), SR.getEnd()); + StringRef Text = Lexer::getSourceText(CSR, SM, LangOpts, &Invalid); + + if (!Invalid) + return Text; + return std::nullopt; +} + +// 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, + const LangOptions &LangOpts, + std::optional *QualifiersToAppend) { + QualType Ty = VD->getType(); + QualType PteTy; + + 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 + return std::nullopt; + + TypeLoc TyLoc = VD->getTypeSourceInfo()->getTypeLoc(); + TypeLoc PteTyLoc = TyLoc.getUnqualifiedLoc().getNextTypeLoc(); + SourceLocation VDNameStartLoc = VD->getLocation(); + + if (!SM.isBeforeInTranslationUnit(PteTyLoc.getSourceRange().getEnd(), + VDNameStartLoc)) { + // 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: + // `T ident`, + // `T ident[]`, where `T` is any type. + // Examples of excluded cases are `T (*ident)[]` or `T ident[][n]`. + return std::nullopt; + } + if (PteTy.hasQualifiers()) { + // TypeLoc does not provide source ranges for qualifiers (it says it's + // intentional but seems fishy to me), so we cannot get the full text + // `PteTy` via source ranges. + *QualifiersToAppend = PteTy.getQualifiers(); + } + + // Note that TypeLoc.getEndLoc() returns the begin location of the last token: + SourceRange CSR{ + PteTyLoc.getBeginLoc(), + Lexer::getLocForEndOfToken(PteTyLoc.getEndLoc(), 0, SM, LangOpts)}; + + return getRangeText(CSR, SM, LangOpts)->str(); +} + +// Returns the text of the name (with qualifiers) of a `FunctionDecl`. +static std::optional getFunNameText(const FunctionDecl *FD, + const SourceManager &SM, + const LangOptions &LangOpts) { + SourceLocation BeginLoc = FD->getQualifier() + ? FD->getQualifierLoc().getBeginLoc() + : FD->getNameInfo().getBeginLoc(); + // Note that `FD->getNameInfo().getEndLoc()` returns the begin location of the + // last token: + SourceLocation EndLoc = Lexer::getLocForEndOfToken( + FD->getNameInfo().getEndLoc(), 0, SM, LangOpts); + SourceRange NameRange{BeginLoc, EndLoc}; + + return getRangeText(NameRange, SM, LangOpts); +} + std::optional DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const { const VarDecl *VD = dyn_cast(BaseDeclRefExpr->getDecl()); @@ -1609,6 +1692,252 @@ return FixIts; } +static bool hasConflictingOverload(const FunctionDecl *FD) { + return !FD->getDeclContext()->lookup(FD->getDeclName()).isSingleResult(); +} + +// Returns the text representation of clang::unsafe_buffer_usage attribute. +static std::string getUnsafeBufferUsageAttributeText() { + static const char *const RawAttr = "[[clang::unsafe_buffer_usage]]"; + std::stringstream SS; + SS << RawAttr << getEndOfLine().str(); + return SS.str(); +} + +// For a `FunDecl`, one of whose `ParmVarDecl`s is being changed to have a new +// type, this function produces fix-its to make the change self-contained. Let +// 'F' be the entity defined by the original `FunDecl` and "NewF" be the entity +// defined by the `FunDecl` after the change to the parameter. Fix-its produced +// by this function are +// 1. Add the `[[clang::unsafe_buffer_usage]]` attribute to each declaration +// of 'F'; +// 2. Create a declaration of "NewF" next to each declaration of `F`; +// 3. Create a definition of "F" (as its' original definition is now belongs +// to "NewF") next to its original definition. The body of the creating +// definition calls to "NewF". +// +// Example: +// +// void f(int *p); // original declaration +// void f(int *p) { // original definition +// p[5]; +// } +// +// To change the parameter `p` to be of `std::span` type, we +// also add overloads: +// +// [[clang::unsafe_buffer_usage]] void f(int *p); // original decl +// void f(std::span p); // added overload decl +// void f(std::span p) { // original def where param is changed +// p[5]; +// } +// [[clang::unsafe_buffer_usage]] void f(int *p) { // added def +// 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 +// +// `NewTypeText` is the string representation of the new type, to which the +// parameter indexed by `ParmIdx` is being changed. +static std::optional +createOverloadsForFixedParams(unsigned ParmIdx, StringRef NewTyText, + const FunctionDecl *FD, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + // FIXME: need to make this conflict checking better: + if (hasConflictingOverload(FD)) + return std::nullopt; + + const SourceManager &SM = Ctx.getSourceManager(); + const LangOptions &LangOpts = Ctx.getLangOpts(); + // FIXME Respect indentation of the original code. + + // A lambda that creates the text representation of a function declaration + // with the new type signature: + const auto NewOverloadSignatureCreator = + [&SM, &LangOpts](const FunctionDecl *FD, unsigned ParmIdx, + StringRef NewTypeText) -> std::optional { + std::stringstream SS; + + SS << ";"; + SS << getEndOfLine().str(); + // Append: ret-type func-name "(" + if (auto Prefix = getRangeText( + SourceRange(FD->getBeginLoc(), (*FD->param_begin())->getBeginLoc()), + SM, LangOpts)) + SS << Prefix->str(); + else + return std::nullopt; // give up + // Append: parameter-type-list + const unsigned NumParms = FD->getNumParams(); + + for (unsigned i = 0; i < NumParms; i++) { + const ParmVarDecl *Parm = FD->getParamDecl(i); + + if (Parm->isImplicit()) + continue; + if (i == ParmIdx) { + SS << NewTypeText.str(); + // print parameter name if provided: + if (IdentifierInfo * II = Parm->getIdentifier()) + SS << " " << II->getName().str(); + } else if (auto ParmTypeText = + getRangeText(Parm->getSourceRange(), SM, LangOpts)) { + // print the whole `Parm` without modification: + SS << ParmTypeText->str(); + } else + return std::nullopt; // something wrong, give up + if (i != NumParms - 1) + SS << ", "; + } + SS << ")"; + return SS.str(); + }; + + // A lambda that creates the text representation of a function definition with + // the original signature: + const auto OldOverloadDefCreator = + [&Handler, &SM, + &LangOpts](const FunctionDecl *FD, unsigned ParmIdx, + StringRef NewTypeText) -> std::optional { + std::stringstream SS; + + SS << getEndOfLine().str(); + // Append: attr-name ret-type func-name "(" param-list ")" "{" + if (auto FDPrefix = getRangeText( + SourceRange(FD->getBeginLoc(), FD->getBody()->getBeginLoc()), SM, + LangOpts)) + SS << getUnsafeBufferUsageAttributeText() << FDPrefix->str() << "{"; + else + return std::nullopt; + // Append: "return" func-name "(" + if (auto FunQualName = getFunNameText(FD, SM, LangOpts)) + SS << "return " << FunQualName->str() << "("; + else + return std::nullopt; + + // Append: arg-list + const unsigned NumParms = FD->getNumParams(); + for (unsigned i = 0; i < NumParms; i++) { + const ParmVarDecl *Parm = FD->getParamDecl(i); + + if (Parm->isImplicit()) + continue; + assert(Parm->getIdentifier() && + "A parameter of a function definition has no name"); + if (i == ParmIdx) + // This is our spanified paramter! + SS << NewTypeText.str() << "(" << Parm->getIdentifier()->getName().str() << ", " + << Handler.getUserFillPlaceHolder("size") << ")"; + else + SS << Parm->getIdentifier()->getName().str(); + if (i != NumParms - 1) + SS << ", "; + } + // finish call and the body + SS << ");}" << getEndOfLine().str(); + // FIXME: 80-char line formatting? + return SS.str(); + }; + + FixItList FixIts{}; + for (FunctionDecl *FReDecl : FD->redecls()) { + std::optional Loc = getPastLoc(FReDecl, SM, LangOpts); + + if (!Loc) + return {}; + if (FReDecl->isThisDeclarationADefinition()) { + assert(FReDecl == FD && "inconsistent function definition"); + // Inserts a definition with the old signature to the end of + // `FReDecl`: + if (auto OldOverloadDef = + OldOverloadDefCreator(FReDecl, ParmIdx, NewTyText)) + FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *OldOverloadDef)); + else + return {}; // give up + } else { + // Adds the unsafe-buffer attribute (if not already there) to `FReDecl`: + if (!FReDecl->hasAttr()) { + FixIts.emplace_back(FixItHint::CreateInsertion( + FReDecl->getBeginLoc(), getUnsafeBufferUsageAttributeText())); + } + // Inserts a declaration with the new signature to the end of `FReDecl`: + if (auto NewOverloadDecl = + NewOverloadSignatureCreator(FReDecl, ParmIdx, NewTyText)) + FixIts.emplace_back(FixItHint::CreateInsertion(*Loc, *NewOverloadDecl)); + else + return {}; + } + } + return FixIts; +} + +// To fix a `ParmVarDecl` to be of `std::span` type. In addition, creating a +// new overload of the function so that the change is self-contained (see +// `createOverloadsForFixedParams`). +static FixItList fixParamWithSpan(const ParmVarDecl *PVD, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + if (PVD->hasDefaultArg()) + // FIXME: generate fix-its for default values: + return {}; + assert(PVD->getType()->isPointerType()); + auto *FD = dyn_cast(PVD->getDeclContext()); + + if (!FD) + return {}; + + std::optional PteTyQualifiers = std::nullopt; + std::optional PteTyText = getPointeeTypeText( + PVD, Ctx.getSourceManager(), Ctx.getLangOpts(), &PteTyQualifiers); + + if (!PteTyText) + return {}; + + std::optional PVDNameText = PVD->getIdentifier()->getName(); + + if (!PVDNameText) + return {}; + + std::string SpanOpen = "std::span<"; + std::string SpanClose = ">"; + std::string SpanTyText; + std::stringstream SS; + + SS << SpanOpen << *PteTyText; + // Append qualifiers to span element type: + if (PteTyQualifiers) + SS << " " << PteTyQualifiers->getAsString(); + SS << SpanClose; + // Save the Span type text: + SpanTyText = SS.str(); + // Append qualifiers to the type of the parameter: + if (PVD->getType().hasQualifiers()) + SS << " " << PVD->getType().getQualifiers().getAsString(); + // Append parameter's name: + SS << " " << PVDNameText->str(); + + FixItList Fixes; + unsigned ParmIdx = 0; + + Fixes.push_back( + FixItHint::CreateReplacement(PVD->getSourceRange(), SS.str())); + for (auto *ParmIter : FD->parameters()) { + if (PVD == ParmIter) + break; + ParmIdx++; + } + if (ParmIdx < FD->getNumParams()) + if (auto OverloadFix = createOverloadsForFixedParams(ParmIdx, SpanTyText, + FD, Ctx, Handler)) { + Fixes.append(*OverloadFix); + return Fixes; + } + return {}; +} + static FixItList fixVariableWithSpan(const VarDecl *VD, const DeclUseTracker &Tracker, const ASTContext &Ctx, @@ -1628,14 +1957,44 @@ return fixVarDeclWithSpan(VD, Ctx, Handler.getUserFillPlaceHolder()); } -static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K, - const DeclUseTracker &Tracker, - const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler) { +// TODO: we should be consistent to use `std::nullopt` to represent no-fix due +// to any unexpected problem. +static FixItList +fixVariable(const VarDecl *VD, Strategy::Kind K, + /* The function decl under analysis */ const Decl *D, + const DeclUseTracker &Tracker, const ASTContext &Ctx, + UnsafeBufferUsageHandler &Handler) { + if (const auto *PVD = dyn_cast(VD)) { + auto *FD = dyn_cast(PVD->getDeclContext()); + if (!FD || FD != D) + // `FD != D` means that `PVD` belongs to a function that is not being + // analyzed currently. Thus `FD` may not be complete. + return {}; + + // TODO If function has a try block we can't change params unless we check + // also its catch block for their use. + // FIXME We might support static class methods, some select methods, + // operators and possibly lamdas. + if (FD->isMain() || FD->isConstexpr() || + FD->getTemplatedKind() != FunctionDecl::TemplatedKind::TK_NonTemplate || + FD->isVariadic() || + // also covers call-operator of lamdas + isa(FD) || + // skip when the function body is a try-block + (FD->hasBody() && isa(FD->getBody())) || + FD->isOverloadedOperator()) + return {}; // TODO test all these cases + } + switch (K) { case Strategy::Kind::Span: { - if (VD->getType()->isPointerType() && VD->isLocalVarDecl()) - return fixVariableWithSpan(VD, Tracker, Ctx, Handler); + if (VD->getType()->isPointerType()) { + if (const auto *PVD = dyn_cast(VD)) + return fixParamWithSpan(PVD, Ctx, Handler); + + if (VD->isLocalVarDecl()) + return fixVariableWithSpan(VD, Tracker, Ctx, Handler); + } return {}; } case Strategy::Kind::Iterator: @@ -1676,14 +2035,14 @@ static std::map getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, - const DeclUseTracker &Tracker, const ASTContext &Ctx, - UnsafeBufferUsageHandler &Handler, - const DefMapTy &VarGrpMap) { + const ASTContext &Ctx, + /* The function decl under analysis */ const Decl *D, + const DeclUseTracker &Tracker, UnsafeBufferUsageHandler &Handler, + const DefMapTy &VarGrpMap) { std::map FixItsForVariable; for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { - const Strategy::Kind ReplacementTypeForVD = S.lookup(VD); FixItsForVariable[VD] = - fixVariable(VD, ReplacementTypeForVD, Tracker, Ctx, Handler); + fixVariable(VD, S.lookup(VD), D, Tracker, Ctx, Handler); // If we fail to produce Fix-It for the declaration we have to skip the // variable entirely. if (FixItsForVariable[VD].empty()) { @@ -1752,8 +2111,8 @@ FixItList GroupFix; if (FixItsForVariable.find(Var) == FixItsForVariable.end()) { - GroupFix = fixVariable(Var, ReplacementTypeForVD, Tracker, - Var->getASTContext(), Handler); + GroupFix = fixVariable(Var, ReplacementTypeForVD, D, + Tracker, Var->getASTContext(), Handler); } else { GroupFix = FixItsForVariable[Var]; } @@ -1812,8 +2171,9 @@ // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. for (auto it = FixablesForAllVars.byVar.cbegin(); it != FixablesForAllVars.byVar.cend();) { - // FIXME: Support ParmVarDecl as well. - if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) { + // FIXME: need to deal with global variables later + if ((!it->first->isLocalVarDecl() && !isa(it->first)) || + Tracker.hasUnclaimedUses(it->first)) { it = FixablesForAllVars.byVar.erase(it); } else { ++it; @@ -1909,8 +2269,10 @@ } Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); - FixItsForVariableGroup = getFixIts(FixablesForAllVars, NaiveStrategy, Tracker, - D->getASTContext(), Handler, VariableGroupsMap); + + FixItsForVariableGroup = + getFixIts(FixablesForAllVars, NaiveStrategy, D->getASTContext(), D, + Tracker, Handler, VariableGroupsMap); // FIXME Detect overlapping FixIts. diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-main.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-main.cpp new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-main.cpp @@ -0,0 +1,13 @@ +// 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 + +// We do not fix parameters of the `main` function + +// CHECK-NOT: fix-it: + +// main function +int main(int argc, char *argv[]) { // expected-warning{{'argv' is an unsafe pointer used for buffer access}} + char tmp; + tmp = argv[5][5]; // expected-note{{used in buffer access here}} \ + expected-warning{{unsafe buffer access}} +} diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-overload.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-overload.cpp new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-overload.cpp @@ -0,0 +1,112 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions -include %s %s 2>&1 | FileCheck %s + +// We cannot deal with overload conflicts for now so NO fix-it to +// function parameters will be emitted if there are overloads for that +// function. + +#ifndef INCLUDE_ME +#define INCLUDE_ME + +void baz(); + +#else + + +void foo(int *p, int * q); + +void foo(int *p); + +void foo(int *p) { + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int tmp; + tmp = p[5]; +} + +// an overload declaration of `bar(int *)` appears after it +void bar(int *p) { + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int tmp; + tmp = p[5]; +} + +void bar(); + +// an overload declaration of `baz(int)` appears is included +void baz(int *p) { + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int tmp; + tmp = p[5]; +} + +namespace NS { + // `NS::foo` is a distinct function from `foo`, so it has no + // overload and shall be fixed. + void foo(int *p) { + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:12-[[@LINE-1]]:18}:"std::span p" + int tmp; + tmp = p[5]; + } + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid foo(int *p) {return foo(std::span(p, <# size #>));}\n" + + // Similarly, `NS::bar` is distinct from `bar`: + void bar(int *p); + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:19}:";\nvoid bar(std::span p)" +} // end of namespace NS + +// This is the implementation of `NS::bar`, which shall be fixed. +void NS::bar(int *p) { + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:14-[[@LINE-1]]:20}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS::bar(int *p) {return NS::bar(std::span(p, <# size #>));}\n" + +namespace NESTED { + void alpha(int); + void beta(int *, int *); + + namespace INNER { + // `NESTED::INNER::alpha` is distinct from `NESTED::alpha`, so it + // has no overload and shall be fixed. + void alpha(int *p) { + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:16-[[@LINE-1]]:22}:"std::span p" + int tmp; + tmp = p[5]; + } + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:6-[[@LINE-1]]:6}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid alpha(int *p) {return alpha(std::span(p, <# size #>));}\n" + } +} + +namespace NESTED { + // There is an `NESTED::beta(int *, int *)` declared above, so this + // unsafe function will not be fixed. + void beta(int *p) { + // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]: + int tmp; + tmp = p[5]; + } + + namespace INNER { + void delta(int); // No fix for `NESTED::INNER::delta` + void delta(int*); + } +} + +// There is an `NESTED::beta(int *)` declared above, so this unsafe +// function will not be fixed. +void NESTED::beta(int *p, int *q) { + // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]: + int tmp; + tmp = p[5]; + tmp = q[5]; +} + +void NESTED::INNER::delta(int * p) { + // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]: + int tmp; + tmp = p[5]; +} + + +#endif diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-qualified-names.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-qualified-names.cpp new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span-qualified-names.cpp @@ -0,0 +1,48 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions %s 2>&1 | FileCheck %s + +namespace NS1 { + void foo(int *); + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:3}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" + // CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:18-[[@LINE-2]]:18}:";\nvoid foo(std::span)" + namespace NS2 { + void foo(int *); + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:5-[[@LINE-1]]:5}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" + // CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:20-[[@LINE-2]]:20}:";\nvoid foo(std::span)" + namespace NS3 { + void foo(int *); + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:7-[[@LINE-1]]:7}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" + // CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-2]]:22-[[@LINE-2]]:22}:";\nvoid foo(std::span)" + } + } + + typedef int MyType; +} + +void NS1::foo(int *p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:15-[[@LINE-1]]:21}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::foo(int *p) {return NS1::foo(std::span(p, <# size #>));}\n" + +void NS1::NS2::foo(int *p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:20-[[@LINE-1]]:26}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::NS2::foo(int *p) {return NS1::NS2::foo(std::span(p, <# size #>));}\n" + +void NS1::NS2::NS3::foo(int *p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:31}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid NS1::NS2::NS3::foo(int *p) {return NS1::NS2::NS3::foo(std::span(p, <# size #>));}\n" + + +void f(NS1::MyType * x) { + // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:8-[[@LINE-1]]:23}:"std::span x" + NS1::MyType tmp; + tmp = x[5]; +} +// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid f(NS1::MyType * x) {return f(std::span(x, <# size #>));}\n" 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 new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp @@ -0,0 +1,159 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits -fsafe-buffer-usage-suggestions -include %s %s 2>&1 | FileCheck %s + +// TODO test if there's not a single character in the file after a decl or def + +#ifndef INCLUDE_ME +#define INCLUDE_ME + +void simple(int *p); +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:20-[[@LINE-2]]:20}:";\nvoid simple(std::span p)" + +#else + +void simple(int *); +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:19-[[@LINE-2]]:19}:";\nvoid simple(std::span)" + +void simple(int *p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:13-[[@LINE-1]]:19}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid simple(int *p) {return simple(std::span(p, <# size #>));}\n" + + +void twoParms(int *p, int * q) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:15-[[@LINE-1]]:21}:"std::span p" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:23-[[@LINE-2]]:30}:"std::span q" + int tmp; + tmp = p[5] + q[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(std::span(p, <# size #>), q);}\n" +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:2-[[@LINE-2]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid twoParms(int *p, int * q) {return twoParms(p, std::span(q, <# size #>));}\n" + +void ptrToConst(const int * x) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:17-[[@LINE-1]]:30}:"std::span x" + int tmp = x[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid ptrToConst(const int * x) {return ptrToConst(std::span(x, <# size #>));}\n" + +// The followings test cases where multiple FileIDs maybe involved +// when the analyzer loads characters from source files. + +#define FUN_NAME(x) _##x##_ + +// The analyzer reads `void FUNNAME(macro_defined_name)(` from the +// source file. The MACRO and this source file have different +// FileIDs. +void FUN_NAME(macro_defined_name)(int * x) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:35-[[@LINE-1]]:42}:"std::span x" + int tmp = x[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid FUN_NAME(macro_defined_name)(int * x) {return FUN_NAME(macro_defined_name)(std::span(x, <# size #>));}\n" + + +// The followings test various type specifiers +namespace { + void simpleSpecifier(unsigned long long int *p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:24-[[@LINE-1]]:49}:"std::span p" + auto tmp = p[5]; + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid simpleSpecifier(unsigned long long int *p) {return simpleSpecifier(std::span(p, <# size #>));}\n" + + void attrParm([[maybe_unused]] int * p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:34-[[@LINE-1]]:41}:"std::span p" + int tmp = p[5]; + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid attrParm({{\[}}{{\[}}maybe_unused{{\]}}{{\]}} int * p) {return attrParm(std::span(p, <# size #>));}\n" + + using T = unsigned long long int; + + void usingTypenameSpecifier(T * p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:31-[[@LINE-1]]:36}:"std::span p" + int tmp = p[5]; + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid usingTypenameSpecifier(T * p) {return usingTypenameSpecifier(std::span(p, <# size #>));}\n" + + typedef unsigned long long int T2; + + void typedefSpecifier(T2 * p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:25-[[@LINE-1]]:31}:"std::span p" + int tmp = p[5]; + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid typedefSpecifier(T2 * p) {return typedefSpecifier(std::span(p, <# size #>));}\n" + + class SomeClass { + } C; + + void classTypeSpecifier(const class SomeClass * p) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:27-[[@LINE-1]]:52}:"std::span p" + if (++p) {} + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid classTypeSpecifier(const class SomeClass * p) {return classTypeSpecifier(std::span(p, <# size #>));}\n" + + struct { + // anon + } ANON_S; + + struct MyStruct { + // namned + } NAMED_S; + + // FIXME: `decltype(ANON_S)` represents an unnamed type but it can + // be referred as "`decltype(ANON_S)`", so the analysis should + // fix-it. + void decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r, + decltype(NAMED_S) ** rr) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:26-[[@LINE-2]]:41}:"std::span p" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:65-[[@LINE-3]]:86}:"std::span r" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:26-[[@LINE-3]]:49}:"std::span rr" + if (++p) {} + if (++q) {} + if (++r) {} + if (++rr) {} + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(std::span(p, <# size #>), q, r, rr);}\n + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, std::span(r, <# size #>), rr);}\n" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-3]]:4-[[@LINE-3]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decltypeSpecifier(decltype(C) * p, decltype(ANON_S) * q, decltype(NAMED_S) * r,\n{{.*}}decltype(NAMED_S) ** rr) {return decltypeSpecifier(p, q, r, std::span(rr, <# size #>));}\n" + +#define MACRO_TYPE(T) long T + + void macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:18-[[@LINE-1]]:46}:"std::span p" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:48-[[@LINE-2]]:77}:"std::span q" + int tmp = p[5]; + tmp = q[5]; + } + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:4-[[@LINE-1]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(std::span(p, <# size #>), q);}\n" + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:4-[[@LINE-2]]:4}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid macroType(unsigned MACRO_TYPE(int) * p, unsigned MACRO_TYPE(long) * q) {return macroType(p, std::span(q, <# size #>));}\n" +} + +// The followings test various declarators: +void decayedArray(int a[]) { + // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:19-[[@LINE-1]]:26}:"std::span a" + int tmp; + tmp = a[5]; +} +// CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\nvoid decayedArray(int a[]) {return decayedArray(std::span(a, <# size #>));}\n" + +void decayedArrayOfArray(int a[10][10]) { + // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + if (++a){} +} + +void complexDeclarator(int * (*a[10])[10]) { + // CHECK-NOT: fix-it:{{.*}}:{[[@LINE-1]] + if (++a){} +} + +// Make sure we do not generate fixes for the following cases: + +#define MACRO_NAME MyName + +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){} +} + +#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 new file mode 100644 --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp @@ -0,0 +1,131 @@ +// 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{{\]}}{{\]}}\nvoid 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{{\]}}{{\]}}\nvoid const_ptr_to_const(const int * const x) {return const_ptr_to_const(std::span(x, <# size #>));}\n" + +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 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" + 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{{\]}}{{\]}}\nvoid 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}} + if (++p) { // expected-note{{used in pointer arithmetic here}} + } +} + +// We do not fix parameters participating unsafe operations for the +// following functions/methods or function-like expressions: + +// CHECK-NOT: fix-it: +class A { + // constructor & descructor + A(int * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} + } + + // class member methods + void foo(int *p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} + } + + // overload operator + int operator+(int * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} + return tmp; + } +}; + +// lambdas +void foo() { + auto Lamb = [&](int *p) // expected-warning{{'p' is an unsafe pointer used for buffer access}} + -> int { + int tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} + return tmp; + }; +} + +// template +template +void template_foo(T * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + T tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} +} + +void instantiate_template_foo() { + int * p; + template_foo(p); // FIXME expected note {{in instantiation of function template specialization 'template_foo' requested here}} +} + +// variadic function +void vararg_foo(int * p...) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + int tmp; + tmp = p[5]; // expected-note{{used in buffer access here}} +} + +// constexpr functions +constexpr int constexpr_foo(int * p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}} + return p[5]; // expected-note{{used in buffer access here}} +} + +// function body is a try-block +void fn_with_try_block(int* p) // expected-warning{{'p' is an unsafe pointer used for buffer access}} + try { + int tmp; + + if (p == nullptr) + throw 42; + tmp = p[5]; // expected-note{{used in buffer access here}} + } + catch (int) { + *p = 0; + } + +// The following two unsupported cases are not specific to +// parm-fixits. Adding them here in case they get forgotten. +void isArrayDecayToPointerUPC(int a[][10], int (*b)[10]) { +// expected-warning@-1{{'a' is an unsafe pointer used for buffer access}} +// expected-warning@-2{{'b' is an unsafe pointer used for buffer access}} + int tmp; + + tmp = a[5][5] + b[5][5]; // expected-warning2{{unsafe buffer access}} expected-note2{{used in buffer access here}} +} + +// parameter having default values: +void parmWithDefaultValue(int * x = 0) { + // expected-warning@-1{{'x' is an unsafe pointer used for buffer access}} + int tmp; + tmp = x[5]; // expected-note{{used in buffer access here}} +} + +void parmWithDefaultValueDecl(int * x = 0); + +void parmWithDefaultValueDecl(int * x) { + // expected-warning@-1{{'x' is an unsafe pointer used for buffer access}} + int tmp; + tmp = x[5]; // expected-note{{used in buffer access here}} +} + diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -36,7 +36,7 @@ void * voidPtrCall(void); char * charPtrCall(void); -void testArraySubscripts(int *p, int **pp) { +void testArraySubscripts(int *p, int **pp) { // expected-note{{change type of 'pp' to 'std::span' to preserve bounds information}} // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}} foo(p[1], // expected-note{{used in buffer access here}} @@ -104,6 +104,12 @@ foo(ap4[1]); // expected-note{{used in buffer access here}} } +void testUnevaluatedContext(int * p) {// no-warning + foo(sizeof(p[1]), // no-warning + sizeof(decltype(p[1]))); // no-warning +} + +// expected-note@+1{{change type of 'a' to 'std::span' to preserve bounds information}} void testQualifiedParameters(const int * p, const int * const q, const int a[10], const int b[10][10]) { // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} // expected-warning@-2{{'q' is an unsafe pointer used for buffer access}} @@ -319,7 +325,7 @@ template void fArr(int t[]); // FIXME: expected note {{in instantiation of}} -int testReturn(int t[]) { +int testReturn(int t[]) {// expected-note{{change type of 't' to 'std::span' to preserve bounds information}} // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}} return t[1]; // expected-note{{used in buffer access here}} }