Skip to content

Commit 6829973

Browse files
jroelofssvkeerthy
jroelofs
authored and
svkeerthy
committed
[llvm-remarkutil] Make invalid states un-representable in the count tool (#140829)
This consolidates some of the error handling around regex arguments to the tool, and sets up the APIs such that errors must be handled before their usage.
1 parent c937cb5 commit 6829973

File tree

5 files changed

+111
-90
lines changed

5 files changed

+111
-90
lines changed

‎llvm/test/tools/llvm-remarkutil/instruction-count.test‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ RUN: llvm-remarkutil instruction-count --parser=yaml %p/Inputs/instruction-count
22
RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml | llvm-remarkutil instruction-count --parser=bitstream | FileCheck %s
33
RUN: llvm-remarkutil count --parser=yaml --count-by=arg --group-by=function --remark-name="InstructionCount" %p/Inputs/instruction-count.yaml | FileCheck %s --check-prefix=COUNT-CHECK
44
RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml | llvm-remarkutil count --parser=bitstream --count-by=arg --group-by=function --remark-name="InstructionCount" | FileCheck %s --check-prefix=COUNT-CHECK
5+
RUN: not llvm-remarkutil count --parser=yaml --count-by=arg --group-by=function --rremark-name=* %p/Inputs/instruction-count.yaml 2>&1 | FileCheck %s --check-prefix=ERROR-REPOPERATOR -DARG=rremark-name
6+
RUN: not llvm-remarkutil count --parser=yaml --count-by=arg --group-by=function --rpass-name=* %p/Inputs/instruction-count.yaml 2>&1 | FileCheck %s --check-prefix=ERROR-REPOPERATOR -DARG=rpass-name
7+
RUN: not llvm-remarkutil count --parser=yaml --count-by=arg --group-by=function --rfilter-arg-by=* %p/Inputs/instruction-count.yaml 2>&1 | FileCheck %s --check-prefix=ERROR-REPOPERATOR -DARG=rfilter-arg-by
8+
RUN: not llvm-remarkutil count --parser=yaml --count-by=arg --group-by=function --rremark-name=InstCombine --remark-name=InstCombine %p/Inputs/instruction-count.yaml 2>&1 | FileCheck %s --check-prefix=ERROR-BOTHFILTERS -DARG=rremark-name
59

610
; CHECK-LABEL: Function,InstructionCount
711
; CHECK: func1,1
@@ -12,3 +16,6 @@ RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml | llvm-rem
1216
; COUNT-CHECK: func1,1
1317
; COUNT-CHECK: func2,2
1418
; COUNT-CHECK: func3,3
19+
20+
; ERROR-REPOPERATOR: error: invalid argument '--[[ARG]]=*': repetition-operator operand invalid
21+
; ERROR-BOTHFILTERS: error: conflicting arguments: --remark-name and --rremark-name

‎llvm/tools/llvm-remarkutil/RemarkCounter.cpp‎

Lines changed: 27 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -111,19 +111,6 @@ static unsigned getValForKey(StringRef Key, const Remark &Remark) {
111111
return *RemarkArg->getValAsInt();
112112
}
113113

114-
Error Filters::regexArgumentsValid() {
115-
if (RemarkNameFilter && RemarkNameFilter->IsRegex)
116-
if (auto E = checkRegex(RemarkNameFilter->FilterRE))
117-
return E;
118-
if (PassNameFilter && PassNameFilter->IsRegex)
119-
if (auto E = checkRegex(PassNameFilter->FilterRE))
120-
return E;
121-
if (ArgFilter && ArgFilter->IsRegex)
122-
if (auto E = checkRegex(ArgFilter->FilterRE))
123-
return E;
124-
return Error::success();
125-
}
126-
127114
bool Filters::filterRemark(const Remark &Remark) {
128115
if (RemarkNameFilter && !RemarkNameFilter->match(Remark.RemarkName))
129116
return false;
@@ -249,28 +236,29 @@ Error RemarkCounter::print(StringRef OutputFileName) {
249236

250237
Expected<Filters> getRemarkFilter() {
251238
// Create Filter properties.
252-
std::optional<FilterMatcher> RemarkNameFilter;
253-
std::optional<FilterMatcher> PassNameFilter;
254-
std::optional<FilterMatcher> RemarkArgFilter;
239+
auto MaybeRemarkNameFilter =
240+
FilterMatcher::createExactOrRE(RemarkNameOpt, RemarkNameOptRE);
241+
if (!MaybeRemarkNameFilter)
242+
return MaybeRemarkNameFilter.takeError();
243+
244+
auto MaybePassNameFilter =
245+
FilterMatcher::createExactOrRE(PassNameOpt, PassNameOptRE);
246+
if (!MaybePassNameFilter)
247+
return MaybePassNameFilter.takeError();
248+
249+
auto MaybeRemarkArgFilter = FilterMatcher::createExactOrRE(
250+
RemarkFilterArgByOpt, RemarkArgFilterOptRE);
251+
if (!MaybeRemarkArgFilter)
252+
return MaybeRemarkArgFilter.takeError();
253+
255254
std::optional<Type> RemarkType;
256-
if (!RemarkNameOpt.empty())
257-
RemarkNameFilter = {RemarkNameOpt, false};
258-
else if (!RemarkNameOptRE.empty())
259-
RemarkNameFilter = {RemarkNameOptRE, true};
260-
if (!PassNameOpt.empty())
261-
PassNameFilter = {PassNameOpt, false};
262-
else if (!PassNameOptRE.empty())
263-
PassNameFilter = {PassNameOptRE, true};
264255
if (RemarkTypeOpt != Type::Failure)
265256
RemarkType = RemarkTypeOpt;
266-
if (!RemarkFilterArgByOpt.empty())
267-
RemarkArgFilter = {RemarkFilterArgByOpt, false};
268-
else if (!RemarkArgFilterOptRE.empty())
269-
RemarkArgFilter = {RemarkArgFilterOptRE, true};
257+
270258
// Create RemarkFilter.
271-
return Filters::createRemarkFilter(std::move(RemarkNameFilter),
272-
std::move(PassNameFilter),
273-
std::move(RemarkArgFilter), RemarkType);
259+
return Filters{std::move(*MaybeRemarkNameFilter),
260+
std::move(*MaybePassNameFilter),
261+
std::move(*MaybeRemarkArgFilter), RemarkType};
274262
}
275263

276264
Error useCollectRemark(StringRef Buffer, Counter &Counter, Filters &Filter) {
@@ -313,12 +301,16 @@ static Error collectRemarks() {
313301
SmallVector<FilterMatcher, 4> ArgumentsVector;
314302
if (!Keys.empty()) {
315303
for (auto &Key : Keys)
316-
ArgumentsVector.push_back({Key, false});
304+
ArgumentsVector.push_back(FilterMatcher::createExact(Key));
317305
} else if (!RKeys.empty())
318-
for (auto Key : RKeys)
319-
ArgumentsVector.push_back({Key, true});
306+
for (auto Key : RKeys) {
307+
auto FM = FilterMatcher::createRE(Key, RKeys);
308+
if (!FM)
309+
return FM.takeError();
310+
ArgumentsVector.push_back(std::move(*FM));
311+
}
320312
else
321-
ArgumentsVector.push_back({".*", true});
313+
ArgumentsVector.push_back(FilterMatcher::createAny());
322314

323315
Expected<ArgumentCounter> AC = ArgumentCounter::createArgumentCounter(
324316
GroupByOpt, ArgumentsVector, Buffer, Filter);

‎llvm/tools/llvm-remarkutil/RemarkCounter.h‎

Lines changed: 1 addition & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -45,66 +45,18 @@ inline std::string groupByToStr(GroupBy GroupBy) {
4545
}
4646
}
4747

48-
/// Filter object which can be either a string or a regex to match with the
49-
/// remark properties.
50-
struct FilterMatcher {
51-
Regex FilterRE;
52-
std::string FilterStr;
53-
bool IsRegex;
54-
FilterMatcher(std::string Filter, bool IsRegex) : IsRegex(IsRegex) {
55-
if (IsRegex)
56-
FilterRE = Regex(Filter);
57-
else
58-
FilterStr = Filter;
59-
}
60-
61-
bool match(StringRef StringToMatch) const {
62-
if (IsRegex)
63-
return FilterRE.match(StringToMatch);
64-
return FilterStr == StringToMatch.trim().str();
65-
}
66-
};
67-
6848
/// Filter out remarks based on remark properties based on name, pass name,
6949
/// argument and type.
7050
struct Filters {
7151
std::optional<FilterMatcher> RemarkNameFilter;
7252
std::optional<FilterMatcher> PassNameFilter;
7353
std::optional<FilterMatcher> ArgFilter;
7454
std::optional<Type> RemarkTypeFilter;
75-
/// Returns a filter object if all the arguments provided are valid regex
76-
/// types otherwise return an error.
77-
static Expected<Filters>
78-
createRemarkFilter(std::optional<FilterMatcher> RemarkNameFilter,
79-
std::optional<FilterMatcher> PassNameFilter,
80-
std::optional<FilterMatcher> ArgFilter,
81-
std::optional<Type> RemarkTypeFilter) {
82-
Filters Filter;
83-
Filter.RemarkNameFilter = std::move(RemarkNameFilter);
84-
Filter.PassNameFilter = std::move(PassNameFilter);
85-
Filter.ArgFilter = std::move(ArgFilter);
86-
Filter.RemarkTypeFilter = std::move(RemarkTypeFilter);
87-
if (auto E = Filter.regexArgumentsValid())
88-
return std::move(E);
89-
return std::move(Filter);
90-
}
55+
9156
/// Returns true if \p Remark satisfies all the provided filters.
9257
bool filterRemark(const Remark &Remark);
93-
94-
private:
95-
/// Check if arguments can be parsed as valid regex types.
96-
Error regexArgumentsValid();
9758
};
9859

99-
/// Convert Regex string error to an error object.
100-
inline Error checkRegex(const Regex &Regex) {
101-
std::string Error;
102-
if (!Regex.isValid(Error))
103-
return createStringError(make_error_code(std::errc::invalid_argument),
104-
Twine("Regex: ", Error));
105-
return Error::success();
106-
}
107-
10860
/// Abstract counter class used to define the general required methods for
10961
/// counting a remark.
11062
struct Counter {
@@ -160,12 +112,6 @@ struct ArgumentCounter : Counter {
160112
StringRef Buffer, Filters &Filter) {
161113
ArgumentCounter AC;
162114
AC.Group = Group;
163-
for (auto &Arg : Arguments) {
164-
if (Arg.IsRegex) {
165-
if (auto E = checkRegex(Arg.FilterRE))
166-
return std::move(E);
167-
}
168-
}
169115
if (auto E = AC.getAllMatchingArgumentsInRemark(Buffer, Arguments, Filter))
170116
return std::move(E);
171117
return AC;

‎llvm/tools/llvm-remarkutil/RemarkUtilHelpers.cpp‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,5 +53,44 @@ getOutputFileForRemarks(StringRef OutputFileName, Format OutputFormat) {
5353
? sys::fs::OF_TextWithCRLF
5454
: sys::fs::OF_None);
5555
}
56+
57+
Expected<FilterMatcher>
58+
FilterMatcher::createRE(const llvm::cl::opt<std::string> &Arg) {
59+
return createRE(Arg.ArgStr, Arg);
60+
}
61+
62+
Expected<FilterMatcher>
63+
FilterMatcher::createRE(StringRef Filter, const cl::list<std::string> &Arg) {
64+
return createRE(Arg.ArgStr, Filter);
65+
}
66+
67+
Expected<FilterMatcher> FilterMatcher::createRE(StringRef Arg,
68+
StringRef Value) {
69+
FilterMatcher FM(Value, true);
70+
std::string Error;
71+
if (!FM.FilterRE.isValid(Error))
72+
return createStringError(make_error_code(std::errc::invalid_argument),
73+
"invalid argument '--" + Arg + "=" + Value +
74+
"': " + Error);
75+
return std::move(FM);
76+
}
77+
78+
Expected<std::optional<FilterMatcher>>
79+
FilterMatcher::createExactOrRE(const llvm::cl::opt<std::string> &ExactArg,
80+
const llvm::cl::opt<std::string> &REArg) {
81+
if (!ExactArg.empty() && !REArg.empty())
82+
return createStringError(make_error_code(std::errc::invalid_argument),
83+
"conflicting arguments: --" + ExactArg.ArgStr +
84+
" and --" + REArg.ArgStr);
85+
86+
if (!ExactArg.empty())
87+
return createExact(ExactArg);
88+
89+
if (!REArg.empty())
90+
return createRE(REArg);
91+
92+
return std::nullopt;
93+
}
94+
5695
} // namespace remarks
5796
} // namespace llvm

‎llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,11 @@
1515
#include "llvm/Remarks/RemarkFormat.h"
1616
#include "llvm/Remarks/RemarkParser.h"
1717
#include "llvm/Remarks/YAMLRemarkSerializer.h"
18+
#include "llvm/Support/CommandLine.h"
1819
#include "llvm/Support/Error.h"
1920
#include "llvm/Support/FileSystem.h"
2021
#include "llvm/Support/MemoryBuffer.h"
22+
#include "llvm/Support/Regex.h"
2123
#include "llvm/Support/ToolOutputFile.h"
2224

2325
// Keep input + output help + names consistent across the various modes via a
@@ -55,5 +57,40 @@ Expected<std::unique_ptr<ToolOutputFile>>
5557
getOutputFileWithFlags(StringRef OutputFileName, sys::fs::OpenFlags Flags);
5658
Expected<std::unique_ptr<ToolOutputFile>>
5759
getOutputFileForRemarks(StringRef OutputFileName, Format OutputFormat);
60+
61+
/// Filter object which can be either a string or a regex to match with the
62+
/// remark properties.
63+
class FilterMatcher {
64+
Regex FilterRE;
65+
std::string FilterStr;
66+
bool IsRegex;
67+
68+
FilterMatcher(StringRef Filter, bool IsRegex)
69+
: FilterRE(Filter), FilterStr(Filter), IsRegex(IsRegex) {}
70+
71+
static Expected<FilterMatcher> createRE(StringRef Arg, StringRef Value);
72+
73+
public:
74+
static FilterMatcher createExact(StringRef Filter) { return {Filter, false}; }
75+
76+
static Expected<FilterMatcher>
77+
createRE(const llvm::cl::opt<std::string> &Arg);
78+
79+
static Expected<FilterMatcher> createRE(StringRef Filter,
80+
const cl::list<std::string> &Arg);
81+
82+
static Expected<std::optional<FilterMatcher>>
83+
createExactOrRE(const llvm::cl::opt<std::string> &ExactArg,
84+
const llvm::cl::opt<std::string> &REArg);
85+
86+
static FilterMatcher createAny() { return {".*", true}; }
87+
88+
bool match(StringRef StringToMatch) const {
89+
if (IsRegex)
90+
return FilterRE.match(StringToMatch);
91+
return FilterStr == StringToMatch.trim().str();
92+
}
93+
};
94+
5895
} // namespace remarks
5996
} // namespace llvm

0 commit comments

Comments
 (0)