Fix crash in modernize-use-ranges #100427
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues
|
No release notes added as this will likely want to get backported |
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Nathan James (njames93) ChangesCrash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues Fixes #100406 Full diff: https://github.com/llvm/llvm-project/pull/100427.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
index e2daa5010e2ae..aba4d17ccd035 100644
--- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
@@ -39,12 +39,6 @@ static constexpr const char ArgName[] = "ArgName";
namespace clang::tidy::utils {
-static bool operator==(const UseRangesCheck::Indexes &L,
- const UseRangesCheck::Indexes &R) {
- return std::tie(L.BeginArg, L.EndArg, L.ReplaceArg) ==
- std::tie(R.BeginArg, R.EndArg, R.ReplaceArg);
-}
-
static std::string getFullPrefix(ArrayRef<UseRangesCheck::Indexes> Signature) {
std::string Output;
llvm::raw_string_ostream OS(Output);
@@ -54,15 +48,6 @@ static std::string getFullPrefix(ArrayRef<UseRangesCheck::Indexes> Signature) {
return Output;
}
-static llvm::hash_code hash_value(const UseRangesCheck::Indexes &Indexes) {
- return llvm::hash_combine(Indexes.BeginArg, Indexes.EndArg,
- Indexes.ReplaceArg);
-}
-
-static llvm::hash_code hash_value(const UseRangesCheck::Signature &Sig) {
- return llvm::hash_combine_range(Sig.begin(), Sig.end());
-}
-
namespace {
AST_MATCHER(Expr, hasSideEffects) {
@@ -123,24 +108,26 @@ makeMatcherPair(StringRef State, const UseRangesCheck::Indexes &Indexes,
}
void UseRangesCheck::registerMatchers(MatchFinder *Finder) {
- Replaces = getReplacerMap();
+ auto Replaces = getReplacerMap();
ReverseDescriptor = getReverseDescriptor();
auto BeginEndNames = getFreeBeginEndMethods();
llvm::SmallVector<StringRef, 4> BeginNames{
llvm::make_first_range(BeginEndNames)};
llvm::SmallVector<StringRef, 4> EndNames{
llvm::make_second_range(BeginEndNames)};
- llvm::DenseSet<ArrayRef<Signature>> Seen;
+ Replacers.clear();
+ llvm::DenseSet<Replacer *> SeenRepl;
for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) {
- const ArrayRef<Signature> &Signatures =
- I->getValue()->getReplacementSignatures();
- if (!Seen.insert(Signatures).second)
+ auto Replacer = I->getValue();
+ if (!SeenRepl.insert(Replacer.get()).second)
continue;
- assert(!Signatures.empty() &&
- llvm::all_of(Signatures, [](auto Index) { return !Index.empty(); }));
+ Replacers.push_back(Replacer);
+ assert(!Replacer->getReplacementSignatures().empty() &&
+ llvm::all_of(Replacer->getReplacementSignatures(),
+ [](auto Index) { return !Index.empty(); }));
std::vector<StringRef> Names(1, I->getKey());
for (auto J = std::next(I); J != E; ++J)
- if (J->getValue()->getReplacementSignatures() == Signatures)
+ if (J->getValue() == Replacer)
Names.push_back(J->getKey());
std::vector<ast_matchers::internal::DynTypedMatcher> TotalMatchers;
@@ -148,7 +135,7 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) {
// signatures in order of length(longest to shortest). This way any
// signature that is a subset of another signature will be matched after the
// other.
- SmallVector<Signature> SigVec(Signatures);
+ SmallVector<Signature> SigVec(Replacer->getReplacementSignatures());
llvm::sort(SigVec, [](auto &L, auto &R) { return R.size() < L.size(); });
for (const auto &Signature : SigVec) {
std::vector<ast_matchers::internal::DynTypedMatcher> Matchers;
@@ -163,7 +150,8 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) {
}
Finder->addMatcher(
callExpr(
- callee(functionDecl(hasAnyName(std::move(Names))).bind(FuncDecl)),
+ callee(functionDecl(hasAnyName(std::move(Names)))
+ .bind((FuncDecl + Twine(Replacers.size() - 1).str()))),
ast_matchers::internal::DynTypedMatcher::constructVariadic(
ast_matchers::internal::DynTypedMatcher::VO_AnyOf,
ASTNodeKind::getFromNodeKind<CallExpr>(),
@@ -205,21 +193,33 @@ static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call,
}
void UseRangesCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>(FuncDecl);
- std::string Qualified = "::" + Function->getQualifiedNameAsString();
- auto Iter = Replaces.find(Qualified);
- assert(Iter != Replaces.end());
+ Replacer *Replacer = nullptr;
+ const FunctionDecl *Function = nullptr;
+ for (auto [Node, Value] : Result.Nodes.getMap()) {
+ StringRef NodeStr(Node);
+ if (!NodeStr.consume_front(FuncDecl))
+ continue;
+ Function = Value.get<FunctionDecl>();
+ size_t Index;
+ if (NodeStr.getAsInteger(10, Index)) {
+ llvm_unreachable("Unable to extract replacer index");
+ }
+ assert(Index < Replacers.size());
+ Replacer = Replacers[Index].get();
+ break;
+ }
+ assert(Replacer && Function);
SmallString<64> Buffer;
- for (const Signature &Sig : Iter->getValue()->getReplacementSignatures()) {
+ for (const Signature &Sig : Replacer->getReplacementSignatures()) {
Buffer.assign({BoundCall, getFullPrefix(Sig)});
const auto *Call = Result.Nodes.getNodeAs<CallExpr>(Buffer);
if (!Call)
continue;
auto Diag = createDiag(*Call);
- if (auto ReplaceName = Iter->getValue()->getReplaceName(*Function))
+ if (auto ReplaceName = Replacer->getReplaceName(*Function))
Diag << FixItHint::CreateReplacement(Call->getCallee()->getSourceRange(),
*ReplaceName);
- if (auto Include = Iter->getValue()->getHeaderInclusion(*Function))
+ if (auto Include = Replacer->getHeaderInclusion(*Function))
Diag << Inserter.createIncludeInsertion(
Result.SourceManager->getFileID(Call->getBeginLoc()), *Include);
llvm::SmallVector<unsigned, 3> ToRemove;
diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h
index 927e9694b0ec7..3a454bcf0cf07 100644
--- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h
+++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.h
@@ -85,7 +85,7 @@ class UseRangesCheck : public ClangTidyCheck {
std::optional<TraversalKind> getCheckTraversalKind() const override;
private:
- ReplacerMap Replaces;
+ std::vector<llvm::IntrusiveRefCntPtr<Replacer>> Replacers;
std::optional<ReverseIteratorDescriptor> ReverseDescriptor;
IncludeInserter Inserter;
};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h
index 6596511c7a38b..69ac9954f4afa 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-ranges/fake_std.h
@@ -7,8 +7,8 @@ template <typename T> class vector {
public:
using iterator = T *;
using const_iterator = const T *;
- using reverse_iterator = T*;
- using reverse_const_iterator = const T*;
+ using reverse_iterator = T *;
+ using reverse_const_iterator = const T *;
constexpr const_iterator begin() const;
constexpr const_iterator end() const;
@@ -72,8 +72,8 @@ template <typename Container> constexpr auto crend(const Container &Cont) {
return Cont.crend();
}
// Find
-template< class InputIt, class T >
-InputIt find( InputIt first, InputIt last, const T& value );
+template <class InputIt, class T>
+InputIt find(InputIt first, InputIt last, const T &value);
// Reverse
template <typename Iter> void reverse(Iter begin, Iter end);
@@ -82,6 +82,7 @@ template <typename Iter> void reverse(Iter begin, Iter end);
template <class InputIt1, class InputIt2>
bool includes(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2);
+inline namespace _V1 {
// IsPermutation
template <class ForwardIt1, class ForwardIt2>
bool is_permutation(ForwardIt1 first1, ForwardIt1 last1, ForwardIt2 first2);
@@ -97,9 +98,10 @@ template <class InputIt1, class InputIt2>
bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2);
template <class InputIt1, class InputIt2, class BinaryPred>
-bool equal(InputIt1 first1, InputIt1 last1,
- InputIt2 first2, InputIt2 last2, BinaryPred p) {
- // Need a definition to suppress undefined_internal_type when invoked with lambda
+bool equal(InputIt1 first1, InputIt1 last1, InputIt2 first2, InputIt2 last2,
+ BinaryPred p) {
+ // Need a definition to suppress undefined_internal_type when invoked with
+ // lambda
return true;
}
@@ -108,6 +110,7 @@ void iota(ForwardIt first, ForwardIt last, T value);
template <class ForwardIt>
ForwardIt rotate(ForwardIt first, ForwardIt middle, ForwardIt last);
+} // namespace _V1
} // namespace std
|
PiotrZSL
approved these changes
Jul 24, 2024
| ReverseDescriptor = getReverseDescriptor(); | ||
| auto BeginEndNames = getFreeBeginEndMethods(); | ||
| llvm::SmallVector<StringRef, 4> BeginNames{ | ||
| llvm::make_first_range(BeginEndNames)}; | ||
| llvm::SmallVector<StringRef, 4> EndNames{ | ||
| llvm::make_second_range(BeginEndNames)}; | ||
| llvm::DenseSet<ArrayRef<Signature>> Seen; | ||
| Replacers.clear(); |
There was a problem hiding this comment.
Note: that Replaces vs Replacers is confusing, consider changing a name of one later on main.
|
Fix is reported to work, so lets merge it. |
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues Fixes #100406 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250581
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 1, 2024
Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues Fixes llvm#100406 (cherry picked from commit 0762db6)
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 5, 2024
Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues Fixes llvm#100406 (cherry picked from commit 0762db6)
|
I posted comment on the backport change, but probably here is a better place |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Crash seems to be caused by the check function not handling inline namespaces correctly for some instances. Changed how the Replacer is got from the MatchResult now which should alleviate any potential issues
Fixes #100406