Index: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h =================================================================== --- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -16,11 +16,33 @@ #include "clang/AST/Decl.h" #include "clang/AST/Stmt.h" +#include "clang/Tooling/Inclusions/HeaderIncludes.h" namespace clang { class Sema; + /// This class caches loaded `tooling::HeaderIncludes` for each unique + /// `FileID`. + class HeaderIncludesCache { + private: + /// For a `FileID` 'F', `HeaderIncludeLocMap` contains 'F' iff + /// the `HeaderIncludes` for 'F' either has been loaded and cached already + /// or unloadable. In either case, no more load attempt for `F` is ever + /// needed. `HeaderIncludeLocMap.find(F)` points to a `std::nullopt` iff + /// the `HeaderIncludes` for 'F' is unloadable. + llvm::DenseMap> + HeaderIncludeLocMap; + + public: + /// \return header-insertion fix-its. One for each unique + /// `FileID` involved in `Fixes` + std::optional> + getHeaderIncludes(const SourceManager &SM, + const llvm::SmallVectorImpl &Fixes, + StringRef HeaderName); + }; + /// The interface that lets the caller handle unsafe buffer usage analysis /// results by overriding this class's handle... methods. class UnsafeBufferUsageHandler { @@ -43,6 +65,9 @@ /// Returns a reference to the `Preprocessor`: virtual bool isSafeBufferOptOut(const SourceLocation &Loc) const = 0; + /// Returns a reference to the cache for the translation unit: + virtual HeaderIncludesCache &headerIncludesCache() const = 0; + /// Returns the text indicating that the user needs to provide input there: virtual std::string getUserFillPlaceHolder(StringRef HintTextToUser = "placeholder") const { Index: clang/lib/Analysis/CMakeLists.txt =================================================================== --- clang/lib/Analysis/CMakeLists.txt +++ clang/lib/Analysis/CMakeLists.txt @@ -39,6 +39,7 @@ clangASTMatchers clangBasic clangLex + clangToolingInclusions DEPENDS omp_gen Index: clang/lib/Analysis/UnsafeBufferUsage.cpp =================================================================== --- clang/lib/Analysis/UnsafeBufferUsage.cpp +++ clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -10,11 +10,14 @@ #include "clang/AST/Decl.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" #include "clang/Lex/Preprocessor.h" #include "clang/Sema/Lookup.h" -#include "llvm/ADT/SmallVector.h" +#include "clang/Tooling/Inclusions/HeaderIncludes.h" +#include "clang/Tooling/Inclusions/IncludeStyle.h" +#include "llvm/ADT/DenseSet.h" #include #include #include @@ -1798,6 +1801,89 @@ }); } +// Creates a fix-it through a `HeaderIncludes` for a source file identified +// by`FID`. The fix-it inserts `#include `. +static std::optional +getHeaderIncludeFixIt(const tooling::HeaderIncludes &HI, FileID FID, + StringRef HeaderName, const SourceManager &SM) { + std::optional Include = + HI.insert(HeaderName.str(), true, tooling::IncludeDirective::Include); + + if (Include) + return FixItHint::CreateInsertion( + SM.getComposedLoc(FID, Include->getOffset()), + Include->getReplacementText()); + return std::nullopt; +} + +std::optional> HeaderIncludesCache::getHeaderIncludes( + const SourceManager &SM, const llvm::SmallVectorImpl &FixesForVD, + StringRef HeaderName) { + if (FixesForVD.empty()) + return std::nullopt; + + std::vector HeaderIncludeFixes{}; + // Only include a header once for a file: + llvm::SmallSet SeenFileIDs; + + for (FixItHint F : FixesForVD) { + FileID FID = SM.getDecomposedExpansionLoc(F.RemoveRange.getBegin()).first; + + if (SeenFileIDs.contains(FID)) + continue; + + auto Iter = HeaderIncludeLocMap.find(FID); + + if (Iter != HeaderIncludeLocMap.end()) { + if (Iter->getSecond()) + if (auto F = getHeaderIncludeFixIt(*Iter->getSecond(), FID, HeaderName, + SM)) { + HeaderIncludeFixes.push_back(*F); + SeenFileIDs.insert(FID); + continue; + } + // Either `HeaderIncludes` is unloadable or something goes wrong in fix-it + // generation: + return std::nullopt; + } + // Never load `HeaderIncludes` for this `FID` before, attempt to load now: + if (auto FileContent = SM.getBufferOrNone(FID)) { + const FileEntry *FE = SM.getFileEntryForID(FID); + tooling::IncludeStyle InclStyle; // TODO set IncludeStyle + auto [Iter, EmplaceSucceed] = HeaderIncludeLocMap.try_emplace( + FID, tooling::HeaderIncludes( + FE->getName(), FileContent.value().getBuffer(), InclStyle)); + + if (EmplaceSucceed) + if (auto F = getHeaderIncludeFixIt(*Iter->getSecond(), FID, HeaderName, + SM)) { + HeaderIncludeFixes.push_back(*F); + SeenFileIDs.insert(FID); + continue; + } + } else + // A `HeaderIncludes` for `FID` is unloadable: + HeaderIncludeLocMap[FID] = std::nullopt; + return std::nullopt; + } + return HeaderIncludeFixes; +} + +static std::string getHeaderFilenameForStrategyKind(Strategy::Kind TargetType) { + switch (TargetType) { + case Strategy::Kind::Span: + return "span"; + case Strategy::Kind::Array: + return "array"; + case Strategy::Kind::Vector: + return "vector"; + case Strategy::Kind::Iterator: + return "span"; + case Strategy::Kind::Wontfix: + assert(false && "Wonfix strategy shouldn't be used to generate Fix-Its"); + }; +} + static std::map getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S, /* The function decl under analysis */ const Decl *D, @@ -1828,19 +1914,31 @@ CorrectFixes.end()); } } - if (ImpossibleToFix) + if (ImpossibleToFix) { FixItsForVariable.erase(VD); - else - FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), - FixItsForVD.begin(), FixItsForVD.end()); - // We conservatively discard fix-its of a variable if - // a fix-it overlaps with macros; or - // a fix-it conflicts with another one + continue; + } + + FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), + FixItsForVD.begin(), FixItsForVD.end()); + + // Now adding insertions of `#include ...` to fix-its of `VD`. + const SourceManager &SM = Sema.getSourceManager(); + + if (auto HeaderIncFixes = Handler.headerIncludesCache().getHeaderIncludes( + SM, FixItsForVariable[VD], + getHeaderFilenameForStrategyKind(ReplacementTypeForVD))) + FixItsForVariable[VD].insert( + FixItsForVariable[VD].end(), + std::make_move_iterator(HeaderIncFixes->begin()), + std::make_move_iterator(HeaderIncFixes->end())); + // FIXME: do we want to give up other fix-its in headers cannot be included? + + // Fix-it shall not overlap with macros or/and templates: if (overlapWithMacro(FixItsForVariable[VD]) || clang::internal::anyConflict(FixItsForVariable[VD], - Ctx.getSourceManager())) { + Ctx.getSourceManager())) FixItsForVariable.erase(VD); - } } return FixItsForVariable; } Index: clang/lib/Sema/AnalysisBasedWarnings.cpp =================================================================== --- clang/lib/Sema/AnalysisBasedWarnings.cpp +++ clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2159,9 +2159,12 @@ namespace { class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { Sema &S; - + HeaderIncludesCache& HeaderIncludes; public: - UnsafeBufferUsageReporter(Sema &S) : S(S) {} + UnsafeBufferUsageReporter(Sema &S, HeaderIncludesCache &HeaderIncludesCache) + : S(S), HeaderIncludes(HeaderIncludesCache) {} + + HeaderIncludesCache &headerIncludesCache() const override { return HeaderIncludes; } void handleUnsafeOperation(const Stmt *Operation, bool IsRelatedToDecl) override { @@ -2311,11 +2314,13 @@ private: Sema &S; const bool EmitFixits; + HeaderIncludesCache &HIC; // Will be set to true iff visiting a `LambdaExpr`: bool IsVisitingLambda = false; public: - CallableVisitor(Sema &S, bool EmitFixits) : S(S), EmitFixits(EmitFixits) {} + CallableVisitor(Sema &S, bool EmitFixits, HeaderIncludesCache &HIC) + : S(S), EmitFixits(EmitFixits), HIC(HIC) {} bool TraverseDecl(Decl *Node) { if (!Node) @@ -2338,7 +2343,7 @@ // somewhere defined. But we want to know if this `Node` has a body // child. So we use `doesThisDeclarationHaveABody`: if (Node->doesThisDeclarationHaveABody()) { - UnsafeBufferUsageReporter R(S); + UnsafeBufferUsageReporter R(S, HIC); checkUnsafeBufferUsage(Node, R, S, EmitFixits); } @@ -2349,7 +2354,7 @@ if (cast(Node)->isDependentContext()) return true; // Not to analyze dependent decl - UnsafeBufferUsageReporter R(S); + UnsafeBufferUsageReporter R(S, HIC); checkUnsafeBufferUsage(Node, R, S, EmitFixits); return true; @@ -2359,7 +2364,7 @@ if (cast(Node)->isDependentContext()) return true; // Not to analyze dependent decl if (Node->hasBody()) { - UnsafeBufferUsageReporter R(S); + UnsafeBufferUsageReporter R(S, HIC); checkUnsafeBufferUsage(Node, R, S, EmitFixits); } @@ -2392,7 +2397,8 @@ // Emit unsafe buffer usage warnings and fixits. if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, SourceLocation()) || !Diags.isIgnored(diag::warn_unsafe_buffer_variable, SourceLocation())) { - CallableVisitor(S, EmitFixits) + HeaderIncludesCache HIC; + CallableVisitor(S, EmitFixits, HIC) .TraverseTranslationUnitDecl( std::remove_const_t(TU)); } Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/span =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/span @@ -0,0 +1 @@ +// Dummy std::span header \ No newline at end of file Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-header-exists.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-header-exists.cpp @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -isystem %S -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// No `#include ` will be emitted if it's already included + +#include +// CHECK-NOT: fix-it:"{{.*}}:"#include \n" + +void unsafe_code(int *p) { + // CHECK: fix-it:"{{.*}}:{[[@LINE-1]]:18-[[@LINE-1]]:24}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK: fix-it:"{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n#if __has_cpp_attribute(clang::unsafe_buffer_usage)\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n#endif\nvoid unsafe_code(int *p) {return unsafe_code(std::span(p, <# size #>));}\n" Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header-conflict.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header-conflict.cpp @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// For now, insertions of `#include` and an unsafe-buffer attribute +// conflict at the beginning of the source file. So we do not emit +// fix-its for the parameter `p`. + +// CHECK-NOT: fix-it: +void unsafe_code(int *p); + +void unsafe_code(int *p) { + int tmp; + tmp = p[5]; +} Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header-included.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header-included.cpp @@ -0,0 +1,7 @@ +// RUN: +// This file is included in test: warn-unsafe-buffer-usage-fixits-include-header.cpp +void unsafe_code() { + int tmp; + int *p = new int[10]; + tmp = p[5]; +} Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-include-header.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// Make sure that if we emit Fix-Its for an included file we also add +// the #include directive to that file (and not necessarily the main +// file). +#include "warn-unsafe-buffer-usage-fixits-include-header-included.cpp" +// CHECK: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-include-header-included.cpp":{3:1-3:1}:"#include \n" +// CHECK: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-include-header.cpp":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"#include \n" +// CHECK: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-include-header.cpp":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"#include \n" + + +// Both the fix-its for `p` in `unsafe_code2` and `unsafe_code3` emit an insertion of `#include `: + +void unsafe_code2() { + int tmp; + int *p = new int[10]; + tmp = p[5]; +} + + +void unsafe_code3() { + int tmp; + int *p = new int[10]; + tmp = p[5]; +} Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-parm-in-header.h =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-parm-in-header.h @@ -0,0 +1,4 @@ +void f(); // some irrelevant decl + +void unsafe_code(int *p); + Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-parm-in-header.cpp =================================================================== --- /dev/null +++ clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-include-header/warn-unsafe-buffer-usage-fixits-parm-in-header.cpp @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// Test that if a function parameter in header needs to be fixed, a +// `#include ` is inserted in header as well. + +// "CHECK DAG" is needed here since we are not sure whether fix-its in the header are emitted first. + +#include "warn-unsafe-buffer-usage-fixits-parm-in-header.h" +// CHECK-DAG: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-parm-in-header.h":{1:1-1:1}:"#include \n" +// CHECK-DAG: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-parm-in-header.h":{3:1-3:1}:"#if __has_cpp_attribute(clang::unsafe_buffer_usage)\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n#endif\n" +// CHECK-DAG: fix-it:"{{.*}}warn-unsafe-buffer-usage-fixits-parm-in-header.h":{3:25-3:25}:";\nvoid unsafe_code(std::span p)" +// CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-3]]:1-[[@LINE-3]]:1}:"#include \n" + +void unsafe_code(int *p) { + // CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-1]]:18-[[@LINE-1]]:24}:"std::span p" + int tmp; + tmp = p[5]; +} +// CHECK-DAG: fix-it:"{{.*}}:{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n#if __has_cpp_attribute(clang::unsafe_buffer_usage)\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n#endif\nvoid unsafe_code(int *p) {return unsafe_code(std::span(p, <# size #>));}\n" 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 @@ -5,6 +5,11 @@ #ifndef INCLUDE_ME #define INCLUDE_ME +// FIXME: currently the insertion of `#include` conflicts with +// the insertion of the unsafe-buffer attribute if both of them are +// inserted at the same location. +void f(); + void simple(int *p); // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"#if __has_cpp_attribute(clang::unsafe_buffer_usage)\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}}\n#endif\n" // CHECK-DAG: fix-it:{{.*}}:{[[@LINE-2]]:20-[[@LINE-2]]:20}:";\nvoid simple(std::span p)"