[llvm-remarkutil] Make invalid states un-representable in the count tool (#140829)

jroelofs · web-flow · commit 42c8742e0e2d · 2025-05-28T16:27:56.000-07:00
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.
diff --git a/llvm/test/tools/llvm-remarkutil/instruction-count.test b/llvm/test/tools/llvm-remarkutil/instruction-count.test
@@ -2,6 +2,10 @@ 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-REPOPERATOR -DARG=rremark-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-REPOPERATOR -DARG=rpass-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-REPOPERATOR -DARG=rfilter-arg-by
+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
 
 ; CHECK-LABEL: Function,InstructionCount
 ; CHECK: func1,1
@@ -12,3 +16,6 @@ RUN: llvm-remarkutil yaml2bitstream %p/Inputs/instruction-count.yaml  | llvm-rem
 ; COUNT-CHECK: func1,1
 ; COUNT-CHECK: func2,2
 ; COUNT-CHECK: func3,3
+
+; ERROR-REPOPERATOR: error: invalid argument '--[[ARG]]=*': repetition-operator operand invalid
+; ERROR-BOTHFILTERS: error: conflicting arguments: --remark-name and --rremark-name
diff --git 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;
@@ -249,28 +236,29 @@ Error RemarkCounter::print(StringRef OutputFileName) {
 
 Expected<Filters> getRemarkFilter() {
   // Create Filter properties.
-  std::optional<FilterMatcher> RemarkNameFilter;
-  std::optional<FilterMatcher> PassNameFilter;
-  std::optional<FilterMatcher> RemarkArgFilter;
+  auto MaybeRemarkNameFilter =
+      FilterMatcher::createExactOrRE(RemarkNameOpt, RemarkNameOptRE);
+  if (!MaybeRemarkNameFilter)
+    return MaybeRemarkNameFilter.takeError();
+
+  auto MaybePassNameFilter =
+      FilterMatcher::createExactOrRE(PassNameOpt, PassNameOptRE);
+  if (!MaybePassNameFilter)
+    return MaybePassNameFilter.takeError();
+
+  auto MaybeRemarkArgFilter = FilterMatcher::createExactOrRE(
+      RemarkFilterArgByOpt, RemarkArgFilterOptRE);
+  if (!MaybeRemarkArgFilter)
+    return MaybeRemarkArgFilter.takeError();
+
   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(*MaybeRemarkNameFilter),
+                 std::move(*MaybePassNameFilter),
+                 std::move(*MaybeRemarkArgFilter), RemarkType};
 }
 
 Error useCollectRemark(StringRef Buffer, Counter &Counter, Filters &Filter) {
@@ -313,12 +301,16 @@ 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, RKeys);
+        if (!FM)
+          return FM.takeError();
+        ArgumentsVector.push_back(std::move(*FM));
+      }
     else
-      ArgumentsVector.push_back({".*", true});
+      ArgumentsVector.push_back(FilterMatcher::createAny());
 
     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
@@ -45,66 +45,18 @@ 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 {
   std::optional<FilterMatcher> RemarkNameFilter;
   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.cpp b/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.cpp
@@ -53,5 +53,44 @@ getOutputFileForRemarks(StringRef OutputFileName, Format OutputFormat) {
                                                     ? sys::fs::OF_TextWithCRLF
                                                     : sys::fs::OF_None);
 }
+
+Expected<FilterMatcher>
+FilterMatcher::createRE(const llvm::cl::opt<std::string> &Arg) {
+  return createRE(Arg.ArgStr, Arg);
+}
+
+Expected<FilterMatcher>
+FilterMatcher::createRE(StringRef Filter, const cl::list<std::string> &Arg) {
+  return createRE(Arg.ArgStr, Filter);
+}
+
+Expected<FilterMatcher> FilterMatcher::createRE(StringRef Arg,
+                                                StringRef Value) {
+  FilterMatcher FM(Value, true);
+  std::string Error;
+  if (!FM.FilterRE.isValid(Error))
+    return createStringError(make_error_code(std::errc::invalid_argument),
+                             "invalid argument '--" + Arg + "=" + Value +
+                                 "': " + Error);
+  return std::move(FM);
+}
+
+Expected<std::optional<FilterMatcher>>
+FilterMatcher::createExactOrRE(const llvm::cl::opt<std::string> &ExactArg,
+                               const llvm::cl::opt<std::string> &REArg) {
+  if (!ExactArg.empty() && !REArg.empty())
+    return createStringError(make_error_code(std::errc::invalid_argument),
+                             "conflicting arguments: --" + ExactArg.ArgStr +
+                                 " and --" + REArg.ArgStr);
+
+  if (!ExactArg.empty())
+    return createExact(ExactArg);
+
+  if (!REArg.empty())
+    return createRE(REArg);
+
+  return std::nullopt;
+}
+
 } // namespace remarks
 } // namespace llvm
diff --git a/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h b/llvm/tools/llvm-remarkutil/RemarkUtilHelpers.h
@@ -15,9 +15,11 @@
 #include "llvm/Remarks/RemarkFormat.h"
 #include "llvm/Remarks/RemarkParser.h"
 #include "llvm/Remarks/YAMLRemarkSerializer.h"
+#include "llvm/Support/CommandLine.h"
 #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 +57,40 @@ 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) {}
+
+  static Expected<FilterMatcher> createRE(StringRef Arg, StringRef Value);
+
+public:
+  static FilterMatcher createExact(StringRef Filter) { return {Filter, false}; }
+
+  static Expected<FilterMatcher>
+  createRE(const llvm::cl::opt<std::string> &Arg);
+
+  static Expected<FilterMatcher> createRE(StringRef Filter,
+                                          const cl::list<std::string> &Arg);
+
+  static Expected<std::optional<FilterMatcher>>
+  createExactOrRE(const llvm::cl::opt<std::string> &ExactArg,
+                  const llvm::cl::opt<std::string> &REArg);
+
+  static FilterMatcher createAny() { return {".*", true}; }
+
+  bool match(StringRef StringToMatch) const {
+    if (IsRegex)
+      return FilterRE.match(StringToMatch);
+    return FilterStr == StringToMatch.trim().str();
+  }
+};
+
 } // namespace remarks
 } // namespace llvm
