Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -864,6 +864,139 @@ return FixIts; } +static FixItList fixParamWithSpan(const ParmVarDecl *PVD, + const ASTContext &Ctx) { + // TODO detect and bail + // templates + // variadic functions + // default parameter values - in any redeclaration + // virtual method + // any method (for now) + // TODO can there be exception spec or some other block between signature and body that could refer to params? + + FixItList FixIts{}; + auto * FD = dyn_cast(PVD->getDeclContext()); + if (!FD) { + // TODO error handling + return {}; + } + + // TODO assert param is a pointer + + // TODO iterate all re-decls + // TODO 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 { + // TODO what about for example "static" - would it be included? + std::string result = + "[[clang::unsafe_buffer_usage]] " + + std::string( + SM.getCharacterData(FD->getBeginLoc()), + SM.getCharacterData(FD->getBody()->getBeginLoc()) + ); + while(!result.empty()) { + // TODO don't hardcode - use the is horiz whitespace function + if (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()) { + // FIXME We currently don't support class methods (implicit this). + 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 { + // TODO some args need to be std::move()-ed - e. g. unique_ptr + 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 {}; // TODO error-handling + + 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()); + + // 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, @@ -889,8 +1022,14 @@ UnsafeBufferUsageHandler &Handler) { switch (K) { case Strategy::Kind::Span: { - if (VD->getType()->isPointerType()) + if (VD->getType()->isPointerType()) { + if (const auto *PVD = dyn_cast(VD)) { + return fixParamWithSpan(PVD, Ctx); + } return fixVariableWithSpan(VD, Tracker, Ctx, Handler); + } + + // TODO shouldn't we return std::nullopt instead? return {}; } case Strategy::Kind::Iterator: @@ -979,11 +1118,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"