[clang][driver] Fix -print-libgcc-file-name on Darwin platforms

[Xazax-hun]

On Darwin, -print-libgcc-file-name was returning a nonsensical result. It would return the name of the library that would be used by the default toolchain implementation, but that was something that didn't exist on Darwin.

Fixing this requires initializing the Darwin toolchain before processing -print-libgcc-file-name. Previously, the Darwin toolchain would only be initialized when building the jobs for this compilation, which is too late since -print-libgcc-file-name requires the toolchain to be initialized in order to provide the right results.

rdar://90633749

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ldionne]

It looks like the embedded targets are already tested to some extent inside clang/test/Driver/darwin-embedded.c. I would add a new test file clang/test/Driver/darwin-embedded-print-libgcc-file-name.c and add something like this in it:

// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
// RUN:     --target=armv7em-apple-darwin \
// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-HARD_STATIC %s
// CHECK-CLANGRT-HARD_STATIC: libclang_rt.hard_static.a

// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
// RUN:     --target=armv7em-apple-darwin -msoft-float \
// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-SOFT_STATIC %s
// CHECK-CLANGRT-SOFT_STATIC: libclang_rt.soft_static.a

// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
// RUN:     --target=armv7em-apple-darwin -fPIC \
// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-HARD_PIC %s
// CHECK-CLANGRT-HARD_PIC: libclang_rt.hard_pic.a

// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
// RUN:     --target=armv7em-apple-darwin -msoft-float -fPIC \
// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-SOFT_PIC %s
// CHECK-CLANGRT-SOFT_PIC: libclang_rt.soft_pic.a

I'm not certain what we're testing, but we're testing the behavior that seems to exist today, so at least we'll have coverage for whatever we do today.

[Xazax-hun]

I added these tests and unfortunately they fail (both before and after my change). I believe part of the problem is that -print-libgcc-file-name is still only dealing with the default toolchain we construct first. The actual toolchain object used is constructed much much later, so early initialization is not sufficient to fix this particular problem. I added these tests as expected to fail to document this shortcoming. I still believe we should take this change as it at least fixes the behavior of this flag for the non-embedded scenarios.

The proper fix would be really involved. I shortly experimented with moving the processing of -print-libgcc-file-name to a later stage in the driver, but that is also problematic as clang might start to emit errors about missing input files in that case (because having an input file is not a requirement for this particular flag, but it is a requirement to build the jobs/actions that happen before we construct the actual toolchain that has the correct data we need).

I am not really sure what is the correct solution here, I start to think that the -print-libgcc-file-name flag is just not compatible with the way clang works, and we might need a separate flag that needs input files as well and is processed at a later stage and is more reliable for all targets.

[ldionne]

Ack. I think it's reasonable not to try to fix everything in one go since it seems really involved. However, my thinking is more that the Darwin driver is the odd one out here, the driver should be initialized when we construct it, not "lazily" when we compute the arguments. I think it's the only driver which does that, and isn't that the source of all our problems?

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Gábor Horváth (Xazax-hun)

Changes

On Darwin, -print-libgcc-file-name was returning a nonsensical result. It would return the name of the library that would be used by the default toolchain implementation, but that was something that didn't exist on Darwin.

Fixing this requires initializing the Darwin toolchain before processing -print-libgcc-file-name. Previously, the Darwin toolchain would only be initialized when building the jobs for this compilation, which is too late since -print-libgcc-file-name requires the toolchain to be initialized in order to provide the right results.

rdar://90633749

---

Full diff: https://github.com/llvm/llvm-project/pull/98325.diff

5 Files Affected:

• (modified) clang/include/clang/Driver/Driver.h (+3-2)
• (modified) clang/lib/Driver/Driver.cpp (+12-1)
• (modified) clang/lib/Driver/ToolChains/Darwin.cpp (+43-21)
• (modified) clang/lib/Driver/ToolChains/Darwin.h (+13)
• (added) clang/test/Driver/darwin-print-libgcc-file-name.c (+97)

diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index cc1538372d5f8..04b46782467d6 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -628,8 +628,9 @@ class Driver {
   /// treated before building actions or binding tools.
   ///
   /// \return Whether any compilation should be built for this
-  /// invocation.
-  bool HandleImmediateArgs(const Compilation &C);
+  /// invocation. The compilation can only be modified when
+  /// this function returns false.
+  bool HandleImmediateArgs(Compilation &C);
 
   /// ConstructAction - Construct the appropriate action to do for
   /// \p Phase on the \p Input, taking in to account arguments
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 221e222bdd47d..083756dfb62f2 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -2128,7 +2128,7 @@ void Driver::HandleAutocompletions(StringRef PassedFlags) const {
   llvm::outs() << llvm::join(SuggestedCompletions, "\n") << '\n';
 }
 
-bool Driver::HandleImmediateArgs(const Compilation &C) {
+bool Driver::HandleImmediateArgs(Compilation &C) {
   // The order these options are handled in gcc is all over the place, but we
   // don't expect inconsistencies w.r.t. that to matter in practice.
 
@@ -2180,6 +2180,15 @@ bool Driver::HandleImmediateArgs(const Compilation &C) {
   }
 
   const ToolChain &TC = C.getDefaultToolChain();
+  auto initDarwinTarget = [&]() {
+    // The 'Darwin' toolchain is initialized only when its arguments are
+    // computed. Get the default arguments for OFK_None to ensure that
+    // initialization is performed before trying to access properties of
+    // the toolchain in the functions below.
+    // FIXME: Remove when darwin's toolchain is initialized during construction.
+    const llvm::Triple Triple(TC.ComputeEffectiveClangTriple(C.getArgs()));
+    C.getArgsForToolChain(&TC, Triple.getArchName(), Action::OFK_None);
+  };
 
   if (C.getArgs().hasArg(options::OPT_v))
     TC.printVerboseInfo(llvm::errs());
@@ -2230,6 +2239,7 @@ bool Driver::HandleImmediateArgs(const Compilation &C) {
   }
 
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
+    initDarwinTarget();
     if (std::optional<std::string> RuntimePath = TC.getRuntimePath())
       llvm::outs() << *RuntimePath << '\n';
     else
@@ -2271,6 +2281,7 @@ bool Driver::HandleImmediateArgs(const Compilation &C) {
   if (C.getArgs().hasArg(options::OPT_print_libgcc_file_name)) {
     ToolChain::RuntimeLibType RLT = TC.GetRuntimeLibType(C.getArgs());
     const llvm::Triple Triple(TC.ComputeEffectiveClangTriple(C.getArgs()));
+    initDarwinTarget();
     RegisterEffectiveTriple TripleRAII(TC, Triple);
     switch (RLT) {
     case ToolChain::RLT_CompilerRT:
diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp
index f354b0974d5f2..e530e45a9793b 100644
--- a/clang/lib/Driver/ToolChains/Darwin.cpp
+++ b/clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1272,23 +1272,8 @@ unsigned DarwinClang::GetDefaultDwarfVersion() const {
 void MachO::AddLinkRuntimeLib(const ArgList &Args, ArgStringList &CmdArgs,
                               StringRef Component, RuntimeLinkOptions Opts,
                               bool IsShared) const {
-  SmallString<64> DarwinLibName = StringRef("libclang_rt.");
-  // On Darwin the builtins component is not in the library name.
-  if (Component != "builtins") {
-    DarwinLibName += Component;
-    if (!(Opts & RLO_IsEmbedded))
-      DarwinLibName += "_";
-  }
-
-  DarwinLibName += getOSLibraryNameSuffix();
-  DarwinLibName += IsShared ? "_dynamic.dylib" : ".a";
-  SmallString<128> Dir(getDriver().ResourceDir);
-  llvm::sys::path::append(Dir, "lib", "darwin");
-  if (Opts & RLO_IsEmbedded)
-    llvm::sys::path::append(Dir, "macho_embedded");
-
-  SmallString<128> P(Dir);
-  llvm::sys::path::append(P, DarwinLibName);
+  std::string P = getCompilerRT(
+      Args, Component, IsShared ? ToolChain::FT_Shared : ToolChain::FT_Static);
 
   // For now, allow missing resource libraries to support developers who may
   // not have compiler-rt checked out or integrated into their build (unless
@@ -1303,18 +1288,55 @@ void MachO::AddLinkRuntimeLib(const ArgList &Args, ArgStringList &CmdArgs,
   // rpaths. This is currently true from this place, but we need to be
   // careful if this function is ever called before user's rpaths are emitted.
   if (Opts & RLO_AddRPath) {
-    assert(DarwinLibName.ends_with(".dylib") && "must be a dynamic library");
+    assert(StringRef(P).ends_with(".dylib") && "must be a dynamic library");
 
     // Add @executable_path to rpath to support having the dylib copied with
     // the executable.
     CmdArgs.push_back("-rpath");
     CmdArgs.push_back("@executable_path");
 
-    // Add the path to the resource dir to rpath to support using the dylib
-    // from the default location without copying.
+    // Add the compiler-rt library's directory to rpath to support using the
+    // dylib from the default location without copying.
     CmdArgs.push_back("-rpath");
-    CmdArgs.push_back(Args.MakeArgString(Dir));
+    CmdArgs.push_back(Args.MakeArgString(llvm::sys::path::parent_path(P)));
+  }
+}
+
+std::string MachO::getCompilerRT(const ArgList &, StringRef Component,
+                                 FileType Type) const {
+  assert(Type != ToolChain::FT_Object &&
+         "it doesn't make sense to ask for the compiler-rt library name as an "
+         "object file");
+  SmallString<64> MachOLibName = StringRef("libclang_rt");
+  // On MachO, the builtins component is not in the library name
+  if (Component != "builtins") {
+    MachOLibName += '.';
+    MachOLibName += Component;
+  }
+  MachOLibName += Type == ToolChain::FT_Shared ? "_dynamic.dylib" : ".a";
+
+  SmallString<128> FullPath(getDriver().ResourceDir);
+  llvm::sys::path::append(FullPath, "lib", "darwin", "macho_embedded", MachOLibName);
+  return std::string(FullPath);
+}
+
+std::string Darwin::getCompilerRT(const ArgList &, StringRef Component,
+                                  FileType Type) const {
+  assert(Type != ToolChain::FT_Object &&
+         "it doesn't make sense to ask for the compiler-rt library name as an "
+         "object file");
+  SmallString<64> DarwinLibName = StringRef("libclang_rt.");
+  // On Darwin, the builtins component is not in the library name
+  if (Component != "builtins") {
+    DarwinLibName += Component;
+    DarwinLibName += '_';
   }
+  DarwinLibName += getOSLibraryNameSuffix();
+  DarwinLibName += Type == ToolChain::FT_Shared ? "_dynamic.dylib" : ".a";
+
+  SmallString<128> FullPath(getDriver().ResourceDir);
+  llvm::sys::path::append(FullPath, "lib", "darwin", DarwinLibName);
+  return std::string(FullPath);
 }
 
 StringRef Darwin::getPlatformFamily() const {
diff --git a/clang/lib/Driver/ToolChains/Darwin.h b/clang/lib/Driver/ToolChains/Darwin.h
index b45279ecedeb2..2e55b49682a7e 100644
--- a/clang/lib/Driver/ToolChains/Darwin.h
+++ b/clang/lib/Driver/ToolChains/Darwin.h
@@ -223,6 +223,13 @@ class LLVM_LIBRARY_VISIBILITY MachO : public ToolChain {
     // There aren't any profiling libs for embedded targets currently.
   }
 
+  // Return the full path of the compiler-rt library on a non-Darwin MachO
+  // system. Those are under
+  // <resourcedir>/lib/darwin/macho_embedded/<...>(.dylib|.a).
+  std::string
+  getCompilerRT(const llvm::opt::ArgList &Args, StringRef Component,
+                FileType Type = ToolChain::FT_Static) const override;
+
   /// }
   /// @name ToolChain Implementation
   /// {
@@ -356,6 +363,12 @@ class LLVM_LIBRARY_VISIBILITY Darwin : public MachO {
   void addProfileRTLibs(const llvm::opt::ArgList &Args,
                         llvm::opt::ArgStringList &CmdArgs) const override;
 
+  // Return the full path of the compiler-rt library on a Darwin MachO system.
+  // Those are under <resourcedir>/lib/darwin/<...>(.dylib|.a).
+  std::string
+  getCompilerRT(const llvm::opt::ArgList &Args, StringRef Component,
+                FileType Type = ToolChain::FT_Static) const override;
+
 protected:
   /// }
   /// @name Darwin specific Toolchain functions
diff --git a/clang/test/Driver/darwin-print-libgcc-file-name.c b/clang/test/Driver/darwin-print-libgcc-file-name.c
new file mode 100644
index 0000000000000..67153ab206f97
--- /dev/null
+++ b/clang/test/Driver/darwin-print-libgcc-file-name.c
@@ -0,0 +1,97 @@
+// Test the output of -print-libgcc-file-name on Darwin.
+
+//
+// All platforms
+//
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=x86_64-apple-macos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-MACOS %s
+// CHECK-CLANGRT-MACOS: libclang_rt.osx.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-ios \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-IOS %s
+// CHECK-CLANGRT-IOS: libclang_rt.ios.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-watchos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-WATCHOS %s
+// CHECK-CLANGRT-WATCHOS: libclang_rt.watchos.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-tvos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-TVOS %s
+// CHECK-CLANGRT-TVOS: libclang_rt.tvos.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-driverkit \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-DRIVERKIT %s
+// CHECK-CLANGRT-DRIVERKIT: libclang_rt.driverkit.a
+
+//
+// Simulators
+//
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-ios-simulator \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-IOS-SIMULATOR %s
+// CHECK-CLANGRT-IOS-SIMULATOR: libclang_rt.iossim.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-watchos-simulator \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-WATCHOS-SIMULATOR %s
+// CHECK-CLANGRT-WATCHOS-SIMULATOR: libclang_rt.watchossim.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     --target=arm64-apple-tvos-simulator \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-TVOS-SIMULATOR %s
+// CHECK-CLANGRT-TVOS-SIMULATOR: libclang_rt.tvossim.a
+
+//
+// Check the cc_kext variants
+//
+
+// TODO
+
+//
+// Check the sanitizer and profile variants
+//
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     -fsanitize=address --target=x86_64-apple-macos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-MACOS-SAN %s
+// CHECK-CLANGRT-MACOS-SAN: libclang_rt.osx.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     -fsanitize=address --target=arm64-apple-ios \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-IOS-SAN %s
+// CHECK-CLANGRT-IOS-SAN: libclang_rt.ios.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     -fsanitize=address --target=arm64-apple-watchos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-WATCHOS-SAN %s
+// CHECK-CLANGRT-WATCHOS-SAN: libclang_rt.watchos.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     -fsanitize=address --target=arm64-apple-tvos \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-TVOS-SAN %s
+// CHECK-CLANGRT-TVOS-SAN: libclang_rt.tvos.a
+
+// RUN: %clang -rtlib=compiler-rt -print-libgcc-file-name \
+// RUN:     -fsanitize=address --target=arm64-apple-driverkit \
+// RUN:     -resource-dir=%S/Inputs/resource_dir 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-CLANGRT-DRIVERKIT-SAN %s
+// CHECK-CLANGRT-DRIVERKIT-SAN: libclang_rt.driverkit.a

[ldionne]

The patch LGTM, but I'd like actual Clang folks to stamp this.

[Xazax-hun]

@MaskRay Thanks for the review. Do you think this is good to go or is there anything else you want me to change?

[Xazax-hun]

It has been a couple of days without any activity. I plan to merge this PR tomorrow and address any potential concerns in a followup PR. Let me know if you want me to wait a bit more if there are any concerns that should be addressed first.