Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -8,6 +8,7 @@ #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Type.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/SmallVector.h" @@ -864,6 +865,147 @@ return FixIts; } +static FixItList fixParamWithSpan(const ParmVarDecl *PVD, + const ASTContext &Ctx) { + assert(PVD->getType()->isPointerType() && "Only PointerType ParmVarDecl can be changed to std::span."); + FixItList FixIts{}; + auto * FD = dyn_cast(PVD->getDeclContext()); + if (!FD) + return {}; + + // TODO It looks like testLambdaCaptureAndGlobal in warn-unsafe-buffer-usage.cpp doesn't have a body and we crash below. + if (!FD->getBody()) + return {}; + + // FIXME Respect indentation of the original code. + + // TODO don't hardcode line delimiter - follow what the source file uses + // example: + // void foo(int* ptr) { ... } + // goal: + // [[clang::unsafe_buffer_usage]] void foo(int* ptr) { std::span(ptr, <# size #>) } + // void foo(int* ptr) { ... } + + const SourceManager &SM = Ctx.getSourceManager(); + + // TODO desugaring might be undesirable in some cases + // e.g. std::string -> std::basic_string<...> + // Maybe this is predominantly specific to std? + const std::string newSpanTypeSpelling = "std::span<" + PVD->getType().getDesugaredType(Ctx)->getPointeeType().getAsString() + ">"; + + const std::string compatOverloadNameAndSignature = [&]() -> std::string { + std::string result = + "[[clang::unsafe_buffer_usage]] " + + std::string( + SM.getCharacterData(FD->getBeginLoc()), + SM.getCharacterData(FD->getBody()->getBeginLoc()) + ); + while(!result.empty()) { + if (isHorizontalWhitespace(result.back())) + result.pop_back(); + else + break; + } + return result; + }(); + + const std::string compatOverloadDef = [&]() -> std::string { + // create compat overload definition + std::string compatOverloadDef = + compatOverloadNameAndSignature + "\n" + + "{\n" + + " return " + FD->getNameInfo().getAsString() + "("; + for (ParmVarDecl * Param : FD->parameters()) { + if (Param->isImplicit()) + return std::string{}; // TODO error handling + + // FIXME: if a param doesn't have a name - add a name in the signature of compat overload and use it here to pass to the original overload + if (Param->getNameAsString().empty()) + return std::string{}; // TODO error handling + + if (Param == PVD) { + // This is our spanified paramter! + // TODO don't hardcode the placeholder + compatOverloadDef += newSpanTypeSpelling + "("+Param->getNameAsString() + ", <# size #>)"; + } else { + compatOverloadDef += Param->getNameAsString(); + } + + compatOverloadDef += ", "; + } + // remove the last ", " + compatOverloadDef.pop_back(); + compatOverloadDef.pop_back(); + // finish call + compatOverloadDef += ");\n"; + // finish body + compatOverloadDef += "}\n"; + // FIXME: 80-char line formatting? + return compatOverloadDef; + }(); + + if (compatOverloadDef.empty()) + return {}; + + for (FunctionDecl *FReDecl : FD->redecls()) { + if (FReDecl->isThisDeclarationADefinition()) { + // define compatibility overload + // We need to call the origin overload from the compatibility overload. + // Let's make sure it has been defined before the call-site by adding the definition + // below the definition of the original overload. + FixIts.emplace_back( + FixItHint::CreateInsertion( + FReDecl->getEndLoc().getLocWithOffset(1), + "\n" + compatOverloadDef) + ); + } else { + // declare compatibility overload + // Declarations commonly have Doxygen comments or other similar form of documentation. + // In order to not break the documentation we'll add the compatibility overload below the orginal one. + // FIXME: I failed to get location of the ';' after the declaration so I am adding one before the compatiblity overload. + FixIts.emplace_back( + FixItHint::CreateInsertion(FReDecl->getEndLoc().getLocWithOffset(1), ";\n" + compatOverloadNameAndSignature) + ); + } + + // get ParmVar redecl in current function redecl that corresponds to PVD + ParmVarDecl* const PVReDecl = FReDecl->getParamDecl(PVD->getFunctionScopeIndex()); + + // FIXME: At least in some special cases we should be able to emit a Fix-It for the default argument (at least with a placeholder). + if (PVReDecl->hasDefaultArg()) + return {}; // TODO test + + const Type *PVReDeclTy = PVReDecl->getType().getTypePtrOrNull(); + if (!PVReDeclTy) + return {}; // TODO test + + // FIXME: We should use std::move() to forward the argument. + if (isa(PVReDeclTy)) + return {}; // TODO test + + if (const auto* PVReDeclRecTy = dyn_cast(PVReDeclTy)) { + if (CXXRecordDecl * PVReDeclCXXRecDe = PVReDeclRecTy->getAsCXXRecordDecl()) { + // FIXME: We should check if we can move the argument. + if (!PVReDeclCXXRecDe->isTriviallyCopyable()) + return {}; // TODO test + } + } + + // change parameter type in the original overload + FixIts.emplace_back( + FixItHint::CreateReplacement( + clang::CharSourceRange::getCharRange( + PVReDecl->getTypeSourceInfo()->getTypeLoc().getBeginLoc(), + PVReDecl->getTypeSourceInfo()->getTypeLoc().getEndLoc().getLocWithOffset(1) + ), + newSpanTypeSpelling + " " + ) + ); + } + + return FixIts; +} + static FixItList fixVariableWithSpan(const VarDecl *VD, const DeclUseTracker &Tracker, const ASTContext &Ctx, @@ -887,10 +1029,33 @@ const DeclUseTracker &Tracker, const ASTContext &Ctx, UnsafeBufferUsageHandler &Handler) { + if (const auto *PVD = dyn_cast(VD)) { + auto * FD = dyn_cast(PVD->getDeclContext()); + if (!FD) + 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() + || isa(FD) // also covers call-operator of lamdas + || 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); + + if (VD->isLocalVarDecl()) + return fixVariableWithSpan(VD, Tracker, Ctx, Handler); + } + return {}; } case Strategy::Kind::Iterator: @@ -979,11 +1144,10 @@ Tracker = std::move(TrackerRes); } - // Filter out non-local vars and vars with unclaimed DeclRefExpr-s. + // Filter out vars with unclaimed DeclRefExpr-s. for (auto it = FixablesForUnsafeVars.byVar.cbegin(); it != FixablesForUnsafeVars.byVar.cend();) { - // FIXME: Support ParmVarDecl as well. - if (!it->first->isLocalVarDecl() || Tracker.hasUnclaimedUses(it->first)) { + if (Tracker.hasUnclaimedUses(it->first)) { it = FixablesForUnsafeVars.byVar.erase(it); } else { ++it; Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// TODO test with #include-d file +// TODO test if there's not a single character in the file after a decl or def + +void local_array_subscript_simple(int *p); +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span " +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:42-[[@LINE-2]]:42}:";\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)" + +void local_array_subscript_simple(int *a); +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span " +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:42-[[@LINE-2]]:42}:";\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)" + +void local_array_subscript_simple(int *p) { +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:35-[[@LINE-1]]:40}:"std::span " + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{..}}clang::unsafe_buffer_usage{{..}} void local_array_subscript_simple(int *p)\n{\n return local_array_subscript_simple(std::span(p, <# size #>));\n}\n"