[llvm] Add subcommand support for OptTable by Prabhuk · Pull Request #155026 · llvm/llvm-project

Prabhuk · 2025-08-22T20:54:50Z

Implement support for subcommands in OptTable to attain feature parity with cl.

Design overview: https://discourse.llvm.org/t/subcommand-feature-support-in-llvm-opttable/88098

TODO: Add tests.

PiJoules · 2025-09-05T20:34:30Z

+  HelloSubOptTable T;
+  unsigned MissingArgIndex, MissingArgCount;
+
+  auto HandleMultipleSubcommands = [](const ArrayRef<StringRef> SubCommands) {


const might not be needed since ArrayRef is already immutable I believe. Same with the other const ArrayRefs below.

Done. PTAL.

PiJoules · 2025-09-05T20:50:08Z

+    // The option CommandIDsOffset.
+    OS << ", ";
+    if (R.getValue("CommandGroup") != nullptr) {
+      std::vector<const Record *> CommandGroup =
+          R.getValueAsListOfDefs("CommandGroup");
+      CommandKeyT CommandKey;
+      for (const auto &Command : CommandGroup)
+        CommandKey.push_back(Command->getName());


Maybe move this into its own function since it's reused below.

Done. PTAL.

PiJoules · 2025-09-05T20:52:38Z

+    for (auto SC : SubCommands) {
+      llvm::errs() << " `" << SC << "`\n";
+    }


Omit braces for single-line bodies: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Same for other bodies below

Done. PTAL.

PiJoules · 2025-09-05T20:59:43Z

  OptTable(const StringTable &StrTable,
           ArrayRef<StringTable::Offset> PrefixesTable,
           ArrayRef<Info> OptionInfos, bool IgnoreCase = false);


Could probably just add

ArrayRef<Command> Commands = {}, ArrayRef<unsigned> CommandIDsTable = {},

so you don't need to make another ctor. Same with other OpTables below.

Done. PTAL.

PiJoules · 2025-09-05T21:16:21Z

+  }
+  if (SubCommands.size() == 1)
+    return SubCommands.front();
+  return SubCommand;


I think SubCommand is still just {} here so maybe just return {}.

Done. PTAL.

PiJoules · 2025-09-05T21:17:45Z

+      std::function<void(ArrayRef<StringRef>)> HandleMultipleSubcommands,
+      std::function<void(ArrayRef<StringRef>)> HandleOtherPositionals) const;


Maybe add comments above briefly explaining what these callbacks are for. Mostly requesting since the implication to me (based on the example usage) is that a user might want to just throw an error and exit. If this is the primary/only desirable case, then that might be worth noting. I personally can't think of a reason one might not want to print help and exit, but maybe someone else might have a reason to. We could probably discourage that with comments or maybe saying the callback is NoReturn if we want.

Done. PTAL.

PiJoules · 2025-09-05T21:41:51Z

+      for (const auto &C : Commands) {
+        if (FirstArg == C.Name) {
+          ActiveCommand = &C;
+          break;
+        }
+      }


nit: Just a personal preference so feel free to ignore, but I think this looks cleaner

auto found = std::find_if(Commands.begin(), Commands.end(), [&](const auto &C){ return FirstArg == C.Name }); ActiveCommand = found == Commands.end() ? nullptr : *found;

Alternatively, it looks like you have getActiveCommand below so maybe you could reuse it here.

Reusing getActiveCommand. Using find_if in getActiveCommand implementation.
Done. PTAL.

PiJoules · 2025-09-05T21:48:31Z

+      // This loop prints subcommands list and sets ActiveCommand to
+      // TopLevelCommand while iterating over all commands.
+      for (const auto &C : Commands) {
+        if (C.Name == TopLevelCommandName) {


Coud probably just do C.Name == "TopLevelCommandName" if it's not changed elsewhere and stick it as a constexpr global in OptTable.h if it's used elsewhere.

warning: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Wstring-compare] 804 | if (C.Name == "TopLevelCommand") { | ^ ~~~~~~~~~~~~~~~~~

Done. PTAL.

PiJoules · 2025-09-05T21:51:44Z

+      assert((CurIndex == 0 || !Command.empty()) &&
+             "Only first command set should be empty!");
+      for (const auto &CommandKey : Command) {
+        auto It = llvm::find_if(Commands, [&](const Record *R) {


We can use std::find_if directly. I think llvm::find_if just calls it.

Done. PTAL.

Prabhuk · 2025-09-15T23:37:41Z

@PiJoules -- Thank you for the review! I've addressed all the comments from the last version you reviewed. PTAL.

PiJoules · 2025-09-17T16:43:16Z

+class HelloSubOptTable : public GenericOptTable {
+public:
+  HelloSubOptTable()
+      : GenericOptTable(OptionStrTable, OptionPrefixesTable, InfoTable, false,


https://llvm.org/docs/CodingStandards.html#comment-formatting

/*IgnoreCase=*/false

Done. Thank you.

PiJoules · 2025-09-17T16:44:13Z

+  if (Subcommand.empty()) {
+    if (Args.hasArg(OPT_version)) {
+      llvm::outs() << "LLVM Hello Subcommand Example 1.0\n";
+    }


https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

if (Args.hasArg(OPT_version)) llvm::outs() << "LLVM Hello Subcommand Example 1.0\n";

Done. Thank you.

PiJoules · 2025-09-17T16:55:54Z

+
+    size_t OldSize = SubCommands.size();
+    for (const OptTable::Command &CMD : Commands) {
+      if (StringRef(CMD.Name) == "TopLevelCommand")


Maybe extract "TopLevelCommand" into a header and make it a global rather than using it as a literal between different cpp.

Done. Thank you. It's added to OptTable.h file

PiJoules · 2025-09-17T16:58:01Z

+// Compile time representation for top level command (aka toolname).
+// Offers backward compatibility with existing Option class definitions before
+// introduction of commandGroup in Option class to support subcommands.
+def TopLevelCommand : Command<"TopLevelCommand">;


What happens if a user would define a Subcommand with the name "TopLevelCommand"? I'm assuming there should only be one with this name assuming it's like a sentinel. If only one should be defined, are there any checks that would assert this or diagnose if more than one was provided?

That's a good point. I'll add the check. Maybe it's worth changing the name to something that is probably not going to be useful as a user defined subcommand name. "TopLevelCommand" -> "TOPLEVELCOMMAND". Other option could be a "Command" type object with empty string for name to depict the top level command. I can make sure all the subcommands have a non empty string name.

Ok I did a refactoring which changes the design a bit. There is no abstraction for "TopLevelCommand" anymore which I always felt weird about maintaining. Now we only have subcommands that can be registered by the user. All regression tests pass and my llvm-hello-sub example behaves correctly. PTAL. I am working on adding more tests to cover subcommand use cases.

PiJoules · 2025-09-17T17:01:34Z

-  return internalParseOneArg(Args, Index, [VisibilityMask](const Option &Opt) {
-    return !Opt.hasVisibilityFlag(VisibilityMask);
-  });
+  return internalParseOneArg(Args, Index, nullptr,


/*ActiveCommand=*/nullptr

Done. Thank you.

PiJoules · 2025-09-17T17:01:39Z

                                           unsigned FlagsToExclude) const {
  return internalParseOneArg(
-      Args, Index, [FlagsToInclude, FlagsToExclude](const Option &Opt) {
+      Args, Index, nullptr,


Done. Thank you.

PiJoules · 2025-09-17T17:03:31Z

  FlagsToExclude &= ~HelpHidden;
  return internalPrintHelp(
-      OS, Usage, Title, ShowHidden, ShowAllAliases,
+      OS, Usage, Title, {}, ShowHidden, ShowAllAliases,


/*Subcommand=*/{}

Done. Thank you.

github-actions · 2025-09-29T23:34:07Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ion.

PiJoules

LGTM thanks

PiJoules · 2025-10-02T18:20:23Z

+                            StringRef SubCommand) const {
+    assert(!SubCommand.empty() &&
+           "This helper is only for valid registered subcommands.");
+    typename ArrayRef<OptTable::SubCommand>::iterator SCIT =


Can probably save a line and just use auto here since it's probably well known that std::find* functions return an iterator.

Done. But it didn't save a line unfortunately :)

PiJoules · 2025-10-02T18:25:33Z

  /// Return the string table used for option names.
  const StringTable &getStrTable() const { return *StrTable; }

+  const ArrayRef<SubCommand> getSubCommands() const { return SubCommands; }


ArrayRef is already immutable so const might not be needed here.

PiJoules · 2025-10-02T18:26:21Z

  // pairs.
  std::map<std::string, std::vector<OptionInfo>> GroupedOptionHelp;

+  typename ArrayRef<OptTable::SubCommand>::iterator ActiveSubCommand =


Can probably use auto here also

Prabhuk · 2025-10-02T20:47:21Z

LGTM thanks

Thank you @PiJoules I'll wait for a day to see if there are any objections before landing.

this is for unblocking eld upstream : llvm/llvm-project#155026 Signed-off-by: Shankar Easwaran <seaswara@quicinc.com>

petrhosek · 2025-10-06T23:49:11Z

Rather than having a directory in examples dedicated to just subcommands, I think we should a directory for Option and subcommands should be just one of the examples in there.

I'll start a new PR for that change.

llvm-ci · 2025-10-07T00:11:05Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-darwin running on doug-worker-3 while building clang-tools-extra,clang,lld,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/14465

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang-Unit :: ./AllClangUnitTests/17/48' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Volumes/RAMDisk/buildbot-root/x86_64-darwin/build/tools/clang/unittests/./AllClangUnitTests-Clang-Unit-41567-17-48.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=48 GTEST_SHARD_INDEX=17 /Volumes/RAMDisk/buildbot-root/x86_64-darwin/build/tools/clang/unittests/./AllClangUnitTests
--

Note: This is test shard 18 of 48.
[==========] Running 509 tests from 107 test suites.
[----------] Global test environment set-up.
[----------] 1 test from MinimizeSourceToDependencyDirectivesTest
[ RUN      ] MinimizeSourceToDependencyDirectivesTest.DefineNumber
[       OK ] MinimizeSourceToDependencyDirectivesTest.DefineNumber (0 ms)
[----------] 1 test from MinimizeSourceToDependencyDirectivesTest (0 ms total)

[----------] 1 test from HeaderSearchTest
[ RUN      ] HeaderSearchTest.ShortenWithWorkingDir
[       OK ] HeaderSearchTest.ShortenWithWorkingDir (0 ms)
[----------] 1 test from HeaderSearchTest (0 ms total)

[----------] 1 test from ModuleDeclStateTest
[ RUN      ] ModuleDeclStateTest.ModuleInterfacePartition
[       OK ] ModuleDeclStateTest.ModuleInterfacePartition (1 ms)
[----------] 1 test from ModuleDeclStateTest (1 ms total)

[----------] 1 test from ParseHLSLRootSignatureTest
[ RUN      ] ParseHLSLRootSignatureTest.InvalidMissingRDParameterTest
[       OK ] ParseHLSLRootSignatureTest.InvalidMissingRDParameterTest (0 ms)
[----------] 1 test from ParseHLSLRootSignatureTest (1 ms total)

[----------] 1 test from ToolChainTest
[ RUN      ] ToolChainTest.VFSSolarisMultiGCCInstallation
[       OK ] ToolChainTest.VFSSolarisMultiGCCInstallation (4 ms)
[----------] 1 test from ToolChainTest (4 ms total)

[----------] 1 test from MultilibTest
[ RUN      ] MultilibTest.SelectSoftFP
[       OK ] MultilibTest.SelectSoftFP (0 ms)
[----------] 1 test from MultilibTest (0 ms total)

[----------] 2 tests from ExprMutationAnalyzerTest
[ RUN      ] ExprMutationAnalyzerTest.ReturnAsNonConstRRef
input.cc:1:47: warning: reference to stack memory associated with local variable 'x' returned [-Wreturn-stack-address]
    1 | int&& f() { int x; return static_cast<int &&>(x); }
      |                                               ^
[       OK ] ExprMutationAnalyzerTest.ReturnAsNonConstRRef (19 ms)
[ RUN      ] ExprMutationAnalyzerTest.SelfRef
[       OK ] ExprMutationAnalyzerTest.SelfRef (9 ms)
[----------] 2 tests from ExprMutationAnalyzerTest (28 ms total)

[----------] 1 test from LifetimeAnalysisTest
...

llvm-ci · 2025-10-07T06:34:35Z

LLVM Buildbot has detected a new failure on builder ppc64le-lld-multistage-test running on ppc64le-lld-multistage-test while building clang-tools-extra,clang,lld,llvm at step 12 "build-stage2-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16524

Here is the relevant piece of the build log for the reference

Step 12 (build-stage2-unified-tree) failure: build (failure) (timed out)
...
724.765 [57/9/6658] Linking CXX executable unittests/Target/TargetMachineCTests
725.171 [57/8/6659] Linking CXX executable bin/llvm-split
725.172 [57/7/6660] Linking CXX executable bin/llc
725.682 [57/6/6661] Linking CXX executable bin/llvm-dwp
728.082 [57/5/6662] Linking CXX executable tools/lld/unittests/AsLibAll/LLDAsLibAllTests
728.501 [57/4/6663] Linking CXX executable bin/bugpoint
730.815 [56/4/6664] Linking CXX executable bin/opt
737.613 [56/3/6665] Linking CXX executable bin/lld
752.341 [56/2/6666] Building CXX object tools/bugpoint-passes/CMakeFiles/BugpointPasses.dir/TestPasses.cpp.o
755.454 [55/2/6667] Linking CXX shared module lib/BugpointPasses.so
command timed out: 1200 seconds without output running [b'ninja'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1956.112859

## What changed Upgrades the prebuilt LLVM dependency from 21.1.8 to 22.1.8 and adapts clice to the LLVM 22 API. Highlights: - **NNS redesign / ElaboratedType removal** (llvm/llvm-project#147835): `NestedNameSpecifier` is a value type and types carry their elaborated keyword and qualifier themselves. Rewrote NNS handling in the template resolver, semantic visitor, unifier, display, and USR generation; `rewrite_specifier` collapses to a single type-component rewrite since prefixes now live inside type nodes. - **DependentTemplateSpecializationType removed** (llvm/llvm-project#158109): dependent specializations are `TemplateSpecializationType`s with a `DependentTemplateName`. The resolver's DTST lookup/rewrite/pseudo-SFINAE paths merged into the TST paths. - **CompilerInstance owns its VFS** (llvm/llvm-project#158381): compilation and scan now seed the instance VFS before creating diagnostics and the file manager. - **clangOptions split** (llvm/llvm-project#167374) and the new `SUBCOMMANDIDS_OFFSET` OPTION column (llvm/llvm-project#155026): include-path and macro updates in the argument parser. - Assorted renames: `getCanonicalTagType`, `getCanonicalTemplateSpecializationType`, `UsingType::getDecl`, `sys::path::make_absolute`, `clang::GetResourcesPath`. Behavioral changes visible in features (all matching clang/clangd 22 rendering, pinned in snapshots): `__size_t (aka unsigned long)` sugar, `(unnamed enum)` naming, namespace-qualified canonical class types in hover, and converted (qualified) template arguments in hover titles for implicit variable template specializations. One deliberate divergence from clangd: inlay type hints keep written class scopes (`S2::Nested<int>`) that LLVM 22's `SuppressScope` would now drop — restored by printing the outer node's written qualifier. Also cherry-picks the release-triple normalization + prebuilt-respin fixes (`/MT`, macOS deployment target) that the 22.1.8 prebuilt was built with, and adds an `LLVM 21 → 22` section to the LLVM changelog documenting every breaking change with upstream references. Note: CI stays red until the pruned 22.1.8 `clice-llvm` release is published (release-llvm is running against this branch); the version pin in `cmake/package.cmake` will be bumped in a follow-up commit on this branch once it is up. ## Tests All four suites pass locally against the 22.1.8 prebuilt: unit (1174), integration (341), smoke (3/3), snap (393). Snapshot updates are limited to the upstream rendering changes listed above; one selection-tree unit test was re-marked because dependent qualifier chains are now `DependentNameTypeLoc` components with name-only ranges.

[OptTable] Subcommand support.

3584c6a

TODO: Add tests.

Prabhuk requested review from MaskRay and petrhosek August 22, 2025 20:55

Prabhuk added 9 commits August 22, 2025 22:04

Cleanup internalPrintHelp code

8665d9e

Fix formatting

d485046

Add custom usage string to tablegen for each subcommand

b325c78

Add todo

61334ef

Make subcommand usage text optional

1690f3b

Make subcommand's position flexible and not just as first argument.

9c419d9

Handle unknown subcommands in a graceful manner.

19e6437

Handle more than one subcommand passed in commandline.

8d5363b

Handle subcommand errors in parsing through lambdas.

51ef95e

Prabhuk marked this pull request as ready for review August 27, 2025 22:05

Prabhuk requested a review from cyndyishida as a code owner August 27, 2025 22:05

Prabhuk requested a review from jh7370 August 27, 2025 22:05

Prabhuk requested a review from PiJoules September 5, 2025 19:07

PiJoules reviewed Sep 5, 2025

View reviewed changes

Prabhuk added 6 commits September 15, 2025 21:19

Address review comments.

043bbc8

Fix initialization order of IgnoreCase.

bc8bd1b

Address review comments 2/

dcb6779

Address review comments 3/

e7ea647

Update inline comment.

ff8034a

Address review comments 4/

e99a9a0

PiJoules reviewed Sep 17, 2025

View reviewed changes

Prabhuk added 5 commits September 23, 2025 09:44

Merge branch 'main' into opttable_subcommands_working

a480c0c

Address review comments.

d41c2ae

Remove TopLevelCommand abstraction completely.

c1cddc9

Update variable names to reflect there is no Toplevelcommand.

e28d2b0

Update macro definition variable names

3bc51b7

Prabhuk added 2 commits September 29, 2025 23:29

Fix formatting.

5c31cea

Unfix unnecessary formatting.

5995d18

Prabhuk added 8 commits September 29, 2025 23:34

Remove empty lines and braces on single line loop body.

064868a

Fix unittest to be more verbose.

c5a0cf5

Format.

dfc592e

Fix the SubCommands find_if return type.

8d11673

Introduce API to check if a subcommand is registered with a given opt…

db25a6a

…ion.

Make API name consistent.

ebdb5b8

Add test to handle unregistered subcommand option case.

3d7c81e

Merge branch 'main' into opttable_subcommands_working

845a073

PiJoules approved these changes Oct 2, 2025

View reviewed changes

Address review comments.

396a041

Prabhuk merged commit fdbd17d into llvm:main Oct 6, 2025
9 checks passed

rupprecht added a commit that referenced this pull request Oct 6, 2025

[bazel] Port #155026: OptTable support for subcommands (#162150)

087c832

quic-seaswara added a commit to qualcomm/eld that referenced this pull request Oct 6, 2025

fix break with llvm tip

2da9314

this is for unblocking eld upstream : llvm/llvm-project#155026 Signed-off-by: Shankar Easwaran <seaswara@quicinc.com>

quic-seaswara mentioned this pull request Oct 6, 2025

fix break with llvm tip qualcomm/eld#449

Merged

quic-seaswara added a commit to qualcomm/eld that referenced this pull request Oct 6, 2025

fix break with llvm tip

dab4e28

this is for unblocking eld upstream : llvm/llvm-project#155026 Signed-off-by: Shankar Easwaran <seaswara@quicinc.com>

petrhosek reviewed Oct 6, 2025

View reviewed changes

petrhosek mentioned this pull request Oct 7, 2025

[llvm-driver] Remove llvm-profdata from the driver #162191

Merged

Prabhuk deleted the opttable_subcommands_working branch October 21, 2025 16:56

ilovepi mentioned this pull request Nov 3, 2025

[CAS] Add llvm-cas tools to inspect on-disk LLVMCAS #114104

Merged

petrhosek mentioned this pull request Nov 5, 2025

[CAS] Add llvm-cas tools to inspect on-disk LLVMCAS #166481

Merged

cachemeifyoucan mentioned this pull request Nov 6, 2025

OptTable SubCommand is not usable with positional command-line arguments #166830

Open

petrhosek mentioned this pull request Jan 26, 2026

Migrate llvm-profdata to Opt flag parsing #177868

Open

16bit-ykiko mentioned this pull request Aug 7, 2026

chore: upgrade LLVM to 22.1.8 clice-io/clice#586

Merged

		std::function<void(ArrayRef<StringRef>)> HandleMultipleSubcommands,
		std::function<void(ArrayRef<StringRef>)> HandleOtherPositionals) const;

Conversation

Prabhuk commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prabhuk commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Prabhuk commented Aug 22, 2025 •

edited

Loading

github-actions Bot commented Sep 29, 2025 •

edited

Loading

petrhosek Oct 6, 2025 •

edited

Loading