Teach the `link` subcommand to accept Clang-style `LDFLAGS` (#6741)

Add an optional additional set of positional parameters that can be
passed to the `link` subcommand for Clang-style (or GCC-style)
`LDFLAGS`. These can _also_ contain object files, etc., and in fact it
is useful to allow them to contain object files in order to integrate
the `carbon link` subcommand into a build system that mixes both link
flags and object files. This at least happens with Bazel, and I suspect
is common.

Eventually, it would be nice to have sufficient semantics to handle all
the varieties of links we want without resorting to this escape hatch,
but that's likely a long way away and so it seems especially useful to
allow falling back to Clang's flags as needed for now.

This does somewhat directly surface the Clang implementation detail in
the command line syntax, but I don't see a lot of good alternatives.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/diagnostics/kind.def

@@ -36,6 +36,8 @@ CARBON_DIAGNOSTIC_KIND(FailureBuildingRuntimes)
 CARBON_DIAGNOSTIC_KIND(FailureRunningClang)
 CARBON_DIAGNOSTIC_KIND(FailureRunningClangToLink)
 CARBON_DIAGNOSTIC_KIND(FormatMultipleFilesToOneOutput)
+CARBON_DIAGNOSTIC_KIND(LinkObjectFilesMissing)
+CARBON_DIAGNOSTIC_KIND(LinkOutputOptionMissing)
 CARBON_DIAGNOSTIC_KIND(ToolFuzzingDisallowed)
 
 // ============================================================================

toolchain/driver/BUILD

@@ -248,6 +248,7 @@ cc_test(
         "//common:error_test_helpers",
         "//common:filesystem",
         "//common:raw_string_ostream",
+        "//testing/base:capture_std_streams",
         "//testing/base:file_helpers",
         "//testing/base:global_exe_path",
         "//testing/base:gtest_main",

toolchain/driver/driver_test.cpp

@@ -22,6 +22,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
+#include "testing/base/capture_std_streams.h"
 #include "testing/base/file_helpers.h"
 #include "testing/base/global_exe_path.h"
 #include "toolchain/testing/yaml_test_helpers.h"
@@ -30,6 +31,7 @@ namespace Carbon {
 namespace {
 
 using ::testing::_;
+using Testing::CallWithCapturedOutput;
 using ::testing::ContainsRegex;
 using ::testing::HasSubstr;
 using Testing::IsSuccess;
@@ -248,10 +250,11 @@ TEST_F(DriverTest, Link) {
       << test_error_stream_.TakeStr();
 
   // Now link this into a binary. Note that we suppress building runtimes on
-  // demand here as no runtimes should be needed for the empty program.
+  // demand here as no runtimes should be needed for the empty program. We also
+  // pass some system library link flags through to the underlying Clang layer.
   EXPECT_TRUE(driver_
                   .RunCommand({"--no-build-runtimes", "link", "--output=test",
-                               "test.o"})
+                               "test.o", "--", "-lc", "-lm"})
                   .success);
   EXPECT_THAT(test_error_stream_.TakeStr(), StrEq(""));
 
@@ -266,6 +269,46 @@ TEST_F(DriverTest, Link) {
   EXPECT_TRUE(result->getBinary()->isObject());
 }
 
+TEST_F(DriverTest, LinkWithFlagLikeFiles) {
+  auto scope = ScopedTempWorkingDir();
+
+  // First compile a file to get a linkable object.
+  MakeTestFile("fn Run() {}", "test.carbon");
+  ASSERT_TRUE(
+      driver_.RunCommand({"compile", "--no-prelude-import", "test.carbon"})
+          .success)
+      << test_error_stream_.TakeStr();
+
+  // Rename it to a flag-like name.
+  Filesystem::Cwd().Rename("test.o", Filesystem::Cwd(), "--test.o").Check();
+
+  // Link this into a binary and pass flags to the Clang link invocation even
+  // though we use `--` before the object file input list to handle weirdly
+  // named objects.
+  //
+  // TODO: This works correctly in the Carbon link subcommand, but Clang itself
+  // fails to pass object files to LLD in a way that supports flag-shaped object
+  // file names. The last flag being `-Wl,--` tries to work around this by
+  // passing a `--` to the linker before the object files. However, LLD in turn
+  // appears to have bugs parsing command lines in this shape. We should get
+  // these fixed and then can remove the `-Wl,--` hack and the test should
+  // actually pass.
+  std::string out;
+  std::string err;
+  EXPECT_FALSE(CallWithCapturedOutput(out, err, [&] {
+                 return driver_.RunCommand({"--no-build-runtimes", "link",
+                                            "--output=test", "--", "--test.o",
+                                            "--", "-lc", "-lm", "-Wl,--"});
+               }).success);
+  EXPECT_THAT(test_error_stream_.TakeStr(), StrEq(""));
+  EXPECT_THAT(out, StrEq(""));
+  // This error seems to stem from incorrectly handling `--` in the LLD command
+  // line. See above; this should go away when the underlying bugs are fixed.
+  EXPECT_THAT(err,
+              HasSubstr("error: completed parsing all 1 configured positional "
+                        "arguments, but found a subsequent `--`"));
+}
+
 TEST_F(DriverTest, ConfigJson) {
   EXPECT_TRUE(driver_.RunCommand({"config", "--json"}).success);
   EXPECT_THAT(test_error_stream_.TakeStr(), StrEq(""));

toolchain/driver/link_subcommand.cpp

@@ -15,12 +15,12 @@ auto LinkOptions::Build(CommandLine::CommandBuilder& b) -> void {
           .name = "OBJECT_FILE",
           .help = R"""(
 The input object files.
+
+If empty, there must be extra Clang link arguments that provide object files
+intermingled with linking flags.
 )""",
       },
-      [&](auto& arg_b) {
-        arg_b.Required(true);
-        arg_b.Append(&object_filenames);
-      });
+      [&](auto& arg_b) { arg_b.Append(&object_filenames); });
 
   b.AddStringOption(
       {
@@ -28,12 +28,28 @@ The input object files.
           .value_name = "FILE",
           .help = R"""(
 The linked file name. The output is always a linked binary.
+
+If not provided, there must be extra Clang link arguments that include
+specifying the output of the link. This allows supporting build systems that
+intermingle the output flag with arbitrary other linker flags that need to use
+legacy parsing logic.
 )""",
       },
-      [&](auto& arg_b) {
-        arg_b.Required(true);
-        arg_b.Set(&output_filename);
-      });
+      [&](auto& arg_b) { arg_b.Set(&output_filename); });
+
+  b.AddStringPositionalArg(
+      {
+          .name = "EXTRA_CLANG_LINK_ARGS",
+          .help = R"""(
+Extra arguments to pass to Clang when forming the link command. This is
+primarily useful for expanding `LDFLAGS` or other baseline linking flags in a
+build system.
+
+These can also be used to pass object files to the link in the event your build
+system mixes object files and linker flags.
+)""",
+      },
+      [&](auto& arg_b) { arg_b.Append(&extra_clang_args); });
 
   codegen_options.Build(b);
 }
@@ -69,8 +85,33 @@ auto LinkSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
       llvm::formatv("--target={0}", options_.codegen_options.target).str();
   clang_args.push_back(target_arg);
 
-  clang_args.push_back("-o");
-  clang_args.push_back(options_.output_filename);
+  if (!options_.output_filename.empty()) {
+    clang_args.push_back("-o");
+    clang_args.push_back(options_.output_filename);
+  } else if (options_.extra_clang_args.empty()) {
+    CARBON_DIAGNOSTIC(LinkOutputOptionMissing, Error,
+                      "no output specified to a link command and no extra "
+                      "Clang options that can provide an output");
+    driver_env.emitter.Emit(LinkOutputOptionMissing);
+    return {.success = false};
+  }
+
+  if (options_.object_filenames.empty() && options_.extra_clang_args.empty()) {
+    CARBON_DIAGNOSTIC(LinkObjectFilesMissing, Error,
+                      "no object files provided to link command and no extra "
+                      "Clang options that could provide them");
+    driver_env.emitter.Emit(LinkObjectFilesMissing);
+    return {.success = false};
+  }
+
+  // Note that we append any extra Clang args before our object filenames. This
+  // allows us to propagate object filenames that collide with Clang flags using
+  // `--` before the filenames. While in theory, this could create a problem in
+  // the presence of mixtures of object files in the two lists and the order
+  // being dependent, we don't expect that in practice.
+  clang_args.append(options_.extra_clang_args.begin(),
+                    options_.extra_clang_args.end());
+  clang_args.push_back("--");
   clang_args.append(options_.object_filenames.begin(),
                     options_.object_filenames.end());
 

toolchain/driver/link_subcommand.h

@@ -23,6 +23,8 @@ struct LinkOptions {
   CodegenOptions codegen_options;
   llvm::StringRef output_filename;
   llvm::SmallVector<llvm::StringRef> object_filenames;
+
+  llvm::SmallVector<llvm::StringRef> extra_clang_args;
 };
 
 // Implements the link subcommand of the driver.

toolchain/driver/testdata/link/fail_object_files_missing.carbon

@@ -0,0 +1,15 @@
+// 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
+//
+// ARGS: --include-diagnostic-kind link --output=foo
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/link/fail_object_files_missing.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/driver/testdata/link/fail_object_files_missing.carbon
+// CHECK:STDERR: error: no object files provided to link command and no extra Clang options that could provide them [LinkObjectFilesMissing]
+// CHECK:STDERR:
+
+// --- foo.carbon

toolchain/driver/testdata/link/fail_output_missing.carbon

@@ -0,0 +1,15 @@
+// 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
+//
+// ARGS: --include-diagnostic-kind link foo.o bar.o
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/link/fail_output_missing.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/driver/testdata/link/fail_output_missing.carbon
+// CHECK:STDERR: error: no output specified to a link command and no extra Clang options that can provide an output [LinkOutputOptionMissing]
+// CHECK:STDERR:
+
+// --- foo.carbon