[Support]Look up in top-level subcommand as a fallback when looking options for a custom subcommand.

[minglotus-6]

Context:

• In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3, cl::SubCommand is introduced.
• Options that don't specify subcommand goes into a special 'top level' subcommand.

Motivating Use Case:

• The motivating use case is to refactor llvm-profdata to use cl::SubCommand to organize subcommands. See pr/71328. A valid use case that's not supported before this patch

  // show-option{1,2} are associated with 'show' subcommand.
  // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp)
  llvm-profdata show --show-option1 --show-option2 --top-level-option3

• Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3".
• After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Mingming Liu (minglotus-6)

Changes

Context:

• In https://lists.llvm.org/pipermail/llvm-dev/2016-June/101804.html and commit 07670b3, cl::SubCommand is introduced.
• Options that don't specify subcommand goes intoa special 'top level' subcommand.

Motivating Use Case:

• The motivating use case is to refactor llvm-profdata to use cl::SubCommand to organize subcommands. See pr/71328. A valid use case that's not supported before this patch

  // show-option{1,2} are associated with 'show' subcommand.
  // top-level-option3 is in top-level subcomand (e.g., `profile-isfs` in SampleProfReader.cpp)
  llvm-profdata show --show-option1 --show-option2 --top-level-option3

• Before this patch, option handler look-up will fail with the following error message "Unknown command line argument --top-level-option3".
• After this patch, option handler look-up will look up in sub-command options first, and use top-level subcommand as a fallback, so 'top-level-option3' is parsed correctly.

---

Full diff: https://github.com/llvm/llvm-project/pull/71776.diff

2 Files Affected:

• (modified) llvm/lib/Support/CommandLine.cpp (+7)
• (modified) llvm/unittests/Support/CommandLineTest.cpp (+68)

diff --git a/llvm/lib/Support/CommandLine.cpp b/llvm/lib/Support/CommandLine.cpp
index 55633d7cafa4791..a7e0cae8b855d7c 100644
--- a/llvm/lib/Support/CommandLine.cpp
+++ b/llvm/lib/Support/CommandLine.cpp
@@ -1667,6 +1667,13 @@ bool CommandLineParser::ParseCommandLineOptions(int argc,
       Handler = LookupLongOption(*ChosenSubCommand, ArgName, Value,
                                  LongOptionsUseDoubleDash, HaveDoubleDash);
 
+      // If Handler is not found in a specialized subcommand, look up handler
+      // in the top-level subcommand.
+      // cl::opt without cl::sub belongs to top-level subcommand.
+      if (!Handler && ChosenSubCommand != &SubCommand::getTopLevel())
+        Handler = LookupLongOption(SubCommand::getTopLevel(), ArgName, Value,
+                                   LongOptionsUseDoubleDash, HaveDoubleDash);
+
       // Check to see if this "option" is really a prefixed or grouped argument.
       if (!Handler && !(LongOptionsUseDoubleDash && HaveDoubleDash))
         Handler = HandlePrefixedOrGroupedOption(ArgName, Value, ErrorParsing,
diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index 41cc8260acfedf7..76589c7deed88e1 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -525,6 +525,74 @@ TEST(CommandLineTest, LookupFailsInWrongSubCommand) {
   EXPECT_FALSE(Errs.empty());
 }
 
+TEST(CommandLineTest, SubcommandOptions) {
+  enum LiteralOptionEnum {
+    foo,
+    bar,
+    baz,
+  };
+
+  cl::ResetCommandLineParser();
+
+  // This is a top-level option and not associated with a subcommand.
+  // A command line using subcommand should parse both subcommand options and
+  // top-level options.  A valid use case is that users of llvm command line
+  // tools should be able to specify top-level options defined in any library.
+  cl::opt<std::string> TopLevelOpt("str", cl::init("txt"),
+                                   cl::desc("A top-level option."));
+
+  StackSubCommand SC("sc", "Subcommand");
+
+  // The positional argument.
+  StackOption<std::string> PositionalOpt(
+      cl::Positional, cl::desc("positional argument test coverage"),
+      cl::sub(SC));
+  // Thel literal argument.
+  StackOption<LiteralOptionEnum> LiteralOpt(
+      cl::desc("literal argument test coverage"), cl::sub(SC), cl::init(bar),
+      cl::values(clEnumVal(foo, "foo"), clEnumVal(bar, "bar"),
+                 clEnumVal(baz, "baz")));
+  StackOption<bool> BoolOpt("enable", cl::sub(SC), cl::init(false));
+
+  std::string Errs;
+  raw_string_ostream OS(Errs);
+
+  for (const char *literalArg : {"--bar", "--foo", "--baz"}) {
+    const char *args[] = {"prog",     "sc",      "input-file",
+                          literalArg, "-enable", "--str=csv"};
+
+    // cl::ParseCommandLineOptions returns true on success. it returns false
+    // and prints errors to caller provided error stream (&OS in this setting).
+    EXPECT_TRUE(cl::ParseCommandLineOptions(6, args, StringRef(), &OS));
+
+    // Tests that the value of `str` option is `csv` as specified.
+    EXPECT_EQ(strcmp(TopLevelOpt.getValue().c_str(), "csv"), 0);
+
+    const char *parsedLiteralOpt;
+    switch (LiteralOpt) {
+    case baz:
+      parsedLiteralOpt = "baz";
+      break;
+    case bar:
+      parsedLiteralOpt = "bar";
+      break;
+    case foo:
+      parsedLiteralOpt = "foo";
+      break;
+    default:
+      llvm_unreachable("unknown option for LiteralOpt");
+    }
+
+    // Tests that literal options are parsed correctly. Skip '--' prefix of
+    // literalArg.
+    EXPECT_EQ(strcmp(parsedLiteralOpt, literalArg + 2), 0);
+
+    // Flush and tests there is no error message.
+    OS.flush();
+    EXPECT_TRUE(Errs.empty());
+  }
+}
+
 TEST(CommandLineTest, AddToAllSubCommands) {
   cl::ResetCommandLineParser();
 

[snehasish]

If you structure this loop to iterate over a list of {input, want} pairs then you can avoid the switch case below and the conversion for checking.

for (auto& test : {std::pair{"--bar", bar}, {"--foo", foo}, {"--baz", baz}) {
  ...
  EXPECT_EQ(LiteralOpt, test.second);
}

[minglotus-6]

good point, done. Also move the loop-invariant test case out of the loop as a part of this commit.

[snehasish]

nit: sizeof(args) instead of 6

[minglotus-6]

done.

[snehasish]

EXPECT_STREQ instead of EXPECT_EQ to avoid the strcmp.

[minglotus-6]

done.

[snehasish]

I think this is unnecessary since we already check the return value of cl::ParseCommandLineOptions?

[minglotus-6]

Yes, I removed Errs and OS in this test case.

[snehasish]

This should be ASSERT_TRUE since it does not make sense to continue if we failed to parse this arg. Though note that it will fail the entire test and the rest of the tests will not be executed (which imo is fine).

[minglotus-6]

yep ASSERT_TRUE is better in this context. done.

[snehasish]

lgtm though please wait a bit for feedback from @MaskRay

[MaskRay]

Add another StackOption<bool> similar to -enable, and test that the option can be parsed after --str=csv

[minglotus-6]

sounds reasonable. Make a slight change by adding an option of StackOption<int>.

[MaskRay]

There are other Subcommand tests. The name SubcommandOptions can be changed to reflect that the test intends to check, e.g. TopLevelOptInSubcommand

[MaskRay]

The code self explains. I think the comments are unneeded.

[minglotus-6]

delete two one-line comments.