From c029931910ceed0e7ec8801889bb4efa8c28f361 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 13 Sep 2024 20:16:53 -0700 Subject: [PATCH] Refactor link and compile into subcommand objects. (#4303) 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. --------- Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com> --- toolchain/driver/BUILD | 5 +- toolchain/driver/compile_subcommand.cpp | 93 ++++++++++++------------- toolchain/driver/compile_subcommand.h | 22 +++++- toolchain/driver/driver.cpp | 87 +++++++++++------------ toolchain/driver/driver.h | 42 +---------- toolchain/driver/driver_subcommand.h | 33 +++++++++ toolchain/driver/link_subcommand.cpp | 21 +++--- toolchain/driver/link_subcommand.h | 17 ++++- 8 files changed, 164 insertions(+), 156 deletions(-) create mode 100644 toolchain/driver/driver_subcommand.h diff --git a/toolchain/driver/BUILD b/toolchain/driver/BUILD index da356241f6c0..2fab283928d1 100644 --- a/toolchain/driver/BUILD +++ b/toolchain/driver/BUILD @@ -86,7 +86,10 @@ cc_library( "link_subcommand.cpp", "link_subcommand.h", ], - hdrs = ["driver.h"], + hdrs = [ + "driver.h", + "driver_subcommand.h", + ], data = [ "//toolchain/install:install_lib_data", ], diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp index 6d06ed5a4fe4..98891e1e34c2 100644 --- a/toolchain/driver/compile_subcommand.cpp +++ b/toolchain/driver/compile_subcommand.cpp @@ -17,9 +17,6 @@ #include "toolchain/sem_ir/inst_namer.h" #include "toolchain/source/source_buffer.h" -// TODO: Remove this include. -#include "toolchain/driver/driver.h" - namespace Carbon { auto operator<<(llvm::raw_ostream& out, CompileOptions::Phase phase) @@ -59,8 +56,7 @@ can be written to standard output as these phases progress. )""", }; -auto CompileOptions::Build(CommandLine::CommandBuilder& b, - CodegenOptions& codegen_options) -> void { +auto CompileOptions::Build(CommandLine::CommandBuilder& b) -> void { b.AddStringPositionalArg( { .name = "FILE", @@ -268,34 +264,33 @@ Emit DWARF debug information. }); } -auto Driver::ValidateCompileOptions(const CompileOptions& options) const - -> bool { +auto CompileSubcommand::ValidateOptions(DriverEnv& driver_env) const -> bool { using Phase = CompileOptions::Phase; - switch (options.phase) { + switch (options_.phase) { case Phase::Lex: - if (options.dump_parse_tree) { - driver_env_.error_stream + if (options_.dump_parse_tree) { + driver_env.error_stream << "ERROR: Requested dumping the parse tree but compile " "phase is limited to '" - << options.phase << "'.\n"; + << options_.phase << "'.\n"; return false; } [[fallthrough]]; case Phase::Parse: - if (options.dump_sem_ir) { - driver_env_.error_stream + if (options_.dump_sem_ir) { + driver_env.error_stream << "ERROR: Requested dumping the SemIR but compile phase " "is limited to '" - << options.phase << "'.\n"; + << options_.phase << "'.\n"; return false; } [[fallthrough]]; case Phase::Check: - if (options.dump_llvm_ir) { - driver_env_.error_stream + if (options_.dump_llvm_ir) { + driver_env.error_stream << "ERROR: Requested dumping the LLVM IR but compile " "phase is limited to '" - << options.phase << "'.\n"; + << options_.phase << "'.\n"; return false; } [[fallthrough]]; @@ -307,16 +302,17 @@ auto Driver::ValidateCompileOptions(const CompileOptions& options) const return true; } +namespace { // Ties together information for a file being compiled. -class Driver::CompilationUnit { +// TODO: Refactor this because it's a long class to have function definitions +// inline. +class CompilationUnit { public: explicit CompilationUnit(DriverEnv& driver_env, const CompileOptions& options, - const CodegenOptions& codegen_options, DiagnosticConsumer* consumer, llvm::StringRef input_filename) : driver_env_(&driver_env), options_(options), - codegen_options_(codegen_options), input_filename_(input_filename), vlog_stream_(driver_env_->vlog_stream) { if (vlog_stream_ != nullptr || options_.stream_errors) { @@ -497,7 +493,7 @@ class Driver::CompilationUnit { // Do codegen. Returns true on success. auto RunCodeGenHelper() -> bool { std::optional codegen = CodeGen::Make( - *module_, codegen_options_.target, driver_env_->error_stream); + *module_, options_.codegen_options.target, driver_env_->error_stream); if (!codegen) { return false; } @@ -593,7 +589,6 @@ class Driver::CompilationUnit { DriverEnv* driver_env_; SharedValueStores value_stores_; const CompileOptions& options_; - const CodegenOptions& codegen_options_; std::string input_filename_; // Copied from driver_ for CARBON_VLOG. @@ -617,10 +612,10 @@ class Driver::CompilationUnit { std::unique_ptr llvm_context_; std::unique_ptr module_; }; +} // namespace -auto Driver::Compile(const CompileOptions& options, - const CodegenOptions& codegen_options) -> RunResult { - if (!ValidateCompileOptions(options)) { +auto CompileSubcommand::Run(DriverEnv& driver_env) -> DriverResult { + if (!ValidateOptions(driver_env)) { return {.success = false}; } @@ -628,33 +623,31 @@ auto Driver::Compile(const CompileOptions& options, // TODO: Replace this with a search for library api files in a // package-specific search path based on the library name. llvm::SmallVector prelude; - if (options.prelude_import && options.phase >= CompileOptions::Phase::Check) { - if (auto find = driver_env_.installation->ReadPreludeManifest(); - find.ok()) { + if (options_.prelude_import && + options_.phase >= CompileOptions::Phase::Check) { + if (auto find = driver_env.installation->ReadPreludeManifest(); find.ok()) { prelude = std::move(*find); } else { - driver_env_.error_stream << "ERROR: " << find.error() << "\n"; + driver_env.error_stream << "ERROR: " << find.error() << "\n"; return {.success = false}; } } // Prepare CompilationUnits before building scope exit handlers. - StreamDiagnosticConsumer stream_consumer(driver_env_.error_stream); + StreamDiagnosticConsumer stream_consumer(driver_env.error_stream); llvm::SmallVector> units; - units.reserve(prelude.size() + options.input_filenames.size()); + units.reserve(prelude.size() + options_.input_filenames.size()); // Add the prelude files. for (const auto& input_filename : prelude) { - units.push_back( - std::make_unique(driver_env_, options, codegen_options, - &stream_consumer, input_filename)); + units.push_back(std::make_unique( + driver_env, options_, &stream_consumer, input_filename)); } // Add the input source files. - for (const auto& input_filename : options.input_filenames) { - units.push_back( - std::make_unique(driver_env_, options, codegen_options, - &stream_consumer, input_filename)); + for (const auto& input_filename : options_.input_filenames) { + units.push_back(std::make_unique( + driver_env, options_, &stream_consumer, input_filename)); } auto on_exit = llvm::make_scope_exit([&]() { @@ -667,9 +660,9 @@ auto Driver::Compile(const CompileOptions& options, stream_consumer.Flush(); }); - // Returns a RunResult object. Called whenever Compile returns. + // Returns a DriverResult object. Called whenever Compile returns. auto make_result = [&]() { - RunResult result = {.success = true}; + DriverResult result = {.success = true}; for (const auto& unit : units) { result.success &= unit->success(); result.per_file_success.push_back( @@ -682,7 +675,7 @@ auto Driver::Compile(const CompileOptions& options, for (auto& unit : units) { unit->RunLex(); } - if (options.phase == CompileOptions::Phase::Lex) { + if (options_.phase == CompileOptions::Phase::Lex) { return make_result(); } // Parse and check phases examine `has_source` because they want to proceed if @@ -695,7 +688,7 @@ auto Driver::Compile(const CompileOptions& options, unit->RunParse(); } } - if (options.phase == CompileOptions::Phase::Parse) { + if (options_.phase == CompileOptions::Phase::Parse) { return make_result(); } @@ -713,24 +706,24 @@ auto Driver::Compile(const CompileOptions& options, node_converters.emplace_back(unit.tokens, unit.tokens->source().filename(), unit.get_parse_tree_and_subtrees); } - CARBON_VLOG_TO(driver_env_.vlog_stream, "*** Check::CheckParseTrees ***\n"); - Check::CheckParseTrees(check_units, node_converters, options.prelude_import, - driver_env_.vlog_stream); - CARBON_VLOG_TO(driver_env_.vlog_stream, + CARBON_VLOG_TO(driver_env.vlog_stream, "*** Check::CheckParseTrees ***\n"); + Check::CheckParseTrees(check_units, node_converters, options_.prelude_import, + driver_env.vlog_stream); + CARBON_VLOG_TO(driver_env.vlog_stream, "*** Check::CheckParseTrees done ***\n"); for (auto& unit : units) { if (unit->has_source()) { unit->PostCheck(); } } - if (options.phase == CompileOptions::Phase::Check) { + if (options_.phase == CompileOptions::Phase::Check) { return make_result(); } // Unlike previous steps, errors block further progress. if (std::any_of(units.begin(), units.end(), [&](const auto& unit) { return !unit->success(); })) { - CARBON_VLOG_TO(driver_env_.vlog_stream, + CARBON_VLOG_TO(driver_env.vlog_stream, "*** Stopping before lowering due to errors ***"); return make_result(); } @@ -741,10 +734,10 @@ auto Driver::Compile(const CompileOptions& options, &**unit->GetCheckUnit().sem_ir); unit->RunLower(converter); } - if (options.phase == CompileOptions::Phase::Lower) { + if (options_.phase == CompileOptions::Phase::Lower) { return make_result(); } - CARBON_CHECK(options.phase == CompileOptions::Phase::CodeGen, + CARBON_CHECK(options_.phase == CompileOptions::Phase::CodeGen, "CodeGen should be the last stage"); // Codegen. diff --git a/toolchain/driver/compile_subcommand.h b/toolchain/driver/compile_subcommand.h index 480e5d03fb88..4267a67aec3f 100644 --- a/toolchain/driver/compile_subcommand.h +++ b/toolchain/driver/compile_subcommand.h @@ -10,6 +10,8 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "toolchain/driver/codegen_options.h" +#include "toolchain/driver/driver_env.h" +#include "toolchain/driver/driver_subcommand.h" namespace Carbon { @@ -30,8 +32,9 @@ struct CompileOptions { friend auto operator<<(llvm::raw_ostream& out, Phase phase) -> llvm::raw_ostream&; - auto Build(CommandLine::CommandBuilder& b, CodegenOptions& codegen_options) - -> void; + auto Build(CommandLine::CommandBuilder& b) -> void; + + CodegenOptions codegen_options; Phase phase; @@ -57,6 +60,21 @@ struct CompileOptions { llvm::StringRef exclude_dump_file_prefix; }; +// Implements the compile subcommand of the driver. +class CompileSubcommand : public DriverSubcommand { + public: + auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); } + + auto Run(DriverEnv& driver_env) -> DriverResult override; + + private: + // Does custom validation of the compile-subcommand options structure beyond + // what the command line parsing library supports. + auto ValidateOptions(DriverEnv& driver_env) const -> bool; + + CompileOptions options_; +}; + } // namespace Carbon #endif // CARBON_TOOLCHAIN_DRIVER_COMPILE_SUBCOMMAND_H_ diff --git a/toolchain/driver/driver.cpp b/toolchain/driver/driver.cpp index 5d3a7ae5880c..b86188de0c13 100644 --- a/toolchain/driver/driver.cpp +++ b/toolchain/driver/driver.cpp @@ -19,49 +19,26 @@ namespace Carbon { -struct Driver::Options { +namespace { +struct Options { static const CommandLine::CommandInfo Info; - enum class Subcommand : int8_t { - Compile, - Link, - }; - - void Build(CommandLine::CommandBuilder& b) { - b.AddFlag( - { - .name = "verbose", - .short_name = "v", - .help = "Enable verbose logging to the stderr stream.", - }, - [&](CommandLine::FlagBuilder& arg_b) { arg_b.Set(&verbose); }); - - b.AddSubcommand(CompileOptions::Info, - [&](CommandLine::CommandBuilder& sub_b) { - compile_options.Build(sub_b, codegen_options); - sub_b.Do([&] { subcommand = Subcommand::Compile; }); - }); - - b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) { - link_options.Build(sub_b, codegen_options); - sub_b.Do([&] { subcommand = Subcommand::Link; }); - }); - - b.RequiresSubcommand(); - } + auto Build(CommandLine::CommandBuilder& b) -> void; bool verbose; - Subcommand subcommand; - CodegenOptions codegen_options; - CompileOptions compile_options; - LinkOptions link_options; + CompileSubcommand compile; + LinkSubcommand link; + + // On success, this is set to the subcommand to run. + DriverSubcommand* subcommand = nullptr; }; +} // namespace // Note that this is not constexpr so that it can include information generated // in separate translation units and potentially overridden at link time in the // version string. -const CommandLine::CommandInfo Driver::Options::Info = { +const CommandLine::CommandInfo Options::Info = { .name = "carbon", .version = Version::ToolchainInfo, .help = R"""( @@ -77,16 +54,36 @@ For questions, issues, or bug reports, please use our GitHub project: )""", }; -auto Driver::ParseArgs(llvm::ArrayRef args, Options& options) - -> CommandLine::ParseResult { - return CommandLine::Parse( - args, driver_env_.output_stream, driver_env_.error_stream, Options::Info, - [&](CommandLine::CommandBuilder& b) { options.Build(b); }); +auto Options::Build(CommandLine::CommandBuilder& b) -> void { + b.AddFlag( + { + .name = "verbose", + .short_name = "v", + .help = "Enable verbose logging to the stderr stream.", + }, + [&](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; }); + }); + + b.RequiresSubcommand(); } -auto Driver::RunCommand(llvm::ArrayRef args) -> RunResult { +auto Driver::RunCommand(llvm::ArrayRef args) -> DriverResult { Options options; - CommandLine::ParseResult result = ParseArgs(args, options); + + CommandLine::ParseResult result = CommandLine::Parse( + args, driver_env_.output_stream, driver_env_.error_stream, Options::Info, + [&](CommandLine::CommandBuilder& b) { options.Build(b); }); + if (result == CommandLine::ParseResult::Error) { return {.success = false}; } else if (result == CommandLine::ParseResult::MetaSuccess) { @@ -97,14 +94,8 @@ auto Driver::RunCommand(llvm::ArrayRef args) -> RunResult { // Note this implies streamed output in order to interleave. driver_env_.vlog_stream = &driver_env_.error_stream; } - - switch (options.subcommand) { - case Options::Subcommand::Compile: - return Compile(options.compile_options, options.codegen_options); - case Options::Subcommand::Link: - return Link(options.link_options, options.codegen_options); - } - llvm_unreachable("All subcommands handled!"); + CARBON_CHECK(options.subcommand != nullptr); + return options.subcommand->Run(driver_env_); } } // namespace Carbon diff --git a/toolchain/driver/driver.h b/toolchain/driver/driver.h index 2b00f29f0ec5..86a084b4f43f 100644 --- a/toolchain/driver/driver.h +++ b/toolchain/driver/driver.h @@ -11,6 +11,7 @@ #include "toolchain/driver/codegen_options.h" #include "toolchain/driver/compile_subcommand.h" #include "toolchain/driver/driver_env.h" +#include "toolchain/driver/driver_subcommand.h" #include "toolchain/driver/link_subcommand.h" namespace Carbon { @@ -22,16 +23,6 @@ namespace Carbon { // with the language. class Driver { public: - // The result of RunCommand(). - struct RunResult { - // Overall success result. - bool success; - - // Per-file success results. May be empty if files aren't individually - // processed. - llvm::SmallVector> per_file_success; - }; - // Constructs a driver with any error or informational output directed to a // specified stream. Driver(llvm::vfs::FileSystem& fs, const InstallPaths* installation, @@ -48,38 +39,9 @@ class Driver { // Returns true if the operation succeeds. If the operation fails, returns // false and any information about the failure is printed to the registered // error stream (stderr by default). - auto RunCommand(llvm::ArrayRef args) -> RunResult; + auto RunCommand(llvm::ArrayRef args) -> DriverResult; private: - struct Options; - - // Implementation is in compile_subcommand.cpp. - // TODO: Remove from Driver. - class CompilationUnit; - - // Delegates to the command line library to parse the arguments and store the - // results in a custom `Options` structure that the rest of the driver uses. - auto ParseArgs(llvm::ArrayRef args, Options& options) - -> CommandLine::ParseResult; - - // Does custom validation of the compile-subcommand options structure beyond - // what the command line parsing library supports. - // Implementation is in compile_subcommand.cpp. - // TODO: Remove from Driver. - auto ValidateCompileOptions(const CompileOptions& options) const -> bool; - - // Implements the compile subcommand of the driver. - // Implementation is in compile_subcommand.cpp. - // TODO: Remove from Driver. - auto Compile(const CompileOptions& options, - const CodegenOptions& codegen_options) -> RunResult; - - // Implements the link subcommand of the driver. - // Implementation is in link_subcommand.cpp. - // TODO: Remove from Driver. - auto Link(const LinkOptions& options, const CodegenOptions& codegen_options) - -> RunResult; - DriverEnv driver_env_; }; diff --git a/toolchain/driver/driver_subcommand.h b/toolchain/driver/driver_subcommand.h new file mode 100644 index 000000000000..4ef54615895a --- /dev/null +++ b/toolchain/driver/driver_subcommand.h @@ -0,0 +1,33 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_TOOLCHAIN_DRIVER_DRIVER_SUBCOMMAND_H_ +#define CARBON_TOOLCHAIN_DRIVER_DRIVER_SUBCOMMAND_H_ + +#include "common/ostream.h" +#include "llvm/Support/VirtualFileSystem.h" +#include "toolchain/driver/driver_env.h" +#include "toolchain/install/install_paths.h" + +namespace Carbon { + +// The result of a driver run. +struct DriverResult { + // Overall success result. + bool success; + + // Per-file success results. May be empty if files aren't individually + // processed. + llvm::SmallVector> per_file_success; +}; + +// A subcommand for the driver. +class DriverSubcommand { + public: + virtual auto Run(DriverEnv& driver_env) -> DriverResult = 0; +}; + +} // namespace Carbon + +#endif // CARBON_TOOLCHAIN_DRIVER_DRIVER_SUBCOMMAND_H_ diff --git a/toolchain/driver/link_subcommand.cpp b/toolchain/driver/link_subcommand.cpp index 6335f415190a..85d94bc5b1a9 100644 --- a/toolchain/driver/link_subcommand.cpp +++ b/toolchain/driver/link_subcommand.cpp @@ -7,9 +7,6 @@ #include "llvm/TargetParser/Triple.h" #include "toolchain/driver/clang_runner.h" -// TODO: Remove this include. -#include "toolchain/driver/driver.h" - namespace Carbon { constexpr CommandLine::CommandInfo LinkOptions::Info = { @@ -24,8 +21,7 @@ TODO: Support linking against binary libraries. )""", }; -auto LinkOptions::Build(CommandLine::CommandBuilder& b, - CodegenOptions& codegen_options) -> void { +auto LinkOptions::Build(CommandLine::CommandBuilder& b) -> void { b.AddStringPositionalArg( { .name = "OBJECT_FILE", @@ -84,8 +80,7 @@ static void AddOSFlags(llvm::StringRef target, } } -auto Driver::Link(const LinkOptions& options, - const CodegenOptions& codegen_options) -> RunResult { +auto LinkSubcommand::Run(DriverEnv& driver_env) -> DriverResult { // TODO: Currently we use the Clang driver to link. This works well on Unix // OSes but we likely need to directly build logic to invoke `link.exe` on // Windows where `cl.exe` doesn't typically cover that logic. @@ -109,15 +104,15 @@ auto Driver::Link(const LinkOptions& options, clang_args.push_back("-nostdlib++"); // Add OS-specific flags based on the target. - AddOSFlags(codegen_options.target, clang_args); + AddOSFlags(options_.codegen_options.target, clang_args); clang_args.push_back("-o"); - clang_args.push_back(options.output_filename); - clang_args.append(options.object_filenames.begin(), - options.object_filenames.end()); + clang_args.push_back(options_.output_filename); + clang_args.append(options_.object_filenames.begin(), + options_.object_filenames.end()); - ClangRunner runner(driver_env_.installation, codegen_options.target, - driver_env_.vlog_stream); + ClangRunner runner(driver_env.installation, options_.codegen_options.target, + driver_env.vlog_stream); return {.success = runner.Run(clang_args)}; } diff --git a/toolchain/driver/link_subcommand.h b/toolchain/driver/link_subcommand.h index 0b8e33b32cb7..3a4013fa03b5 100644 --- a/toolchain/driver/link_subcommand.h +++ b/toolchain/driver/link_subcommand.h @@ -9,6 +9,8 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "toolchain/driver/codegen_options.h" +#include "toolchain/driver/driver_env.h" +#include "toolchain/driver/driver_subcommand.h" namespace Carbon { @@ -18,13 +20,24 @@ namespace Carbon { struct LinkOptions { static const CommandLine::CommandInfo Info; - auto Build(CommandLine::CommandBuilder& b, CodegenOptions& codegen_options) - -> void; + auto Build(CommandLine::CommandBuilder& b) -> void; + CodegenOptions codegen_options; llvm::StringRef output_filename; llvm::SmallVector object_filenames; }; +// Implements the link subcommand of the driver. +class LinkSubcommand : public DriverSubcommand { + public: + auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); } + + auto Run(DriverEnv& driver_env) -> DriverResult override; + + private: + LinkOptions options_; +}; + } // namespace Carbon #endif // CARBON_TOOLCHAIN_DRIVER_LINK_SUBCOMMAND_H_