From e7eb3b7b5a26842c74c344db275abc4eb050c993 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Sat, 3 Jan 2026 09:35:29 -0800 Subject: [PATCH] Consolidate default Clang argument handling (#6545) This unifies the default Clang arguments between the `clang` subcommand, the `link` subcommand, and the `ClangInvocation` built for C++ interop. This sets the stage to integrate either pre-built or on-demand runtimes flags for both of these. However, this PR should have very little practical difference. The biggest functional change is wrapping the default arguments in flags to allow unused flags so that we can build a collection of flags viable across compile and link. --- toolchain/base/BUILD | 2 + toolchain/base/clang_invocation.cpp | 105 +++++++++++++----- toolchain/base/clang_invocation.h | 14 ++- .../testdata/interop/cpp/cpp_namespace.carbon | 2 + toolchain/driver/BUILD | 1 + toolchain/driver/clang_runner.cpp | 19 +++- toolchain/driver/compile_subcommand.cpp | 32 +++--- toolchain/driver/link_subcommand.cpp | 36 ------ .../compile/fail_clang_arg_extra_input.carbon | 2 +- toolchain/language_server/context.cpp | 3 +- 10 files changed, 131 insertions(+), 85 deletions(-) diff --git a/toolchain/base/BUILD b/toolchain/base/BUILD index 20ee253e74e4..6c6be345a6e9 100644 --- a/toolchain/base/BUILD +++ b/toolchain/base/BUILD @@ -53,7 +53,9 @@ cc_library( srcs = ["clang_invocation.cpp"], hdrs = ["clang_invocation.h"], deps = [ + ":install_paths", "//common:check", + "//common:string_helpers", "//toolchain/diagnostics:diagnostic_emitter", "@llvm-project//clang:basic", "@llvm-project//clang:frontend", diff --git a/toolchain/base/clang_invocation.cpp b/toolchain/base/clang_invocation.cpp index a424a91df3ba..35ea4cd02dd3 100644 --- a/toolchain/base/clang_invocation.cpp +++ b/toolchain/base/clang_invocation.cpp @@ -4,8 +4,16 @@ #include "toolchain/base/clang_invocation.h" +#include +#include + #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/Utils.h" +#include "common/string_helpers.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/FormatVariadic.h" namespace Carbon { @@ -60,23 +68,36 @@ class ClangDriverDiagnosticConsumer : public clang::DiagnosticConsumer { } // namespace -static auto BuildClangInvocationImpl( - Diagnostics::NoLocEmitter& emitter, - llvm::IntrusiveRefCntPtr fs, - llvm::ArrayRef clang_path_and_args) +auto BuildClangInvocation(Diagnostics::Consumer& consumer, + llvm::IntrusiveRefCntPtr fs, + const InstallPaths& install_paths, + llvm::StringRef target_str, + llvm::ArrayRef extra_args) -> std::unique_ptr { + Diagnostics::ErrorTrackingConsumer error_tracker(consumer); + Diagnostics::NoLocEmitter emitter(&error_tracker); + ClangDriverDiagnosticConsumer diagnostics_consumer(&emitter); + llvm::SmallVector args; + args.push_back("--start-no-unused-arguments"); + AppendDefaultClangArgs(install_paths, target_str, args); + args.push_back("--end-no-unused-arguments"); + args.append({ + llvm::formatv("--target={0}", target_str).str(), + + // Add our include file name as the input file, and force it to be + // interpreted as C++. + "-x", + "c++", + IncludesFileName, + }); + // The clang driver inconveniently wants an array of `const char*`, so convert // the arguments. - llvm::SmallVector driver_args(llvm::map_range( - clang_path_and_args, [](const std::string& str) { return str.c_str(); })); - - // Add our include file name as the input file, and force it to be interpreted - // as C++. - driver_args.push_back("-x"); - driver_args.push_back("c++"); - driver_args.push_back(IncludesFileName); + llvm::OwningArrayRef cstr_arg_storage; + llvm::SmallVector cstr_args = BuildCStrArgs( + install_paths.clang_path().native(), args, extra_args, cstr_arg_storage); // Build a diagnostics engine. Note that we don't have any diagnostic options // yet; they're produced by running the driver. @@ -88,19 +109,8 @@ static auto BuildClangInvocationImpl( // Ask the driver to process the arguments and build a corresponding clang // frontend invocation. - return clang::createInvocation(driver_args, - {.Diags = driver_diags, .VFS = fs}); -} - -auto BuildClangInvocation(Diagnostics::Consumer& consumer, - llvm::IntrusiveRefCntPtr fs, - llvm::ArrayRef clang_path_and_args) - -> std::unique_ptr { - Diagnostics::ErrorTrackingConsumer error_tracker(consumer); - Diagnostics::NoLocEmitter emitter(&error_tracker); - - // Forward to the implementation to avoid exposing `import_cpp` outside check. - auto invocation = BuildClangInvocationImpl(emitter, fs, clang_path_and_args); + auto invocation = + clang::createInvocation(cstr_args, {.Diags = driver_diags, .VFS = fs}); // If Clang produced an error, throw away its invocation. if (error_tracker.seen_error()) { @@ -110,4 +120,49 @@ auto BuildClangInvocation(Diagnostics::Consumer& consumer, return invocation; } +auto AppendDefaultClangArgs(const InstallPaths& /*install_paths*/, + llvm::StringRef target_str, + llvm::SmallVectorImpl& args) -> void { + args.append({ + // Enable PIE by default, but allow it to be overridden by Clang + // arguments. Clang's default is configurable, but we'd like our + // defaults to be more stable. + // TODO: Decide if we want this. + "-fPIE", + + // Override the default linker to use. + "-fuse-ld=lld", + }); + + // Add target-specific flags. + llvm::Triple triple(target_str); + switch (triple.getOS()) { + case llvm::Triple::Darwin: + case llvm::Triple::MacOSX: + // On macOS we need to set the sysroot to a viable SDK. Currently, this + // hard codes the path to be the unversioned symlink. The prefix is also + // hard coded in Homebrew and so this seems likely to work reasonably + // well. Homebrew and I suspect the Xcode Clang both have this hard coded + // at build time, so this seems reasonably safe but we can revisit if/when + // needed. + args.push_back( + "--sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk"); + + // We also need to insist on a modern linker, otherwise the driver tries + // too old and deprecated flags. The specific number here comes from an + // inspection of the Clang driver source code to understand where features + // were enabled, and this appears to be the latest version to control + // driver behavior. + // + // TODO: We should replace this with use of `lld` eventually. + args.push_back("-mlinker-version=705"); + break; + + default: + break; + } + + // TODO: Add flags for the installed runtimes using `install_paths`. +} + } // namespace Carbon diff --git a/toolchain/base/clang_invocation.h b/toolchain/base/clang_invocation.h index bae2b58a7557..e81867f241cb 100644 --- a/toolchain/base/clang_invocation.h +++ b/toolchain/base/clang_invocation.h @@ -11,6 +11,7 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/Support/VirtualFileSystem.h" +#include "toolchain/base/install_paths.h" #include "toolchain/diagnostics/diagnostic_emitter.h" namespace Carbon { @@ -20,9 +21,20 @@ namespace Carbon { // `consumer` if the arguments are invalid. auto BuildClangInvocation(Diagnostics::Consumer& consumer, llvm::IntrusiveRefCntPtr fs, - llvm::ArrayRef clang_path_and_args) + const InstallPaths& install_paths, + llvm::StringRef target_str, + llvm::ArrayRef extra_args = {}) -> std::unique_ptr; +// Appends the default Clang command line arguments used when building a +// Carbon-compatible Clang invocation. +// +// Where possible, code should use `BuildClangInvocation` above. However, when +// invoking Clang directly, this can be used to get the core compatible flags. +auto AppendDefaultClangArgs(const InstallPaths& install_paths, + llvm::StringRef target_str, + llvm::SmallVectorImpl& args) -> void; + } // namespace Carbon #endif // CARBON_TOOLCHAIN_BASE_CLANG_INVOCATION_H_ diff --git a/toolchain/check/testdata/interop/cpp/cpp_namespace.carbon b/toolchain/check/testdata/interop/cpp/cpp_namespace.carbon index c32ef165ca66..fe5684ce945d 100644 --- a/toolchain/check/testdata/interop/cpp/cpp_namespace.carbon +++ b/toolchain/check/testdata/interop/cpp/cpp_namespace.carbon @@ -4,6 +4,8 @@ // // INCLUDE-FILE: toolchain/testing/testdata/min_prelude/none.carbon // +// EXTRA-ARGS: --target=aarch64-unknown-linux +// // AUTOUPDATE // TIP: To test this file alone, run: // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/cpp_namespace.carbon diff --git a/toolchain/driver/BUILD b/toolchain/driver/BUILD index eac5afdd2b96..1812694c969f 100644 --- a/toolchain/driver/BUILD +++ b/toolchain/driver/BUILD @@ -39,6 +39,7 @@ cc_library( "//common:string_helpers", "//common:vlog", "//third_party/llvm:clang_cc1", + "//toolchain/base:clang_invocation", "//toolchain/base:install_paths", "//toolchain/base:kind_switch", "//toolchain/base:runtime_sources", diff --git a/toolchain/driver/clang_runner.cpp b/toolchain/driver/clang_runner.cpp index 5ac2163d6ba0..b9025d013d66 100644 --- a/toolchain/driver/clang_runner.cpp +++ b/toolchain/driver/clang_runner.cpp @@ -51,6 +51,7 @@ #include "llvm/Support/raw_ostream.h" #include "llvm/TargetParser/Host.h" #include "third_party/llvm/clang_cc1.h" +#include "toolchain/base/clang_invocation.h" #include "toolchain/base/install_paths.h" #include "toolchain/driver/clang_runtimes.h" #include "toolchain/driver/runtimes_cache.h" @@ -214,12 +215,12 @@ auto ClangRunner::RunInternal( // Rebuild the args as C-string args. llvm::OwningArrayRef cstr_arg_storage; - llvm::SmallVector cstr_args = - BuildCStrArgs(clang_path, args, cstr_arg_storage); // Handle special dispatch for CC1 commands as they don't use the driver and // we don't synthesize any default arguments there. if (!args.empty() && args[0].starts_with("-cc1")) { + llvm::SmallVector cstr_args = + BuildCStrArgs(clang_path, args, cstr_arg_storage); if (args[0] == "-cc1") { CARBON_VLOG("Dispatching `-cc1` command line..."); int exit_code = @@ -246,6 +247,20 @@ auto ClangRunner::RunInternal( return exit_code == 0; } + // We start with a custom prefix of arguments to establish Carbon's default + // configuration for invoking Clang. These may not all be needed for all + // invocations, so we also suppress warnings about any that are ignored. + llvm::SmallVector prefix_args; + prefix_args.push_back("--start-no-unused-arguments"); + + AppendDefaultClangArgs(*installation_, target, prefix_args); + + prefix_args.push_back("--end-no-unused-arguments"); + + // Rebuild the args as C-string args. + llvm::SmallVector cstr_args = + BuildCStrArgs(clang_path, prefix_args, args, cstr_arg_storage); + CARBON_VLOG("Running Clang driver with the following arguments:\n"); for (const char* cstr_arg : llvm::ArrayRef(cstr_args)) { CARBON_VLOG(" '{0}'\n", cstr_arg); diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp index 377a0a864c17..48faae45428e 100644 --- a/toolchain/driver/compile_subcommand.cpp +++ b/toolchain/driver/compile_subcommand.cpp @@ -1082,31 +1082,25 @@ auto CompileSubcommand::Run(DriverEnv& driver_env) -> DriverResult { // TODO: Share any arguments we specify here with the `carbon clang` // subcommand. { - llvm::SmallVector clang_path_and_args = { - driver_env.installation->clang_path(), - // Propagate the target to Clang. - llvm::formatv("--target={0}", options_.codegen_options.target).str(), - // Enable PIE by default, but allow it to be overridden by Clang - // arguments. Clang's default is configurable, but we'd like our - // defaults to be more stable. - // TODO: Decide if we want this. - "-fPIE", - // Propagate our optimization level to Clang as a default. This can be - // overridden by Clang arguments, but doing so will only have an effect - // if those arguments affect Clang's IR, not its pass pipeline. - GetClangOptimizationFlag(options_.opt_level).str(), - }; if (driver_env.fuzzing && !options_.clang_args.empty()) { // Parsing specific Clang arguments can reach deep into // external libraries that aren't fuzz clean. TestAndDiagnoseIfFuzzingExternalLibraries(driver_env, "compile"); return {.success = false}; } - for (auto str : options_.clang_args) { - clang_path_and_args.push_back(str.str()); - } - clang_invocation = BuildClangInvocation(driver_env.consumer, driver_env.fs, - clang_path_and_args); + + // TODO: Move this into `BuildClangInvocation` when it can accept an + // optimization level. + llvm::SmallVector clang_args = { + // Propagate our optimization level to Clang as a default. This can be + // overridden by Clang arguments, but doing so will only have an effect + // if those arguments affect Clang's IR, not its pass pipeline. + GetClangOptimizationFlag(options_.opt_level), + }; + clang_args.append(options_.clang_args); + clang_invocation = BuildClangInvocation( + driver_env.consumer, driver_env.fs, *driver_env.installation, + options_.codegen_options.target, clang_args); if (!clang_invocation) { return {.success = false}; } diff --git a/toolchain/driver/link_subcommand.cpp b/toolchain/driver/link_subcommand.cpp index 04c3ddd4741a..315ad3abc47c 100644 --- a/toolchain/driver/link_subcommand.cpp +++ b/toolchain/driver/link_subcommand.cpp @@ -38,36 +38,6 @@ The linked file name. The output is always a linked binary. codegen_options.Build(b); } -static void AddOSFlags(llvm::StringRef target, - llvm::SmallVectorImpl& args) { - llvm::Triple triple(target); - switch (triple.getOS()) { - case llvm::Triple::Darwin: - case llvm::Triple::MacOSX: - // On macOS we need to set the sysroot to a viable SDK. Currently, this - // hard codes the path to be the unversioned symlink. The prefix is also - // hard coded in Homebrew and so this seems likely to work reasonably - // well. Homebrew and I suspect the Xcode Clang both have this hard coded - // at build time, so this seems reasonably safe but we can revisit if/when - // needed. - args.push_back( - "--sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk"); - // We also need to insist on a modern linker, otherwise the driver tries - // too old and deprecated flags. The specific number here comes from an - // inspection of the Clang driver source code to understand where features - // were enabled, and this appears to be the latest version to control - // driver behavior. - // - // TODO: We should replace this with use of `lld` eventually. - args.push_back("-mlinker-version=705"); - break; - - default: - // By default, just let the Clang driver handle everything. - break; - } -} - static constexpr CommandLine::CommandInfo SubcommandInfo = { .name = "link", .help = R"""( @@ -99,9 +69,6 @@ auto LinkSubcommand::Run(DriverEnv& driver_env) -> DriverResult { llvm::formatv("--target={0}", options_.codegen_options.target).str(); clang_args.push_back(target_arg); - // Use LLD, which we provide in our install directory, for linking. - clang_args.push_back("-fuse-ld=lld"); - // Disable linking the C++ standard library until can build and ship it as // part of the Carbon toolchain. This clearly won't work once we get into // interop, but for now it avoids spurious failures and distraction. The plan @@ -110,9 +77,6 @@ auto LinkSubcommand::Run(DriverEnv& driver_env) -> DriverResult { // TODO: Replace this when ready. clang_args.push_back("-nostdlib++"); - // Add OS-specific flags based on the target. - 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(), diff --git a/toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon b/toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon index d9fdaed7a9b9..b32e61efaaef 100644 --- a/toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon +++ b/toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon @@ -9,7 +9,7 @@ // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon // TIP: To dump output, run: // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/driver/testdata/compile/fail_clang_arg_extra_input.carbon -// CHECK:STDERR: error: unable to handle compilation, expected exactly one compiler job in ' "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" "bar.cpp"; "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" ""; ' [CppInteropDriverError] +// CHECK:STDERR: error: unable to handle compilation, expected exactly one compiler job in ' "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" ""; "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" "bar.cpp"; ' [CppInteropDriverError] // CHECK:STDERR: // --- foo.carbon diff --git a/toolchain/language_server/context.cpp b/toolchain/language_server/context.cpp index 5600afb569fd..635c8beb8fba 100644 --- a/toolchain/language_server/context.cpp +++ b/toolchain/language_server/context.cpp @@ -169,7 +169,8 @@ auto Context::File::SetText(Context& context, std::optional version, Parse::GetTreeAndSubtreesStore::MakeWithExplicitSize(IdTag(), 1, getter); auto clang_invocation = - BuildClangInvocation(consumer, fs, {context.installation().clang_path()}); + BuildClangInvocation(consumer, fs, context.installation(), + llvm::sys::getDefaultTargetTriple()); Check::CheckParseTrees(units, getters, fs, check_options, std::move(clang_invocation));