diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index fa72d20bea6fec..c75eaba820c42f 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -7,9 +7,10 @@ //===----------------------------------------------------------------------===// #include "clang/Analysis/Analyses/UnsafeBufferUsage.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" #include "llvm/ADT/SmallVector.h" +#include #include using namespace llvm; @@ -383,6 +384,7 @@ class DeclUseTracker { DeclUseTracker() = default; DeclUseTracker(const DeclUseTracker &) = delete; // Let's avoid copies. DeclUseTracker(DeclUseTracker &&) = default; + DeclUseTracker &operator=(DeclUseTracker &&) = default; // Start tracking a freshly discovered DRE. void discoverUse(const DeclRefExpr *DRE) { Uses->insert(DRE); } @@ -556,97 +558,137 @@ static std::tuple findGadg return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)}; } -void clang::checkUnsafeBufferUsage(const Decl *D, - UnsafeBufferUsageHandler &Handler) { - assert(D && D->getBody()); - - SmallSet WarnedDecls; - - auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D); - - DenseMap, - std::vector>> Map; +struct WarningGadgetSets { + std::map>> byVar; + // These Gadgets are not related to pointer variables (e. g. temporaries). + llvm::SmallVector, 16> noVar; +}; - // First, let's sort gadgets by variables. If some gadgets cover more than one +static WarningGadgetSets +groupWarningGadgetsByVar(WarningGadgetList &&AllUnsafeOperations) { + WarningGadgetSets result; + // If some gadgets cover more than one // variable, they'll appear more than once in the map. - for (const auto &G : FixableGadgets) { - DeclUseList DREs = G->getClaimedVarUseSites(); + for (auto &G : AllUnsafeOperations) { + DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites(); - // Populate the map. - for (const DeclRefExpr *DRE : DREs) { + bool AssociatedWithVarDecl = false; + for (const DeclRefExpr *DRE : ClaimedVarUseSites) { if (const auto *VD = dyn_cast(DRE->getDecl())) { - Map[VD].first.push_back(G.get()); + result.byVar[VD].emplace(std::move(G)); + AssociatedWithVarDecl = true; } } + + if (!AssociatedWithVarDecl) { + result.noVar.emplace_back(std::move(G)); + continue; + } } + return result; +} - for (const auto &G : WarningGadgets) { - DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites(); +struct FixableGadgetSets { + std::map>> byVar; +}; - // Populate the map. - bool Pushed = false; - for (const DeclRefExpr *DRE : ClaimedVarUseSites) { +static FixableGadgetSets +groupFixablesByVar(FixableGadgetList &&AllFixableOperations) { + FixableGadgetSets FixablesForUnsafeVars; + for (auto &F : AllFixableOperations) { + DeclUseList DREs = F->getClaimedVarUseSites(); + + for (const DeclRefExpr *DRE : DREs) { if (const auto *VD = dyn_cast(DRE->getDecl())) { - Map[VD].second.push_back(G.get()); - Pushed = true; + FixablesForUnsafeVars.byVar[VD].emplace(std::move(F)); } } + } + return FixablesForUnsafeVars; +} - if (!Pushed) { - // We won't return to this gadget later. Emit the warning right away. - Handler.handleUnsafeOperation(G->getBaseStmt()); - continue; +static std::map +getFixIts(FixableGadgetSets &FixablesForUnsafeVars, const Strategy &S) { + std::map FixItsForVariable; + for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) { + // TODO fixVariable - fixit for the variable itself + bool ImpossibleToFix = false; + llvm::SmallVector FixItsForVD; + for (const auto &F : Fixables) { + llvm::Optional Fixits = F->getFixits(S); + if (!Fixits) { + ImpossibleToFix = true; + break; + } else { + const FixItList CorrectFixes = Fixits.value(); + FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(), + CorrectFixes.end()); + } } + if (ImpossibleToFix) + FixItsForVariable.erase(VD); + else + FixItsForVariable[VD].insert(FixItsForVariable[VD].end(), + FixItsForVD.begin(), FixItsForVD.end()); } + return FixItsForVariable; +} +static Strategy +getNaiveStrategy(const llvm::SmallVectorImpl &UnsafeVars) { Strategy S; + for (const VarDecl *VD : UnsafeVars) { + S.set(VD, Strategy::Kind::Span); + } + return S; +} - for (const auto &Item : Map) { +void clang::checkUnsafeBufferUsage(const Decl *D, + UnsafeBufferUsageHandler &Handler) { + assert(D && D->getBody()); - const VarDecl *VD = Item.first; - const std::vector &VDFixableGadgets = Item.second.first; - const std::vector &VDWarningGadgets = Item.second.second; + WarningGadgetSets UnsafeOps; + FixableGadgetSets FixablesForUnsafeVars; + DeclUseTracker Tracker; - // If the variable has no unsafe gadgets, skip it entirely. - if (VDWarningGadgets.empty()) - continue; + { + auto [FixableGadgets, WarningGadgets, TrackerRes] = findGadgets(D); + UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets)); + FixablesForUnsafeVars = groupFixablesByVar(std::move(FixableGadgets)); + Tracker = std::move(TrackerRes); + } - std::optional Fixes; - - // Avoid suggesting fixes if not all uses of the variable are identified - // as known gadgets. - // FIXME: Support parameter variables as well. - if (!Tracker.hasUnclaimedUses(VD) && VD->isLocalVarDecl()) { - // Choose the appropriate strategy. FIXME: We should try different - // strategies. - S.set(VD, Strategy::Kind::Span); - - // Check if it works. - // FIXME: This isn't sufficient (or even correct) when a gadget has - // already produced a fixit for a different variable i.e. it was mentioned - // in the map twice (or more). In such case the correct thing to do is - // to undo the previous fix first, and then if we can't produce the new - // fix for both variables, revert to the old one. - Fixes = FixItList{}; - for (const FixableGadget *G : VDFixableGadgets) { - std::optional F = G->getFixits(S); - if (!F) { - Fixes = std::nullopt; - break; - } - - for (auto &&Fixit: *F) - Fixes->push_back(std::move(Fixit)); - } + // Filter out non-local vars and 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)) { + it = FixablesForUnsafeVars.byVar.erase(it); + } else { + ++it; } + } + + llvm::SmallVector UnsafeVars; + for (const auto &[VD, ignore] : FixablesForUnsafeVars.byVar) + UnsafeVars.push_back(VD); + + Strategy NaiveStrategy = getNaiveStrategy(UnsafeVars); + std::map FixItsForVariable = + getFixIts(FixablesForUnsafeVars, NaiveStrategy); + + // FIXME Detect overlapping FixIts. + + for (const auto &G : UnsafeOps.noVar) { + Handler.handleUnsafeOperation(G->getBaseStmt()); + } - if (Fixes) { - // If we reach this point, the strategy is applicable. - Handler.handleFixableVariable(VD, std::move(*Fixes)); + for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) { + auto FixItsIt = FixItsForVariable.find(VD); + if (FixItsIt != FixItsForVariable.end()) { + Handler.handleFixableVariable(VD, std::move(FixItsIt->second)); } else { - // The strategy has failed. Emit the warning without the fixit. - S.set(VD, Strategy::Kind::Wontfix); - for (const WarningGadget *G : VDWarningGadgets) { + for (const auto &G : WarningGadgets) { Handler.handleUnsafeOperation(G->getBaseStmt()); } }