[llvm] [llvm-remarkutil] Make invalid states un-representable in the count tool (PR #140829)
Jon Roelofs via llvm-commits
llvm-commits at lists.llvm.org
Tue May 20 19:07:35 PDT 2025
https://github.com/jroelofs updated https://github.com/llvm/llvm-project/pull/140829
>From 28a34df08f2fe95029012304e547a526fa43d4a8 Mon Sep 17 00:00:00 2001
From: Jon Roelofs <jonathan_roelofs at apple.com>
Date: Mon, 19 May 2025 12:18:12 -0700
Subject: [PATCH 1/2] [llvm-remarkutil] Make invalid states un-representable in
the count tool.
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.
---
llvm/tools/llvm-remarkutil/RemarkCounter.cpp | 71 ++++++++++---------
llvm/tools/llvm-remarkutil/RemarkCounter.h | 56 +--------------
.../tools/llvm-remarkutil/RemarkUtilHelpers.h | 33 +++++++++
3 files changed, 73 insertions(+), 87 deletions(-)
diff --git a/llvm/tools/llvm-remarkutil/RemarkCounter.cpp b/llvm/tools/llvm-remarkutil/RemarkCounter.cpp
index 2d9432e41d9c0..c9806b2ba8a17 100644
--- a/llvm/tools/llvm-remarkutil/RemarkCounter.cpp
+++ b/llvm/tools/llvm-remarkutil/RemarkCounter.cpp
@@ -111,19 +111,6 @@ static unsigned getValForKey(StringRef Key, const Remark &Remark) {
return *RemarkArg->getValAsInt();
}
-Error Filters::regexArgumentsValid() {
- if (RemarkNameFilter && RemarkNameFilter->IsRegex)
- if (auto E = checkRegex(RemarkNameFilter->FilterRE))
- return E;
- if (PassNameFilter && PassNameFilter->IsRegex)
- if (auto E = checkRegex(PassNameFilter->FilterRE))
- return E;
- if (ArgFilter && ArgFilter->IsRegex)
- if (auto E = checkRegex(ArgFilter->FilterRE))
- return E;
- return Error::success();
-}
-
bool Filters::filterRemark(const Remark &Remark) {
if (RemarkNameFilter && !RemarkNameFilter->match(Remark.RemarkName))
return false;
@@ -250,27 +237,42 @@ Error RemarkCounter::print(StringRef OutputFileName) {
Expected<Filters> getRemarkFilter() {
// Create Filter properties.
std::optional<FilterMatcher> RemarkNameFilter;
+ if (!RemarkNameOpt.empty())
+ RemarkNameFilter = FilterMatcher::createExact(RemarkNameOpt);
+ else if (!RemarkNameOptRE.empty()) {
+ auto FM = FilterMatcher::createRE(RemarkNameOptRE, "rremark-name");
+ if (!FM)
+ return FM.takeError();
+ RemarkNameFilter = std::move(*FM);
+ }
+
std::optional<FilterMatcher> PassNameFilter;
+ if (!PassNameOpt.empty())
+ PassNameFilter = FilterMatcher::createExact(PassNameOpt);
+ else if (!PassNameOptRE.empty()) {
+ auto FM = FilterMatcher::createRE(PassNameOptRE, "rpass-name");
+ if (!FM)
+ return FM.takeError();
+ PassNameFilter = std::move(*FM);
+ }
+
std::optional<FilterMatcher> RemarkArgFilter;
+ if (!RemarkFilterArgByOpt.empty())
+ RemarkArgFilter = FilterMatcher::createExact(RemarkFilterArgByOpt);
+ else if (!RemarkArgFilterOptRE.empty()) {
+ auto FM = FilterMatcher::createRE(RemarkArgFilterOptRE, "rfilter-arg-by");
+ if (!FM)
+ return FM.takeError();
+ RemarkArgFilter = std::move(*FM);
+ }
+
std::optional<Type> RemarkType;
- if (!RemarkNameOpt.empty())
- RemarkNameFilter = {RemarkNameOpt, false};
- else if (!RemarkNameOptRE.empty())
- RemarkNameFilter = {RemarkNameOptRE, true};
- if (!PassNameOpt.empty())
- PassNameFilter = {PassNameOpt, false};
- else if (!PassNameOptRE.empty())
- PassNameFilter = {PassNameOptRE, true};
if (RemarkTypeOpt != Type::Failure)
RemarkType = RemarkTypeOpt;
- if (!RemarkFilterArgByOpt.empty())
- RemarkArgFilter = {RemarkFilterArgByOpt, false};
- else if (!RemarkArgFilterOptRE.empty())
- RemarkArgFilter = {RemarkArgFilterOptRE, true};
+
// Create RemarkFilter.
- return Filters::createRemarkFilter(std::move(RemarkNameFilter),
- std::move(PassNameFilter),
- std::move(RemarkArgFilter), RemarkType);
+ return Filters{std::move(RemarkNameFilter), std::move(PassNameFilter),
+ std::move(RemarkArgFilter), RemarkType};
}
Error useCollectRemark(StringRef Buffer, Counter &Counter, Filters &Filter) {
@@ -313,12 +315,17 @@ static Error collectRemarks() {
SmallVector<FilterMatcher, 4> ArgumentsVector;
if (!Keys.empty()) {
for (auto &Key : Keys)
- ArgumentsVector.push_back({Key, false});
+ ArgumentsVector.push_back(FilterMatcher::createExact(Key));
} else if (!RKeys.empty())
- for (auto Key : RKeys)
- ArgumentsVector.push_back({Key, true});
+ for (auto Key : RKeys) {
+ auto FM = FilterMatcher::createRE(Key, "count-by");
+ if (!FM)
+ return FM.takeError();
+ ArgumentsVector.push_back(std::move(*FM));
+ }
else
- ArgumentsVector.push_back({".*", true});
+ ArgumentsVector.push_back(
+ cantFail(FilterMatcher::createRE(".*", "count-by")));
Expected<ArgumentCounter> AC = ArgumentCounter::createArgumentCounter(
GroupByOpt, ArgumentsVector, Buffer, Filter);
diff --git a/llvm/tools/llvm-remarkutil/RemarkCounter.h b/llvm/tools/llvm-remarkutil/RemarkCounter.h
index 34d5bff774055..3b977791d87c2 100644
--- a/llvm/tools/llvm-remarkutil/RemarkCounter.h
+++ b/llvm/tools/llvm-remarkutil/RemarkCounter.h
@@ -45,26 +45,6 @@ inline std::string groupByToStr(GroupBy GroupBy) {
}
}
-/// Filter object which can be either a string or a regex to match with the
-/// remark properties.
-struct FilterMatcher {
- Regex FilterRE;
- std::string FilterStr;
- bool IsRegex;
- FilterMatcher(std::string Filter, bool IsRegex) : IsRegex(IsRegex) {
- if (IsRegex)
- FilterRE = Regex(Filter);
- else
- FilterStr = Filter;
- }
-
- bool match(StringRef StringToMatch) const {
- if (IsRegex)
- return FilterRE.match(StringToMatch);
- return FilterStr == StringToMatch.trim().str();
- }
-};
-
/// Filter out remarks based on remark properties based on name, pass name,
/// argument and type.
struct Filters {
@@ -72,39 +52,11 @@ struct Filters {
std::optional<FilterMatcher> PassNameFilter;
std::optional<FilterMatcher> ArgFilter;
std::optional<Type> RemarkTypeFilter;
- /// Returns a filter object if all the arguments provided are valid regex
- /// types otherwise return an error.
- static Expected<Filters>
- createRemarkFilter(std::optional<FilterMatcher> RemarkNameFilter,
- std::optional<FilterMatcher> PassNameFilter,
- std::optional<FilterMatcher> ArgFilter,
- std::optional<Type> RemarkTypeFilter) {
- Filters Filter;
- Filter.RemarkNameFilter = std::move(RemarkNameFilter);
- Filter.PassNameFilter = std::move(PassNameFilter);
- Filter.ArgFilter = std::move(ArgFilter);
- Filter.RemarkTypeFilter = std::move(RemarkTypeFilter);
- if (auto E = Filter.regexArgumentsValid())
- return std::move(E);
- return std::move(Filter);
- }
+
/// Returns true if \p Remark satisfies all the provided filters.
bool filterRemark(const Remark &Remark);
-
-private:
- /// Check if arguments can be parsed as valid regex types.
- Error regexArgumentsValid();
};
-/// Convert Regex string error to an error object.
-inline Error checkRegex(const Regex &Regex) {
- std::string Error;
- if (!Regex.isValid(Error))
- return createStringError(make_error_code(std::errc::invalid_argument),
- Twine("Regex: ", Error));
- return Error::success();
-}
-
/// Abstract counter class used to define the general required methods for
/// counting a remark.
struct Counter {
@@ -160,12 +112,6 @@ struct ArgumentCounter : Counter {
StringRef Buffer, Filters &Filter) {
ArgumentCounter AC;
AC.Group = Group;
- for (auto &Arg : Arguments) {
- if (Arg.IsRegex) {
- if (auto E = checkRegex(Arg.FilterRE))
- return std::move(E);
- }
- }
if (auto E = AC.getAllMatchingArgumentsInRemark(Buffer, Arguments, Filter))
return std::move(E);
return AC;
diff --git a/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h b/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h
index 5d2335224d4c2..31a64ce668ed1 100644
--- a/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h
+++ b/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h
@@ -18,6 +18,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Regex.h"
#include "llvm/Support/ToolOutputFile.h"
// Keep input + output help + names consistent across the various modes via a
@@ -55,5 +56,37 @@ Expected<std::unique_ptr<ToolOutputFile>>
getOutputFileWithFlags(StringRef OutputFileName, sys::fs::OpenFlags Flags);
Expected<std::unique_ptr<ToolOutputFile>>
getOutputFileForRemarks(StringRef OutputFileName, Format OutputFormat);
+
+/// Filter object which can be either a string or a regex to match with the
+/// remark properties.
+class FilterMatcher {
+ Regex FilterRE;
+ std::string FilterStr;
+ bool IsRegex;
+
+ FilterMatcher(StringRef Filter, bool IsRegex)
+ : FilterRE(Filter), FilterStr(Filter), IsRegex(IsRegex) {}
+
+public:
+ static FilterMatcher createExact(StringRef Filter) { return {Filter, false}; }
+
+ static Expected<FilterMatcher> createRE(StringRef Filter,
+ StringRef Argument) {
+ FilterMatcher FM(Filter, true);
+ std::string Error;
+ if (!FM.FilterRE.isValid(Error))
+ return createStringError(make_error_code(std::errc::invalid_argument),
+ "invalid argument '--" + Argument + "=" +
+ Filter + "': " + Error);
+ return std::move(FM);
+ }
+
+ bool match(StringRef StringToMatch) const {
+ if (IsRegex)
+ return FilterRE.match(StringToMatch);
+ return FilterStr == StringToMatch.trim().str();
+ }
+};
+
} // namespace remarks
} // namespace llvm
>From e5acd0489e9f6f5fc4b5a8c71d765568c180db88 Mon Sep 17 00:00:00 2001
From: Jon Roelofs <jonathan_roelofs at apple.com>
Date: Tue, 20 May 2025 19:07:20 -0700
Subject: [PATCH 2/2] add tests for count error handling
---
llvm/test/tools/llvm-remarkutil/instruction-count.test | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/llvm/test/tools/llvm-remarkutil/instruction-count.test b/llvm/test/tools/llvm-remarkutil/instruction-count.test
index c4bc3e5780e3f..09a9dab026ec2 100644
--- a/llvm/test/tools/llvm-remarkutil/instruction-count.test
+++ b/llvm/test/tools/llvm-remarkutil/instruction-count.test
@@ -2,6 +2,9 @@ RUN: llvm-remarkutil instruction-count --parser=yaml %p/Inputs/instruction-count
RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml | llvm-remarkutil instruction-count --parser=bitstream | FileCheck %s
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
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
+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-REMARK-NAME
+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-PASS-NAME
+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-FILTER-ARG-BY
; CHECK-LABEL: Function,InstructionCount
; CHECK: func1,1
@@ -12,3 +15,7 @@ RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml | llvm-rem
; COUNT-CHECK: func1,1
; COUNT-CHECK: func2,2
; COUNT-CHECK: func3,3
+
+; ERROR-REMARK-NAME: error: invalid argument '--rremark-name=*': repetition-operator operand invalid
+; ERROR-PASS-NAME: error: invalid argument '--rpass-name=*': repetition-operator operand invalid
+; ERROR-FILTER-ARG-BY: error: invalid argument '--rfilter-arg-by=*': repetition-operator operand invalid
\ No newline at end of file
More information about the llvm-commits
mailing list