Refactor link and compile into subcommand objects.

[jonmeow]

Note the purpose here is to make it simpler to add more subcommands, without adding a lot of things to Driver.

This creates a copy of CodegenOptions, but it was double-registered at present which felt odd. It's also fairly small right now. If this becomes an issue, maybe we can look into using optional for delayed initialization, or just go back to straight sharing.

[jonmeow]

[merged] Depends on #4300, currently just the "Refactor into subcommands" commit

[chandlerc]

Looking just at "Refactor link and compile into subcommand objects." commit, generally LG. Suggestion inline to fix the test failure.

[jonmeow]

Just to note, all dependencies are merged but this still needs approval.

[josh11b]

Would it make sense for CompileSubcommand and LinkSubcommand (or possibly even DriverSubcommand) to expose an API that takes b and a function that sets subcommand and does AddSubcommand instead of the current BuildOptions method?

Something like:

Suggested change

	b.AddSubcommand(CompileOptions::Info,
	[&](CommandLine::CommandBuilder& sub_b) {
	compile.BuildOptions(sub_b);
	sub_b.Do([&] { subcommand = &compile; });
	});
	b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
	link.BuildOptions(sub_b);
	sub_b.Do([&] { subcommand = &link; });
	});
	auto set_subcommand = [&](DriverSubcommand* selected) { subcommand = selected; };
	compile.AddSubcommand(b, set_subcommand);
	link.AddSubcommand(b, set_subcommand);

[jonmeow]

I've been mulling that, but would you be okay with it being split out to a different PR? I think I'd change the structure of things a little more, particularly without Info is tracked, in order to provide a good API for it.

(I'm also working on a third command to try to get a better handle on differences)

[jonmeow]

Actually, after playing a little -- something in the commandline framework stops this from easily working. I'm not sure what the issue with my code is, but this doesn't work:

--- a/toolchain/driver/compile_subcommand.h
+++ b/toolchain/driver/compile_subcommand.h
@@ -63,7 +63,14 @@ struct CompileOptions {
 // Implements the compile subcommand of the driver.
 class CompileSubcommand : public DriverSubcommand {
  public:
-  auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); }
+  auto AddSubcommand(CommandLine::CommandBuilder& b,
+                     std::function<void(DriverSubcommand&)> do_callback)
+      -> void {
+    b.AddSubcommand(CompileOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
+      options_.Build(sub_b);
+      sub_b.Do([&] { do_callback(*this); });
+    });
+  }

   auto Run(DriverEnv& driver_env) -> DriverResult override;

diff --git a/toolchain/driver/driver.cpp b/toolchain/driver/driver.cpp
index 434c3ddb7..ce2485b56 100644
--- a/toolchain/driver/driver.cpp
+++ b/toolchain/driver/driver.cpp
@@ -63,16 +63,11 @@ auto Options::Build(CommandLine::CommandBuilder& b) -> void {
       },
       [&](CommandLine::FlagBuilder& arg_b) { arg_b.Set(&verbose); });

-  b.AddSubcommand(CompileOptions::Info,
-                  [&](CommandLine::CommandBuilder& sub_b) {
-                    compile.BuildOptions(sub_b);
-                    sub_b.Do([&] { subcommand = &compile; });
-                  });
-
-  b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
-    link.BuildOptions(sub_b);
-    sub_b.Do([&] { subcommand = &link; });
-  });
+  auto do_callback = [&](DriverSubcommand& do_subcommand) {
+    subcommand = &do_subcommand;
+  };
+  compile.AddSubcommand(b, do_callback);
+  link.AddSubcommand(b, do_callback);

   b.RequiresSubcommand();
 }
diff --git a/toolchain/driver/link_subcommand.h b/toolchain/driver/link_subcommand.h
index 3a4013fa0..4a97b0310 100644
--- a/toolchain/driver/link_subcommand.h
+++ b/toolchain/driver/link_subcommand.h
@@ -30,7 +30,14 @@ struct LinkOptions {
 // Implements the link subcommand of the driver.
 class LinkSubcommand : public DriverSubcommand {
  public:
-  auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); }
+  auto AddSubcommand(CommandLine::CommandBuilder& b,
+                     std::function<void(DriverSubcommand&)> do_callback)
+      -> void {
+    b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
+      options_.Build(sub_b);
+      sub_b.Do([&] { do_callback(*this); });
+    });
+  }

   auto Run(DriverEnv& driver_env) -> DriverResult override;

If you see a bug here, let me know. Otherwise, I'd rather leave the code as-is.

[jonmeow]

Note, the crash here is opaque and has no meaningful stack.

#0 0x000056533ae26d9d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/usr/local/google/home/jperkins/.cache/bazel/_bazel_jperkins/85deb7d9d96f7e0e80b42618a55969d7/sandbox/linux-sandbox/9160/execroot/_main/bazel-out/k8-fastbuild/bin/toolchain/testing/file_test.runfiles/_main/toolchain/testing/file_test+0x753bd9d)
#1 0x000056533ae272db PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
#2 0x000056533ae249a6 llvm::sys::RunSignalHandlers() (/usr/local/google/home/jperkins/.cache/bazel/_bazel_jperkins/85deb7d9d96f7e0e80b42618a55969d7/sandbox/linux-sandbox/9160/execroot/_main/bazel-out/k8-fastbuild/bin/toolchain/testing/file_test.runfiles/_main/toolchain/testing/file_test+0x75399a6)
#3 0x000056533ae27ca5 SignalHandler(int) Signals.cpp:0:0
#4 0x00007f3d1dc8b1a0 (/lib/x86_64-linux-gnu/libc.so.6+0x3d1a0)
#5 0x000032e9bfe28290

[josh11b]

I don't know what the bug is. I'm fine with looking at it later (particularly if we can figure out how to get a better stack trace).