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

@@ -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",

toolchain/base/clang_invocation.cpp

@@ -4,8 +4,16 @@
 
 #include "toolchain/base/clang_invocation.h"
 
+#include <filesystem>
+#include <string>
+
 #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<llvm::vfs::FileSystem> fs,
-    llvm::ArrayRef<std::string> clang_path_and_args)
+auto BuildClangInvocation(Diagnostics::Consumer& consumer,
+                          llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
+                          const InstallPaths& install_paths,
+                          llvm::StringRef target_str,
+                          llvm::ArrayRef<llvm::StringRef> extra_args)
     -> std::unique_ptr<clang::CompilerInvocation> {
+  Diagnostics::ErrorTrackingConsumer error_tracker(consumer);
+  Diagnostics::NoLocEmitter emitter(&error_tracker);
+
   ClangDriverDiagnosticConsumer diagnostics_consumer(&emitter);
 
+  llvm::SmallVector<std::string> 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<const char*> 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<char> cstr_arg_storage;
+  llvm::SmallVector<const char*> 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<llvm::vfs::FileSystem> fs,
-                          llvm::ArrayRef<std::string> clang_path_and_args)
-    -> std::unique_ptr<clang::CompilerInvocation> {
-  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<std::string>& 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

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<llvm::vfs::FileSystem> fs,
-                          llvm::ArrayRef<std::string> clang_path_and_args)
+                          const InstallPaths& install_paths,
+                          llvm::StringRef target_str,
+                          llvm::ArrayRef<llvm::StringRef> extra_args = {})
     -> std::unique_ptr<clang::CompilerInvocation>;
 
+// 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<std::string>& args) -> void;
+
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_BASE_CLANG_INVOCATION_H_

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

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",

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<char> cstr_arg_storage;
-  llvm::SmallVector<const char*, 64> 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<const char*, 64> 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<std::string> 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<const char*, 64> 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);

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<std::string> 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<llvm::StringRef> 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};
     }

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<llvm::StringRef>& 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(),

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++" "<carbon Cpp imports>"; ' [CppInteropDriverError]
+// CHECK:STDERR: error: unable to handle compilation, expected exactly one compiler job in ' "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" "<carbon Cpp imports>";  "{{.*}}/clang" "-cc1" {{.*}} "-x" "c++" "bar.cpp"; ' [CppInteropDriverError]
 // CHECK:STDERR:
 
 // --- foo.carbon

toolchain/language_server/context.cpp

@@ -169,7 +169,8 @@ auto Context::File::SetText(Context& context, std::optional<int64_t> 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));