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 @@ -109,8 +109,8 @@ AST_MATCHER_P(Stmt, forEveryDescendant, internal::Matcher, innerMatcher) { const DynTypedMatcher &DTM = static_cast(innerMatcher); - - MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); + + MatchDescendantVisitor Visitor(&DTM, Finder, Builder, ASTMatchFinder::BK_All); return Visitor.findMatch(DynTypedNode::create(Node)); } } // namespace clang::ast_matchers @@ -123,6 +123,16 @@ // Convenience typedef. using FixItList = SmallVector; +// To bind each `FixItList` with the `FixableGadget` which it will fix: +// FIXME: for now declarations are not Gadgets so a FixItList is associated to +// either a Gadget or a VarDecl (only one of them is significant). +using GadgetFix = + std::tuple; +// naming component indexes of `GadgetFix`: +constexpr const int GFFixItList = 0; +// constexpr const int GFGadget = 1; //unused for now +constexpr const int GFVarDecl = 2; + // Defined below. class Strategy; } // namespace @@ -360,6 +370,115 @@ } // namespace namespace { +// This class represents all fixes generated in one function declaration, and +// allows to iterate over the non-conflicting subset as well as its' +// complementary subset (i.e., conflicting subset). Any fix in the +// non-conflicting subset does not conflict with any other generated fix. Two +// fixes conflict if their source ranges overlap. +class NonConflictingFixes { +private: + // All Fix-its: + const GadgetFix * const FixIts; + + // Number of Fix-its; + const int NumFixIts; + + // Indices of Fix-its in `FixIts` that shall not be emitted: + BitVector ConflictingFixIts; + +public: + /// \return the range of the non-conflicting `GadgetFixIt`s + auto nonConflictingFixIts() const { + const GadgetFix *Begin = FixIts; + const GadgetFix *End = FixIts + NumFixIts; + const BitVector &BV = ConflictingFixIts; + + return llvm::make_filter_range(llvm::make_range(Begin, End), + [&BV, Begin](const GadgetFix &Elt) { + return !BV.test(&Elt - Begin); + }); + } + + /// \return the range of the conflicting `GadgetFixIt`s + auto conflictingFixIts() const { + const GadgetFix *Begin = FixIts; + const GadgetFix *End = FixIts + NumFixIts; + std::vector Result; + const BitVector &BV = ConflictingFixIts; + + return llvm::make_filter_range( + llvm::make_range(Begin, End), + [&BV, Begin](const GadgetFix &Elt) { return BV.test(&Elt - Begin); }); + } + + // The constructor. Populating `ConflictingFixIts` and `ConflictingGroups`: + NonConflictingFixes(const SourceManager &SM, + const std::vector &&FixIts) + : FixIts(FixIts.data()), NumFixIts(FixIts.size()) { + // To find out all conflicting hints (i.e., `FixItHint`s), we sorts the + // source ranges of all hints and keeps "merging" adjacent ones if they + // conflict. Eventually, we have a sequence of non-conflicting source + // ranges, each of which is associated to a set of fix-its. For each such + // fix-it set `F`, if `F` has more than one elements, `F` is added to + // `ConflictingFixIts`. + + // Wrap a `FixItHint`'s source range with the index of its' onwer in + // `FixIts`: + using HintSrcRange_t = std::pair; + std::vector AllHintRanges; + const int NumFixIts = FixIts.size(); + + for (int i = 0; i < NumFixIts; i++) + for (FixItHint Hint : get(FixIts[i])) { + AllHintRanges.emplace_back(Hint.RemoveRange.getAsRange(), i); + } + // Sorts all hints' source ranges: + std::sort(AllHintRanges.begin(), AllHintRanges.end(), + [&SM](const HintSrcRange_t &R1, const HintSrcRange_t &R2) { + return SM.isBeforeInTranslationUnit(R1.first.getBegin(), + R2.first.getBegin()); + }); + + // To iterate and merge conflicting source ranges, maintaining + // `MergedRange`--- the source range merged from the most recently + // iterated ones; and + // `MRFixIts`--- the set of indices of fix-it in `FixIts` associated + // to `MergedRange`. These fix-its owns hints covered by + // the `MergedRange`. + SourceRange MergedRange; + BitVector MRFixIts = BitVector(FixIts.size()); + + ConflictingFixIts = BitVector(FixIts.size()); + for (auto &HintRange : AllHintRanges) { + if (MRFixIts.none() || + SM.isBeforeInTranslationUnit(MergedRange.getEnd(), + HintRange.first.getBegin())) { + // In this case we need a "new" `MergedRange` as either it is the + // initial case or the current `HintRange` does not overlap the + // `MergedRange`. + // Before having a "new" `MergedRange`, to save the associated + // conflicting fix-its in `ConflictingFixIts`: + if (MRFixIts.count() > 1) + ConflictingFixIts |= MRFixIts; + MRFixIts.reset(); + MergedRange = + SourceRange(HintRange.first.getBegin(), HintRange.first.getEnd()); + } else { + // In case `HintRange` overlaps the `MergedRange`: + if (SM.isBeforeInTranslationUnit(MergedRange.getEnd(), + HintRange.first.getEnd())) { + // Expanding `MergedRange` to include `HintRange`: + MergedRange.setEnd(HintRange.first.getEnd()); + } // Else, `MergedRange` includes `HintRange` already. + } + MRFixIts.set(HintRange.second); + } + // Saving conflicting fix-its associated to the last `MergedRange`: + if (MRFixIts.count() > 1) + ConflictingFixIts |= MRFixIts; + } +}; + // An auxiliary tracking facility for the fixit analysis. It helps connect // declarations to its and make sure we've covered all uses with our analysis // before we try to fix the declaration. @@ -640,7 +759,7 @@ // `Ctx` a reference to the ASTContext // Returns: // the generated fix-it -static FixItHint fixVarDeclWithSpan(const VarDecl *D, +static FixItList fixVarDeclWithSpan(const VarDecl *D, const ASTContext &Ctx) { const QualType &T = D->getType().getDesugaredType(Ctx); const QualType &SpanEltT = T->getPointeeType(); @@ -654,8 +773,8 @@ if (const Expr *Init = D->getInit()) populateInitializerFixItWithSpan(Init, Ctx, OS); - return FixItHint::CreateReplacement( - SourceRange{D->getInnerLocStart(), D->getEndLoc()}, OS.str()); + return {FixItHint::CreateReplacement( + SourceRange{D->getInnerLocStart(), D->getEndLoc()}, OS.str())}; } static FixItList fixVariableWithSpan(const VarDecl *VD, @@ -663,24 +782,27 @@ const ASTContext &Ctx) { const DeclStmt *DS = Tracker.lookupDecl(VD); assert(DS && "Fixing non-local variables not implemented yet!"); - assert(DS->getSingleDecl() == VD && - "Fixing non-single declarations not implemented yet!"); + // assert(DS->getSingleDecl() == VD && + // "Fixing non-single declarations not implemented yet!"); // now they will conflict // Currently DS is an unused variable but we'll need it when // non-single decls are implemented, where the pointee type name // and the '*' are spread around the place. (void)DS; // FIXME: handle cases where DS has multiple declarations - FixItHint Fix = fixVarDeclWithSpan(VD, Ctx); - return {Fix}; + return fixVarDeclWithSpan(VD, Ctx); } static FixItList fixVariable(const VarDecl *VD, Strategy::Kind K, const DeclUseTracker &Tracker, const ASTContext &Ctx) { switch (K) { - case Strategy::Kind::Span: - return fixVariableWithSpan(VD, Tracker, Ctx); + case Strategy::Kind::Span: { + if (!VD->getType()->getPointeeType().isNull()) + // Only pointer-type variable can be transformed to be of `std::span`: + return fixVariableWithSpan(VD, Tracker, Ctx); + return {}; + } case Strategy::Kind::Iterator: case Strategy::Kind::Array: case Strategy::Kind::Vector: @@ -734,6 +856,8 @@ } Strategy S; + const ASTContext &Ctx = D->getASTContext(); + std::vector Fixes; // Collecting all fixes for (const auto &Item : Map) { @@ -745,16 +869,15 @@ if (VDWarningGadgets.empty()) continue; - std::optional Fixes; - const ASTContext &Ctx = D->getASTContext(); + std::optional> VDFixes; // fix-its for this variable if (VD->isLocalVarDecl()) { // For test purpose, we emit fix-its for local variables always for // now. // FIXME: Remove this code block once we have needed `FixableGadget`s S.set(VD, Strategy::Kind::Span); - Handler.handleFixableVariable( - VD, fixVariable(VD, S.lookup(VD), Tracker, Ctx)); + Fixes.emplace_back(fixVariable(VD, S.lookup(VD), Tracker, Ctx), nullptr, + VD); } // Avoid suggesting fixes if not all uses of the variable are identified // as known gadgets. @@ -770,26 +893,24 @@ // 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{}; + VDFixes = std::vector(); for (const FixableGadget *G : VDFixableGadgets) { std::optional F = G->getFixits(S); if (!F) { - Fixes = std::nullopt; + VDFixes = std::nullopt; break; } - - for (auto &&Fixit: *F) - Fixes->push_back(std::move(Fixit)); + VDFixes->emplace_back(std::move(*F), G, nullptr); } } - if (Fixes) { + if (VDFixes) { + // If we reach this point, the strategy is applicable. FixItList VarF = fixVariable(VD, S.lookup(VD), Tracker, Ctx); - Fixes->insert(Fixes->end(), std::make_move_iterator(VarF.begin()), - std::make_move_iterator(VarF.end())); - // If we reach this point, the strategy is applicable. - Handler.handleFixableVariable(VD, std::move(*Fixes)); + Fixes.insert(Fixes.end(), std::make_move_iterator(VDFixes->begin()), + std::make_move_iterator(VDFixes->end())); + Fixes.emplace_back(std::move(VarF), nullptr, VD); } else { // The strategy has failed. Emit the warning without the fixit. S.set(VD, Strategy::Kind::Wontfix); @@ -798,4 +919,22 @@ } } } + + // Process all fixes with the knowledge if they conflict: + NonConflictingFixes NC = + NonConflictingFixes{Ctx.getSourceManager(), std::move(Fixes)}; + + for (GadgetFix GF : NC.nonConflictingFixIts()) { + // To emit fix-its for non-conflicting variable declaration fixes + if (get(GF) != nullptr) { + Handler.handleFixableVariable(get(GF), + std::move(get(GF))); + } + } + for (auto CGF : NC.conflictingFixIts()) + // To emit warnings only (with empty fix-its) for conflicting variable + // declaration fixes + if (get(CGF) != nullptr) { + Handler.handleFixableVariable(get(CGF), FixItList{}); + } } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp @@ -71,3 +71,22 @@ // crash at runtime after applying the fix-it, foo(p[5], q[5]); // expected-warning2{{unchecked operation on raw buffer in expression}} } + +void fixits_conflict() { + // no fix-its will be emitted since fixits of `a` and `b` conflict in source locations: + // CHECK: int * a = new int[10], + // CHECK: * b = new int[10]; + // CHECK: foo(a[5], b[5]); + int * a = new int[10], // expected-warning{{variable 'a' participates in unchecked buffer operations}} \ + expected-warning{{variable 'b' participates in unchecked buffer operations}} + * b = new int[10]; + + foo(a[5], b[5]); // expected-warning2{{unchecked operation on raw buffer in expression}} + + + // CHECK: std::span c{new int [10], 10}; + // CHECK: foo(c[5]); + int * c = new int[10]; // expected-warning{{variable 'c' participates in unchecked buffer operations}} + + foo(c[5]); // expected-warning{{unchecked operation on raw buffer in expression}} +}