Looping in @davide and @JDevlieghere. LLDB has some hand-rolled goop in debug server's build that could potentially be replaced by this.

LGTM

LGTM!

I prefer DEPS to COMPONENTS because I've tried to explicitly use the term components to mean targets that have paired build and install targets. Otherwise looks good.

Looping in @bruno in case he has feedback.

This all looks reasonable to me. I had a few small comments below. There are a few isolated bits of code cleanup that you included in this patch, and I'd prefer to see those land separately.

@dexonsmith is often super busy, so @bruno may be able to weigh in instead.

Historically we've duplicated cmake_policy calls on a per-project basis which we needed to support standalone builds. That said, it would be nice if we had LLVM vend some CMake modules that encapsulated this stuff better. In general I think that cleanup would be ideal to do after we move LLVM to the GitHub mono-repo because then we can create a cmake/modules folder at the root of the mono-repo that has common modules useable by all sub-projects.

LGTM

LGTM

This looks good to me. Please correct me if I'm wrong, but it also looks like you've written this patch so that it will work correctly even if it is landed before the llvm-readobj changes are landed. If that is the case go ahead and land this.

For reference this relates to D41673. Looking in @kubamracek.

LGTM

I like @phosek's idea of moving the files into /share, but that needs some accompanying changes to clang to find those files in their new location.

You should split the CMake cache file you created into two files, (1) a CMake Cache to manage the build configuration and (2) a tool chain file for targeting Linux. As @semeenai pointed out we absolutly want the behavior of the toolchain file being loaded multiple times. That is the correct way this build should work.

LGTM

I'm going to nitpick here a little. The patch is fine, but one of two things should change. Either line 29 should use list(APPEND...) instead of set, or you should do something more like:

A few inline comments. Overall I like the direction.

@phosek not sure if there is an equivalent. Probably strip -x is the closest.

@phosek On darwin ld64 supports the flags -all_load and -force_load which work similarly to --whole-archive. The ld64 manpage has the documentation for those options.

This patch LGTM.

Abstracting check for whether or not it is safe to remove an instruction as per the feedback from davide.

LGTM

LGTM

LGTM.

LGTM.

LGTM

This is a workaround predating CMake 3.1 when they fixed the underlying issues with the expansion of variables in STREQUAL. See: https://cmake.org/cmake/help/v3.1/policy/CMP0054.html.

LGTM.

LGTM

LGTM.

Yes, that comment is very out of date. The compiler-rt standalone build is essential to many of the users. It is quite common to build and use compiler-rt without LLVM (like sanitizer support in gcc).

The docs changes look good to me. I think if we want to deprecate llvm/projects for runtime libraries, we should have a discussion on llvm-dev & cfe-dev. I think too few people are using llvm/runtimes today as part of their regular builds for me to feel comfortable deprecating llvm/projects without giving some notice first and getting more people to try the runtimes directory.

This change isn't safe. Compiler-RT is buildable without LLVM's modules as long as you disable the tests, so you can't use an AddLLVM function inside AddCompilerRT unless it is only used when tests are disabled.

LGTM.

LGTM.

LGTM.

LGTM. It is a little strange that CMake generates the install/strip target automatically, but won't let you put / in a target name, but there really isn't anything to be done about it.

LGTM.

LGTM!

So, I wasn't actually familiar with how CMAKE_INSTALL_DO_STRIP worked, so I went and researched it. I think we actually should mimic how that works. So instead of having an option that enables stripping on the install targets, we should create two targets, one that strips and one that doesn't. Then the user can select whether or not they want their install contents stripped by choosing which target they run.

Is there a reason to add a new option instead of just adding -DCMAKE_INSTALL_DO_STRIP=${CMAKE_INSTALL_DO_STRIP} to the install commands?

Seems reasonable to me.

LGTM.

LGTM.

LGTM!

LGTM!

If we're setting the policy to NEW we should also be setting BUILD_WITH_INSTALL_NAME_DIR to match BUILD_WITH_INSTALL_RPATH, otherwise we're making builds using CMake 3.9 and later build differently than earlier versions of CMake which is undesirable.

LGTM

I don't see anything wrong with this patch as a workaround, but it seems like there might be a bigger issue here.

One comment below. Otherwise LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

@phosek, I would love to see compiler-rt fully refactored so that we target one triple at a time always. I'd also love to see the clang driver refactored so that the code to resolve paths to runtime libraries was shared so we follow the same conventions across all driver implementations.

LGTM.

LGTM.

@mstorsjo that is the problem that the LLVM/runtimes directory solves. It supports building all the runtime libraries as a single build with one build-tree per target and the management of separate build trees is handled by the build system.

One small nitpick, otherwise LGTM!

LGTM

Generally we frown on commit messages as concise as your description here. Might be worth a quick sentence or two explaining why cmake_parse_aguments is better as a breadcrumb for anyone who comes along in the future.

The code path you're modifying isn't just for building optimized tools, it also gets triggered when building the host tools in cross compilations. It is totally reasonable that I might want to use a different linker for host binaries from the linker used for target binaries. One example would be if I'm cross compiling to say PPC and wanting to use gold, but want to use lld as the linker for the host tools.

I think leaving VERSION as it is today, but changing SOVERSION to be ${LLVM_VERSION_MAJOR}${LLVM_VERSION_SUFFIX} makes the most sense to me. That should meet your needs.

Rather than hand rolling ar commands, could we make libunwind, libcxxabi, and libcxx produce object libraries, then we could just create normal static library targets with the object libraries as sources.

Ok. Rather than adding a new option I think you should just add LLVM_VERSION_SUFFIX to the SOVERSION property, then that should work for you.

I don't think we can do this unconditionally like this. It seems to me like you could have a situation where you want to use different linkers for the host tools and target binaries.

Sorry for the long delay on this review. It keeps getting lost in my inbox.

Looks reasonable to me

Who is the "we" you mention in your commit message? This seems to me like a patch very specific to a single downstream user, and you describe it as temporary, so I'm not convinced we should take it upstream.

LGTM.

LGTM.

LGTM. In the future you can just commit this kind of patch without review. It definitely falls under the category of obvious good fix.

Sounds reasonable to me. The added complexity of not passing it doesn't seem worth it.

One comment inline below.

Sorry I've been bad at staying on top of your patches.

LGTM.

LGTM.

LGTM. I keep meaning to spend time rewriting all this to use the LLVMExternalProjectUtils module which avoids issues like this, but I never really have the time to do it.

LGTM. I am curious what your use case for adding and removing options is, can you elaborate?

LGTM

Building libcxx without LLVM's CMake modules is very important, not just to Apple. This is how several open source distributions work, and it would be a huge disservice to break this. Same is true for compiler-rt, libcxxabi, libunwind, etc.

You can put the include inside the function body of configure_compiler_rt_lit_site_cfg, but you can't put it just inside the file. By design AddCompilerRT is mostly usable without AddLLVM.

LGTM. Please watch the bots closely with this change because CMake can be used in strange ways.

LGTM!

LGTM!

I'm not sure this change is sufficient to do what you're intending. I think you also need to pass the library path of the cxx library into the bootstrap configuration otherwise it will use the one from the sysroot.

LGTM!