[clang][driver] Linking to just-built libc++.dylib when bootstrapping libc++ with clang

[fahadnayyar]

Summary

When libc++ is bootstrapped with clang using the cmake options
-DLLVM_ENABLE_PROJECTS="clang;llvm;lldb" and
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" then the resulting clang uses
the just-built libcxx headers from <build-directory>/bin/../include/c++/v1
. So the clang in this case should also use the just-built libc++.dylib
from <build-directory>/bin/../lib instead of using the system's libc++
libraries.

rdar://107060541

[fdeazeve]

Thanks for working on this! The idea looks good to me, but I am not familiar with this part of the codebase, so I'll defer the review to others

[ldionne]

Thanks for working on this! This generally LGTM, with a few comments. You should be able to add tests in a way similar to clang/test/Driver/darwin-header-search-libcxx.cpp. The idea is basically to create a dummy sysroot that contains a libc++.dylib file in the right location and trick the compiler into adding -L as you want.

clang/lib/Driver/ToolChains/Darwin.cpp

423

428

Otherwise you are forming the path <install>/bin/../lib/../lib/libc++.dylib, which is correct but weird.

[ldionne]

clang/lib/Driver/ToolChains/Darwin.cpp

420–421

I would reword this to:

// If the toolchain contains libc++.dylib (in <install>/bin/../lib/libc++.dylib), use that instead of the sysroot-provided one. This matches what we do for determining which libc++ headers to use.

435

Where do we set the usual <sysroot>/usr/lib search path? I think it might make sense to move the code you added to that place.

[fahadnayyar]

Added test case. Now checking the existence of libc++ headers in the toolchain when including the libc++.dylib form toolchain and vice versa. Also checking the absence of -nostdinc, -nostdinc++ or -nostdlib
arguments before linking with libc++.dylib from the toolchain.

[ldionne]

clang/lib/Driver/ToolChains/Darwin.cpp

435

What I mean: there's a place where we must be adding -L <sysroot>/usr/lib in the code, and it isn't in darwin::Linker::AddLinkArgs. Where is it? Would it make sense to move the code you added to that place instead?

438

The more I look at this, the more I think it seems kind of weird to me that a linker argument would be impacted by -nostdinc & friends, which control header search paths. IMO we should use the simplest behavior and only check for libc++.dylib here, and not check for libc++.dylib when we determine the header search paths.

@arphaman Do you have an opinion?

[fahadnayyar]

clang/lib/Driver/ToolChains/Darwin.cpp

435

<sysroot>/usr/local/include is added here in `DarwinClang::AddClangSystemIncludeArgs``: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Darwin.cpp#L2346 and <sysroot>/usr/include is added here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Darwin.cpp#L2374 .. But since we want to add a new argument to the linker invocation, I feel `darwin::Linker::AddLinkArgs`` is the best place. But let me know if you think otherwise.

[fahadnayyar]

Fixed regression error in clang-tidy test.

[fahadnayyar]

Fixing some more regression test errors.

[arphaman]

clang/lib/Driver/ToolChains/Darwin.cpp

438

That makes sense to me. I'm not sure why specifically do we need to check if the dylib exists when determining the header search path. Is there a specific reason we couldn't mix the local headers and the system's dylib in that case?

[ldionne]

clang/lib/Driver/ToolChains/Darwin.cpp

438

We definitely don't want to mix the local headers and the system dylib. They can be configured differently (e.g. different ABI flags amongst other things). However it shouldn't really matter since when we build libc++, we build both the dylib and the headers so either both should be picked up or neither should be. I wouldn't bother checking. If we really wanted to check, we could actually error-out if one (but not both) are there, since that would point out to a mismatch.

[fahadnayyar]

Passing -L <toolchain-install>/bin/../lib/ unconditionally to the linker.

[fahadnayyar]

Rebasing to latest main.

[ldionne]

clang/test/Driver/darwin-header-search-libcxx.cpp

119

Is this change really needed anymore? Why?

clang/test/Driver/darwin-link-libcxx-from-toolchain.cpp

11

I don't see CHECK-TOOLCHAIN-LIBCXX-LINKING-2 defined anywhere?

13

Some of these test cases and comments don't make sense for this test -- you copy-pasted the test for header search paths and that's fine, but please go through it to make sure you make the necessary edits.

[fahadnayyar]

Simplifying the test case.

[fahadnayyar]

clang/test/Driver/darwin-header-search-libcxx.cpp

119

This is unrelated small bug in darwin-header-search-libcxx.cpp. I thought maybe we fix that small issue also in this patch. What you think?

[ldionne]

clang/test/Driver/darwin-header-search-libcxx.cpp

95

105

119

Ah, I see. I think I understand the bug. Yeah I think it makes sense to fix it here but I added a few more fixes in comments.

133

[ldionne]

This LGTM w/ comments applied. Thanks!

[ldionne]

clang/test/Driver/darwin-header-search-libcxx.cpp

105

Nevermind.

133

Nevermind.

[fahadnayyar]

Edits in comments.

[ldionne]

Gentle ping. Are we waiting on anything to ship this?

clang/lib/Driver/ToolChains/Darwin.cpp

void darwin::Linker::AddLinkArgs(Compilation &C, const ArgList &Args,
   Args.AddAllArgs(CmdArgs, options::OPT_segaddr);
   Args.AddAllArgs(CmdArgs, options::OPT_segs__read__only__addr);
   Args.AddAllArgs(CmdArgs, options::OPT_segs__read__write__addr);
   Args.AddAllArgs(CmdArgs, options::OPT_seg__addr__table);
   Args.AddAllArgs(CmdArgs, options::OPT_seg__addr__table__filename);
   Args.AddAllArgs(CmdArgs, options::OPT_sub__library);
   Args.AddAllArgs(CmdArgs, options::OPT_sub__umbrella);
 
+  // Pass -L <toolchain-install>/bin/../lib/ to linker to prioritize toolchain's
+  // libc++.dylib over the sysroot-provided one. This matches what we do for
+  // determining which libc++ headers to use.
+  llvm::SmallString<128> InstallBinPath =
+      llvm::StringRef(D.getInstalledDir()); // <install>/bin
+  llvm::SmallString<128> LibCxxDylibDirPath = InstallBinPath;
+  llvm::sys::path::append(LibCxxDylibDirPath, "..",
+                          "lib"); // <install>/bin/../lib
+  CmdArgs.push_back("-L");
+  CmdArgs.push_back(C.getArgs().MakeArgString(LibCxxDylibDirPath));
+
   // Give --sysroot= preference, over the Apple specific behavior to also use
   // --isysroot as the syslibroot.
   StringRef sysroot = C.getSysRoot();
   if (sysroot != "") {
     CmdArgs.push_back("-syslibroot");
     CmdArgs.push_back(C.getArgs().MakeArgString(sysroot));
   } else if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
     CmdArgs.push_back("-syslibroot");
     CmdArgs.push_back(A->getValue());
   }
 
   Args.AddLastArg(CmdArgs, options::OPT_twolevel__namespace);
   Args.AddLastArg(CmdArgs, options::OPT_twolevel__namespace__hints);
   Args.AddAllArgs(CmdArgs, options::OPT_umbrella);
   Args.AddAllArgs(CmdArgs, options::OPT_undefined);
   Args.AddAllArgs(CmdArgs, options::OPT_unexported__symbols__list);

clang/test/Driver/darwin-header-search-libcxx.cpp

 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1 %s
 //
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-cc1"
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-SYSROOT_AND_TOOLCHAIN-1-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
-// Make sure that using -nostdinc, -nostdinc++ or -nostdlib will drop both the toolchain
+// Make sure that using -nostdinc, -nostdinc++ or -nostdlibinc will drop both the toolchain
 // C++ include path and the sysroot one.
 //
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
 // RUN:     --target=x86_64-apple-darwin16 \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdinc \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-NOSTDLIBINC %s
 // CHECK-LIBCXX-NOSTDINC: "-cc1"
 // CHECK-LIBCXX-NOSTDINC-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-NOSTDINC-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 //
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
 // RUN:     --target=x86_64-apple-darwin16 \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdinc++ \
-// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
+// RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr_cxx_v1 \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-NOSTDINCXX %s
 // CHECK-LIBCXX-NOSTDINCXX: "-cc1"
 // CHECK-LIBCXX-NOSTDINCXX-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-NOSTDINCXX-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 //
 // RUN: %clang -### %s -fsyntax-only 2>&1 \
 // RUN:     --target=x86_64-apple-darwin16 \
 // RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     -isysroot %S/Inputs/basic_darwin_sdk_usr \
 // RUN:     -stdlib=platform \
 // RUN:     -nostdlibinc \
 // RUN:   | FileCheck -DSYSROOT=%S/Inputs/basic_darwin_sdk_usr \
 // RUN:               -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain \
 // RUN:               --check-prefix=CHECK-LIBCXX-NOSTDLIBINC %s
 // CHECK-LIBCXX-NOSTDLIBINC: "-cc1"
 // CHECK-LIBCXX-NOSTDLIBINC-NOT: "-internal-isystem" "[[TOOLCHAIN]]/usr/bin/../include/c++/v1"
 // CHECK-LIBCXX-NOSTDLIBINC-NOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v1"
 
 // Make sure we explain that we considered a path but didn't add it when it
 // doesn't exist.

clang/test/Driver/darwin-link-libcxx-from-toolchain.cpp

+// UNSUPPORTED: system-windows
+
+// Tests to check that we pass -L <install>/bin/../lib/ to the linker to prioritize the toolchain's libc++.dylib over system's libc++.tbd in all cases.
+
+// Check that even if stdlib is not passed, on apple platforms we pass the toolchain's dylib path to the linker.
+
+// RUN: %clang -### %s 2>&1 \
+// RUN:     --target=x86_64-apple-darwin \
+// RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
+// RUN:   | FileCheck -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx \
+// RUN:               --check-prefix=CHECK-TOOLCHAIN-LIBCXX-LINKING-1 %s
+
+// Check with -stdlib on both cases of toolchain installations (with and without libcxx).
+
+// RUN: %clang -### %s 2>&1 \
+// RUN:     --target=x86_64-apple-darwin \
+// RUN:     -stdlib=libc++ \
+// RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain_no_libcxx/usr/bin \
+// RUN:   | FileCheck -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain_no_libcxx --check-prefix=CHECK-TOOLCHAIN-LIBCXX-LINKING-1 %s
+
+// RUN: %clang -### %s 2>&1 \
+// RUN:     --target=x86_64-apple-darwin \
+// RUN:     -stdlib=libc++ \
+// RUN:     -ccc-install-dir %S/Inputs/basic_darwin_toolchain/usr/bin \
+// RUN:   | FileCheck -DTOOLCHAIN=%S/Inputs/basic_darwin_toolchain --check-prefix=CHECK-TOOLCHAIN-LIBCXX-LINKING-1 %s
+
+// CHECK-TOOLCHAIN-LIBCXX-LINKING-1: "/usr/bin/ld"
+// CHECK-TOOLCHAIN-LIBCXX-LINKING-1: "-L" "[[TOOLCHAIN]]/usr/bin/../lib"