Do we have any logic to emit an n_strx for an empty string? If so, that would need to be adjusted to emit 1 instead of 0. If not, LGTM.

Ping. Assuming everyone is okay with the direction, any comments on the change itself?

Both CMake and the driver (assuming it's invoked as clang++/g++) should take care of adding the C++ standard library, so this LGTM.

In D89177#2332675, @beanz wrote:

In D89177#2332627, @ldionne wrote:

That isn't what I meant. It's entirely okay for the runtimes to be driven via AddExternalProject like the runtimes build does, since that's akin to having a separate CMake invocation for each configuration. That's okay.

What I'm saying is that if the next logical step is to also add support for multiple distributions in libc++'s build itself (e.g. adding LIBCXX_<DISTRIBUTION>_ENABLE_SHARED & al), then I don't think that's a good idea.

Totally agree. That would be the path compiler-rt’s Darwin build goes, and it is a frequent problem.

This is needed to avoid potential race conditions where some of the C++ headers have been copied but not all of them, and some other runtime that doesn't depend on the C++ headers sees this incomplete state and gets a spurious build failure, so LGTM.

In D89177#2326875, @phosek wrote:

In D89177#2326832, @smeenai wrote:

In D89177#2325566, @phosek wrote:

We've already considered introducing a similar mechanism so thank you for working on this! There's one issue that I haven't figured out how to resolve and I'd be interested in your thoughts: building executables and libraries in the most optimal may require incompatible flags. For example, when building a shared library you have to use -fPIC but for executables that's suboptimal; similarly, when building executables you may want to use LTO but if you also want to create a distribution that contains static libraries, that's a problem because those will contain bitcode. So far our solution has been to simply do multiple builds with different flags (in GN this can be done in a single build by leveraging the "toolchain" concept that also allows sharing as much work as possible, but I haven't figured out how to do anything like that in CMake).

Good question. We need something similar as well, where some clients of the libraries want them built with RTTI (because their programs rely on RTTI and you can get link errors when linking them against non-RTTI LLVM libraries), but we don't wanna build the executables with RTTI because of the size increases.

We recently ran into this as well where we need RTTI for LLVM libraries but don't want to enable it for the toolchain.

I don't know of any way to do this in CMake other than just configuring multiple times with the different flags, like you said. Maybe we could implement some logic for that in the build system, but I'm not sure if it's worth it. (I'm wondering if the Ninja multi-config generator added in CMake 3.17 could be useful for this, although I haven't played around with that at all yet.) I do recognize it limits the usefulness of this approach quite a bit (since if you're doing separate configurations you can just do different LLVM_DISTRIBUTION_COMPONENT settings), but I'm hoping it can still be valuable for simpler scenarios.

One idea I had would be to eliminate the use of global variables like CMAKE_C_FLAGS and CMAKE_CXX_FLAGS and always set these via target_compile_options (which is something we should do in any case to move towards more modern CMake); since we always use functions like add_llvm_component_library to create targets in LLVM, we could then have these create multiple targets that would differ in flags like -fPIC or -frtti and set up dependencies among these appropriately. It means you'd have to build files multiple times (I don't think there's any way around that) but you'd save time on things like tablegen.

Address review comments

Reading up on the Ninja multi-config generator a bit, you can define your own build types, so you should be able to create e.g. one build type for LTO and another for PIC. You'd still need some external logic to define that you use the LTO mode for executables and the PIC mode for libraries, but the multiple distribution support should work nicely in conjunction with that.

In D89177#2325566, @phosek wrote:

We've already considered introducing a similar mechanism so thank you for working on this! There's one issue that I haven't figured out how to resolve and I'd be interested in your thoughts: building executables and libraries in the most optimal may require incompatible flags. For example, when building a shared library you have to use -fPIC but for executables that's suboptimal; similarly, when building executables you may want to use LTO but if you also want to create a distribution that contains static libraries, that's a problem because those will contain bitcode. So far our solution has been to simply do multiple builds with different flags (in GN this can be done in a single build by leveraging the "toolchain" concept that also allows sharing as much work as possible, but I haven't figured out how to do anything like that in CMake).

LGTM.

In D85140#2310910, @phosek wrote:

Also a styling nit, this should have been cmake/Modules, not cmake/modules. The former is used only by LLVM and is a historic relic, all other projects use cmake/Modules which is more common in CMake-based builds.

LGTM, but I'll give the sanitizer folks a bit of time to review as well.

I think the reason for this restriction is that on Windows, __declspec(dllimport) changes the compiler's code generation to assume that the symbol will be imported (i.e. if the compiler sees a call to a function X which is marked __declspec(dllimport), it'll generate an indirect call to __imp_X instead). If you're linking against libc++ statically, the imported version of the symbol will of course not exist, so you want to disable the __declspec(dllimport) annotations by default if you're building just the static library for Windows.

It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.

LGTM barring the comment about more tests.

LGTM

LGTM

In D88454#2304023, @phosek wrote:

@smeenai do you already have an alternative for your use case?

In D88627#2306231, @daltenty wrote:

This actually isn't exclusively a revert of D85329 "[AIX] Try to not use LLVM tools while building runtimes" as the title seems to suggest. Before that patch there were already explicit TOOLCHAIN_TOOLS specified in the call to llvm_ExternalProject_Add in the runtimes build, which already suppressed the target-specific tool selection as mention in the description here.

That said, I'm not opposed to the resulting change, which moves all that logic to one place rather than having an extra layer on top just for the runtimes build. We should clarify the title and description though, because removing the TOOLCHAIN_TOOLS here entirely will actually be a new change to how runtimes are built.

I was abusing this functionality to be able to get the C++ headers installed without having to build all the runtimes build prerequisites; we have a configuration where we only need the C++ headers installed (and not the library), and using the runtime-libcxx-headers was an easy way to get them without needing to first build Clang and all the other tools the runtimes build requires. I should be able to come up with something else for that use case though.

In D87199#2264781, @int3 wrote:

but would any of this be easier if SyntheticSections were InputSections instead of OutputSections

Maybe... we could make them InputSections and then generate unsigned relocations for each of their entries. There would be some additional overhead (e.g. the addend field would be useless) but it would allow us to handle the rebases in one place. One thing I'm not sure about is whether other architectures have an equivalent of X86_64_RELOC_UNSIGNED that can be used for this; it looks like arm64 does, but not sure about PPC (not that it really matters).

There are other benefits to having SyntheticSections be InputSections: there would no longer be a need for the SectionPointerUnion, and DSOHandle could just be an ordinary Defined symbol.

I actually can't think of many real downsides... do you have any in mind? That said, the current architecture is not terrible, and I'm inclined to work on implementing more features and pushing the refactoring till later.

The LLVMExternalProjectUtils change is needed either way, but I believe the WinMsvc case to be a CMake bug, and I've put up https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5264 to fix it. (We should still go ahead with it as a workaround for now though.)

https://gitlab.kitware.com/cmake/cmake/-/commit/55196a1440e26917d40e6a7a3eb8d9fb323fa657 is the relevant CMake change, and I believe we need to add logic to find llvm-lib. I'll play with that.

It seems strange for CMake to prefer "ar" to "lib" when targeting Windows. Do you happen to know the CMake change that's responsible?

You have a small typo in the commit message: s/ldd/lld/

LGTM. It's interesting that this is a thing.

LGTM

LGTM

LGTM

The test cleanups are really nice, but I'm not a fan of coding this sort of test-specific behavior (especially the path) into LLD itself :/ (I know we have some precedent with LLD_IN_TEST, but that's only used in canExitEarly and it's a pretty minor behavior change.)

LGTM

LGTM

LGTM

What was the conclusion for the comments about the priority level (100 vs. 101)?

In D87199#2262522, @int3 wrote:

Is there a funnel point that we know that all the rebase entries will pass through to ensure that we catch them rather than adding them at a couple of sites?

It may be clearer if we scan the GOT/LazyPointerSection inside RebaseSection::finalizeContents() for any absolute addresses, rather than relying on the code that's adding entries to either of those sections to also handle the rebasing. I will try that... however, there isn't a funnel point that will cover absolute addresses from both our SyntheticSections as well as from InputSections.

LGTM

In D86762#2244647, @amccarth wrote:

Is there a more specific term than "traditional linker"?

I understand that LLD's search strategy is uncommon among linkers in the Posix (and/or ELF?) realm, but it's not uncommon in other linkers, even very old ones. For example, the VMS (now OpenVMS) linker has used LLD's strategy by default for many decades. (It does allow opting in to the more restrictive search.) LLD's approach matches Microsoft's link.exe. I can't be sure, but I don't recall the so-called traditional behavior from linkers from other vendors of Windows, DOS, or CP/M linkers.

I have no objection to turning the warning on in appropriate contexts or with the argument that it helps to ensure good layering. I'm just quibbling over the term "traditional."

LGTM

LGTM; good catch.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM; this is a nice cleanup.

LGTM. Thanks for limiting re-exports to sub-documents in the same TBD!

LGTM

You're missing a test case.

LGTM

In D86359#2235358, @jhenderson wrote:

It looks like no documentation was added for the new option. Please add it!

I think llvm-libtool-darwin would also need some adjustments to handle bitcode objects, archives, and universal binaries. That's definitely out of scope for this diff, but I'm just flagging it as another tool with missing bitcode support.

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

In D86313#2229408, @phosek wrote:

In D86313#2229323, @smeenai wrote:

In D86313#2229269, @phosek wrote:

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

We could consider completely decoupling runtimes from targets. Originally, each runtime build was identified only by its target, so you'd use RUNTIMES_${target}_${variable} to set target-specific variables. That became insufficient when we introduced the support for multilib, so now you also have a name and you can use RUNTIMES_${target}+${name}_${variable}. We could switch to RUNTIMES_${name}_${variable} and it'd be the responsibility of the user to set the target explicitly if they want it (FWIW it's something we already have to do in our build anyway because of Windows where the CMake behavior when you set CMAKE_${LANG}_COMPILER_TARGET breaks the build). If not, they get the default behavior, matching the default case today. What do you think?

To clarify, what would be setting ${name} in this scenario?

It'd be set by user. Today we have BUILTIN_TARGETS and RUNTIME_TARGETS, we could either repurpose those and create new variables. You'd use it as follows:

list(APPEND BUILTIN_TARGETS "darwin")
set(BUILTINS_darwin_CMAKE_SYSTEM_NAME Darwin CACHE STRING "")
...
list(APPEND BUILTIN_TARGETS "linux")
set(BUILTINS_linux_CMAKE_SYSTEM_NAME Linux CACHE STRING "")
set(BUILTINS_linux_CMAKE_C_COMPILER_TARGET x86_64-linux-gnu CACHE STRING "")
...

Here darwin and linux are arbitrary names.

Check LLVM_RUNTIME_TARGETS as well

Okay, I would argue that this version of the change is much better than the current situation. In particular, it disallows the loophole of specifying a triple for a particular platform instead of "darwin", and it forces users of "darwin" to acknowledge they understand how the Darwin compiler-rt build works by setting a particular variable (and it still disallows multiple Darwin triples). Lemme know what you think.

Add explicit error and flag

In D86313#2229152, @beanz wrote:

I think this change makes configuration confusingly inaccurate. Since the compiler-rt Darwin build does not support picking and choosing which targets to configure making the configuration fail if a user tries to is the reasonable approach.

Making the build do something different and unintuitive from what the configuration requested seems undesirable to me.

Thank you!

rG402b063c8067 should fix it. Sorry for the breakage!

That didn't work, but I'm able to repro locally using the CMake cache file used by the builder, so I'm digging into it.

I pushed rG12b4df991950 as a speculative fix. I'll monitor the bot.