[NFCI] Change SpecialCaseList::inSectionBlame to return pair<uint, uint> (FileIdx, LineNo). #141540
Conversation
This file contains hidden or 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
Created using spr 1.3.6 [skip ci]
Created using spr 1.3.6
|
@llvm/pr-subscribers-llvm-support Author: Qinkun Bao (qinkunbao) ChangesAccoring to the discussion in Full diff: https://github.com/llvm/llvm-project/pull/141540.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Support/SpecialCaseList.h b/llvm/include/llvm/Support/SpecialCaseList.h
index 653a3b14ebf03..bce337f553a93 100644
--- a/llvm/include/llvm/Support/SpecialCaseList.h
+++ b/llvm/include/llvm/Support/SpecialCaseList.h
@@ -17,6 +17,7 @@
#include "llvm/Support/GlobPattern.h"
#include "llvm/Support/Regex.h"
#include <memory>
+#include <utility>
#include <string>
#include <vector>
@@ -93,17 +94,17 @@ class SpecialCaseList {
LLVM_ABI bool inSection(StringRef Section, StringRef Prefix, StringRef Query,
StringRef Category = StringRef()) const;
- /// Returns the line number corresponding to the special case list entry if
- /// the special case list contains a line
+ /// Returns the file index and the line numebr <FileIdx, LineNo> corresponding
+ /// to the special case list entry if the special case list contains a line
/// \code
/// @Prefix:<E>=@Category
/// \endcode
/// where @Query satisfies the glob <E> in a given @Section.
- /// Returns zero if there is no exclusion entry corresponding to this
+ /// Returns (zero, zero) if there is no exclusion entry corresponding to this
/// expression.
- LLVM_ABI unsigned inSectionBlame(StringRef Section, StringRef Prefix,
- StringRef Query,
- StringRef Category = StringRef()) const;
+ LLVM_ABI std::pair<unsigned, unsigned>
+ inSectionBlame(StringRef Section, StringRef Prefix, StringRef Query,
+ StringRef Category = StringRef()) const;
protected:
// Implementations of the create*() functions that can also be used by derived
@@ -145,12 +146,14 @@ class SpecialCaseList {
Section(std::unique_ptr<Matcher> M) : SectionMatcher(std::move(M)) {};
Section() : Section(std::make_unique<Matcher>()) {};
+ unsigned FileIdx;
std::unique_ptr<Matcher> SectionMatcher;
SectionEntries Entries;
std::string SectionStr;
};
std::vector<Section> Sections;
+ unsigned currFileIdx;
LLVM_ABI Expected<Section *> addSection(StringRef SectionStr, unsigned LineNo,
bool UseGlobs = true);
diff --git a/llvm/lib/Support/SpecialCaseList.cpp b/llvm/lib/Support/SpecialCaseList.cpp
index 56327b56282c2..5a66f163d7913 100644
--- a/llvm/lib/Support/SpecialCaseList.cpp
+++ b/llvm/lib/Support/SpecialCaseList.cpp
@@ -112,6 +112,7 @@ bool SpecialCaseList::createInternal(const std::vector<std::string> &Paths,
return false;
}
std::string ParseError;
+ ++currFileIdx;
if (!parse(FileOrErr.get().get(), ParseError)) {
Error = (Twine("error parsing file '") + Path + "': " + ParseError).str();
return false;
@@ -122,6 +123,7 @@ bool SpecialCaseList::createInternal(const std::vector<std::string> &Paths,
bool SpecialCaseList::createInternal(const MemoryBuffer *MB,
std::string &Error) {
+ ++currFileIdx;
if (!parse(MB, Error))
return false;
return true;
@@ -133,6 +135,7 @@ SpecialCaseList::addSection(StringRef SectionStr, unsigned LineNo,
Sections.emplace_back();
auto &Section = Sections.back();
Section.SectionStr = SectionStr;
+ Section.FileIdx = currFileIdx;
if (auto Err = Section.SectionMatcher->insert(SectionStr, LineNo, UseGlobs)) {
return createStringError(errc::invalid_argument,
@@ -207,20 +210,21 @@ SpecialCaseList::~SpecialCaseList() = default;
bool SpecialCaseList::inSection(StringRef Section, StringRef Prefix,
StringRef Query, StringRef Category) const {
- return inSectionBlame(Section, Prefix, Query, Category);
+ auto [FileIdx, LineNo] = inSectionBlame(Section, Prefix, Query, Category);
+ return LineNo;
}
-unsigned SpecialCaseList::inSectionBlame(StringRef Section, StringRef Prefix,
- StringRef Query,
- StringRef Category) const {
+std::pair<unsigned, unsigned>
+SpecialCaseList::inSectionBlame(StringRef Section, StringRef Prefix,
+ StringRef Query, StringRef Category) const {
for (auto it = Sections.crbegin(); it != Sections.crend(); ++it) {
if (it->SectionMatcher->match(Section)) {
unsigned Blame = inSectionBlame(it->Entries, Prefix, Query, Category);
if (Blame)
- return Blame;
+ return {it->FileIdx, Blame};
}
}
- return 0;
+ return {0, 0};
}
unsigned SpecialCaseList::inSectionBlame(const SectionEntries &Entries,
diff --git a/llvm/tools/llvm-cfi-verify/llvm-cfi-verify.cpp b/llvm/tools/llvm-cfi-verify/llvm-cfi-verify.cpp
index 0f2c7da94230e..b372957204691 100644
--- a/llvm/tools/llvm-cfi-verify/llvm-cfi-verify.cpp
+++ b/llvm/tools/llvm-cfi-verify/llvm-cfi-verify.cpp
@@ -78,8 +78,7 @@ static void printBlameContext(const DILineInfo &LineInfo, unsigned Context) {
File->getBuffer().split(Lines, '\n');
for (unsigned i = std::max<size_t>(1, LineInfo.Line - Context);
- i <
- std::min<size_t>(Lines.size() + 1, LineInfo.Line + Context + 1);
+ i < std::min<size_t>(Lines.size() + 1, LineInfo.Line + Context + 1);
++i) {
if (i == LineInfo.Line)
outs() << ">";
@@ -193,12 +192,16 @@ printIndirectCFInstructions(FileAnalysis &Analysis,
unsigned BlameLine = 0;
for (auto &K : {"cfi-icall", "cfi-vcall"}) {
- if (!BlameLine)
- BlameLine =
+ if (!BlameLine) {
+ auto [FileIdx, Line] =
SpecialCaseList->inSectionBlame(K, "src", LineInfo.FileName);
- if (!BlameLine)
- BlameLine =
+ BlameLine = Line;
+ }
+ if (!BlameLine) {
+ auto [FileIdx, Line] =
SpecialCaseList->inSectionBlame(K, "fun", LineInfo.FunctionName);
+ BlameLine = Line;
+ }
}
if (BlameLine) {
diff --git a/llvm/unittests/Support/SpecialCaseListTest.cpp b/llvm/unittests/Support/SpecialCaseListTest.cpp
index ce58328f396b6..da4c557e740e6 100644
--- a/llvm/unittests/Support/SpecialCaseListTest.cpp
+++ b/llvm/unittests/Support/SpecialCaseListTest.cpp
@@ -14,6 +14,7 @@
#include "gtest/gtest.h"
using testing::HasSubstr;
+using testing::Pair;
using testing::StartsWith;
using namespace llvm;
@@ -70,13 +71,14 @@ TEST_F(SpecialCaseListTest, Basic) {
EXPECT_FALSE(SCL->inSection("", "fun", "hello"));
EXPECT_FALSE(SCL->inSection("", "src", "hello", "category"));
- EXPECT_EQ(3u, SCL->inSectionBlame("", "src", "hello"));
- EXPECT_EQ(4u, SCL->inSectionBlame("", "src", "bye"));
- EXPECT_EQ(5u, SCL->inSectionBlame("", "src", "hi", "category"));
- EXPECT_EQ(6u, SCL->inSectionBlame("", "src", "zzzz", "category"));
- EXPECT_EQ(0u, SCL->inSectionBlame("", "src", "hi"));
- EXPECT_EQ(0u, SCL->inSectionBlame("", "fun", "hello"));
- EXPECT_EQ(0u, SCL->inSectionBlame("", "src", "hello", "category"));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "hello"), Pair(1u, 3u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "bye"), Pair(1u, 4u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "hi", "category"), Pair(1u, 5u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "zzzz", "category"), Pair(1u, 6u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "hi"), Pair(0u, 0u));
+ EXPECT_THAT(SCL->inSectionBlame("", "fun", "hello"), Pair(0u, 0u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "hello", "category"),
+ Pair(0u, 0u));
}
TEST_F(SpecialCaseListTest, CorrectErrorLineNumberWithBlankLine) {
@@ -216,8 +218,9 @@ TEST_F(SpecialCaseListTest, NoTrigramsInARule) {
}
TEST_F(SpecialCaseListTest, RepetitiveRule) {
- std::unique_ptr<SpecialCaseList> SCL = makeSpecialCaseList("fun:*bar*bar*bar*bar*\n"
- "fun:bar*\n");
+ std::unique_ptr<SpecialCaseList> SCL =
+ makeSpecialCaseList("fun:*bar*bar*bar*bar*\n"
+ "fun:bar*\n");
EXPECT_TRUE(SCL->inSection("", "fun", "bara"));
EXPECT_FALSE(SCL->inSection("", "fun", "abara"));
EXPECT_TRUE(SCL->inSection("", "fun", "barbarbarbar"));
@@ -226,7 +229,8 @@ TEST_F(SpecialCaseListTest, RepetitiveRule) {
}
TEST_F(SpecialCaseListTest, SpecialSymbolRule) {
- std::unique_ptr<SpecialCaseList> SCL = makeSpecialCaseList("src:*c\\+\\+abi*\n");
+ std::unique_ptr<SpecialCaseList> SCL =
+ makeSpecialCaseList("src:*c\\+\\+abi*\n");
EXPECT_TRUE(SCL->inSection("", "src", "c++abi"));
EXPECT_FALSE(SCL->inSection("", "src", "c\\+\\+abi"));
}
@@ -242,8 +246,9 @@ TEST_F(SpecialCaseListTest, PopularTrigram) {
}
TEST_F(SpecialCaseListTest, EscapedSymbols) {
- std::unique_ptr<SpecialCaseList> SCL = makeSpecialCaseList("src:*c\\+\\+abi*\n"
- "src:*hello\\\\world*\n");
+ std::unique_ptr<SpecialCaseList> SCL =
+ makeSpecialCaseList("src:*c\\+\\+abi*\n"
+ "src:*hello\\\\world*\n");
EXPECT_TRUE(SCL->inSection("", "src", "dir/c++abi"));
EXPECT_FALSE(SCL->inSection("", "src", "dir/c\\+\\+abi"));
EXPECT_FALSE(SCL->inSection("", "src", "c\\+\\+abi"));
@@ -323,10 +328,33 @@ TEST_F(SpecialCaseListTest, Version3) {
EXPECT_TRUE(SCL->inSection("sect2", "src", "def"));
EXPECT_TRUE(SCL->inSection("sect1", "src", "def"));
- EXPECT_EQ(2u, SCL->inSectionBlame("sect1", "src", "fooz"));
- EXPECT_EQ(4u, SCL->inSectionBlame("sect1", "src", "barz"));
- EXPECT_EQ(5u, SCL->inSectionBlame("sect1", "src", "def"));
- EXPECT_EQ(8u, SCL->inSectionBlame("sect2", "src", "def"));
- EXPECT_EQ(8u, SCL->inSectionBlame("sect2", "src", "dez"));
+ EXPECT_THAT(SCL->inSectionBlame("sect1", "src", "fooz"), Pair(1u, 2u));
+ EXPECT_THAT(SCL->inSectionBlame("sect1", "src", "barz"), Pair(1u, 4u));
+ EXPECT_THAT(SCL->inSectionBlame("sect1", "src", "def"), Pair(1u, 5u));
+ EXPECT_THAT(SCL->inSectionBlame("sect2", "src", "def"), Pair(1u, 8u));
+ EXPECT_THAT(SCL->inSectionBlame("sect2", "src", "dez"), Pair(1u, 8u));
}
+
+TEST_F(SpecialCaseListTest, FileIdx) {
+ std::vector<std::string> Files;
+ Files.push_back(makeSpecialCaseListFile("src:bar\n"
+ "src:*foo*\n"
+ "src:ban=init\n"
+ "src:baz\n"
+ "src:*def\n"));
+ Files.push_back(makeSpecialCaseListFile("src:baz\n"
+ "src:car\n"
+ "src:def*"));
+ auto SCL = SpecialCaseList::createOrDie(Files, *vfs::getRealFileSystem());
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "bar"), Pair(1u, 1u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "fooaaaaaa"), Pair(1u, 2u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "ban", "init"), Pair(1u, 3u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "baz"), Pair(2u, 1u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "car"), Pair(2u, 2u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "aaaadef"), Pair(1u, 5u));
+ EXPECT_THAT(SCL->inSectionBlame("", "src", "defaaaaa"), Pair(2u, 3u));
+ for (auto &Path : Files)
+ sys::fs::remove(Path);
}
+
+} // namespace
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.6
Created using spr 1.3.6
qinkunbao
changed the title
qinkunbao
changed the title
vitalybuka
reviewed
May 28, 2025
| std::unique_ptr<Matcher> SectionMatcher; | ||
| SectionEntries Entries; | ||
| std::string SectionStr; | ||
| }; | ||
|
|
||
| std::vector<Section> Sections; | ||
| unsigned currFileIdx; |
There was a problem hiding this comment.
we don't need this as part of the state
please add another argument into addSection(fileIdx) and parse(fileIdx)
vitalybuka
reviewed
May 28, 2025
llvm/lib/Support/SpecialCaseList.cpp
Outdated
| @@ -122,6 +123,7 @@ bool SpecialCaseList::createInternal(const std::vector<std::string> &Paths, | |||
|
|
|||
| bool SpecialCaseList::createInternal(const MemoryBuffer *MB, | |||
| std::string &Error) { | |||
| ++currFileIdx; | |||
There was a problem hiding this comment.
MemoryBuffer is always one, let's make it 0
vitalybuka
reviewed
May 28, 2025
llvm/lib/Support/SpecialCaseList.cpp
Outdated
| @@ -112,6 +112,7 @@ bool SpecialCaseList::createInternal(const std::vector<std::string> &Paths, | |||
| return false; | |||
| } | |||
| std::string ParseError; | |||
| ++currFileIdx; | |||
There was a problem hiding this comment.
Wouild be nice to match idx in Paths array
for (const auto &Path : Paths) -> for (unsigned Idx = 0; ....)
qinkunbao
changed the base branch from
users/qinkunbao/spr/main.change-insectionblame-to-return-pairuint-uint-fileidx-lineno
to
main
|
I have local changes, I'll push update |
vitalybuka
approved these changes
May 28, 2025
Created using spr 1.3.6
qinkunbao
commented
May 28, 2025
| @@ -70,7 +70,7 @@ class FileSystem; | |||
| /// --- | |||
| class SpecialCaseList { | |||
| public: | |||
| constexpr static std::pair<unsigned, unsigned> NotFound = {0, 0}; | |||
| static constexpr std::pair<unsigned, unsigned> NotFound = {0, 0}; | |||
There was a problem hiding this comment.
I thought static constexpr and constexpr static mean exactly the same thing
qinkunbao
deleted the
users/qinkunbao/spr/change-insectionblame-to-return-pairuint-uint-fileidx-lineno
branch
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…nt> (FileIdx, LineNo). (llvm#141540) Accoring to the discussion in llvm#140529, we need to SSCL can be created from multiple ignore list files, so we can repeat-fsanitize-ignorelist=. The change is necessary to achieve the feature described in llvm#139128.
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.
Accoring to the discussion in #140529, we need to SSCL can be
created from multiple ignore list files, so we can repeat-fsanitize-ignorelist=. The change is necessary to achieve the feature described in #139128.