Adding ‘FORCE’ makes this not user overridable, which is the whole point of this being a cache variable.

This is fine with me. I have no real attachment to the name.

LGTM.

Yea, this makes sense even if it is a bit sad. We do use the --whole-archive approach for libLLVM, and it works. I was hoping to avoid it here in part to free up the ability to link libclang_shared before or in parallel to archiving the libclang*.a archives.

LGTM.

One thought, we should probably evaluate why those tools are using add_library instead of add_llvm_library. The right fix might be to move them over. This is exactly the kind of behavior that add_llvm_library and llvm_add_library were designed to handle.

I don't like the idea of putting this in llvm_update_compile_flags. That function does exactly what its name says, and adding additional behavior to it seems undesirable. Where are we creating libraries with add_library that we need this?

Reading through the old comments on this thread I think there are a few things going on here, but the core of the issue is you're using BUILD_SHARED_LIBS *outside* LLVM's build system. That is explicitly not supported (https://www.llvm.org/docs/CMake.html). We do not support BUILD_SHARED_LIBS for distributing LLVM in any form.

This is definitely not the right fix. We don’t want to set everything as Public dependencies. Our build system’s model for dependencies is that each component specified what it directly uses which is its Private dependencies. If you are having issues that probably means missing dependencies.

One comment inline. Otherwise LGTM.

I really don't think this is the right solution.

LGTM

I have a thought for how we can make the CMake interface for this better. Instead of LLVM_EXTENSIONS being a global property if it were a pre-defined list like LLVM_ALL_PROJECTS, then the LLVM_LINK_<X>_INTO_TOOLS variables can be generated by LLVM's configuration instead of relying on Polly's to run. If you do that LLVM_LINK_POLLY_INTO_TOOLS could be set to On even if Polly isn't enabled to build, but that actually will be fine if you change the calls to target_link_libraries in the tools to use a generator expression something like: $<$<TARGET_EXISTS:Polly>,Polly>.

This is good to land.

Looks reasonable to me.

In D62353#1516322, @hintonda wrote:

Btw, where would you want to put them?

Pre-monorepo, it would depend on what the bot is building, but with the mono-repo we could just create a new top-level directory to contain them all. Name each file to match the bot config it runs.

LGTM.

I have the same concerns as @labath, so we should certainly put it behind a flag.

@winksaville I've figured out how to resolve the gtest issue, but unfortunately that isn't good enough to get the check-runtimes target working. A change went in back in February (rL353601), which breaks running compiler-rt's tests when building a distribution in non-trivial ways, which will unfortunately be difficult to resolve.

I know this is an old change, but there are actually some big problems with this. Compiler-RT in particular uses COMPILER_RT_STANDALONE_BUILD=Off to basically mean clang is a target in-tree to depend on. This completely breaks being able to run the compiler-rt tests from the runtimes directory because a bunch of the tests add dependencies on clang. See clang_compile and its uses in CompilerRTCompile.cmake.

In D62279#1514467, @winksaville wrote:

Even with both gtest patches it still isn't working, ninja check-all is failing as before :(

Makes sense to me.

In D62279#1514046, @winksaville wrote:

I've add gtest_main and gtest to CLANG_BOOTSTRAP_TARGETS as a guess, because that's where check-all is defined:
...
My guess is likely wrong, what do you advise?

In D62215#1512849, @winksaville wrote:

IIRC, on linux if LTO is ON I had to use ld.gold for both stage 1 and 2, although that maybe
just the way it ended up and it wasn't necessary. But I'll test whatever you come up with.

A very interesting idea. A few thoughts inline.

Fixing this patch

Just came to my attention this explodes on some versions of CMake. I'm looking at it and will update once I find a better approach.

D62269 should fix the missing dependency on gtest.

In D62215#1512543, @winksaville wrote:

With this error I guessed that I needed to build LLVMgold.so, but then I also determined
I needed to link everything with ld.gold to complete the build process, which is why I
uploaded this patch.

@sbc100, I think I see what I did wrong in my last attempt at a fix. I was reading the wrong CMake property. rL361334 should resolve that. Can you give it a try and let me know if that resolves the problem for you?

In D61909#1510975, @E5ten wrote:

@beanz But if libclang_shared is intended to be a shippable binary and BUILD_SHARED_LIBS is only intended to be an option used in developer builds, and libclang_shared while not causing conflicts (thanks for the info on how that works by the way) would be redundant in a local developer build one would see BUILD_SHARED_LIBS enabled in wouldn't it? And also to me it doesn't really make sense to enable the intended shippable binary in a build that is specifically enabling a developer option, and so is obviously not intended for shipment (I get that in the arch case it is intended for shipment but that case is them using an option they shouldn't not the option being one that should be used when the built products are intended for redistribution).

@sbc100 they do not reproduce for me locally. They are Linux-specific and I’m on Darwin.

In D61909#1510955, @E5ten wrote:

@beanz Well I took libclang_shared as effectively an equivalent to the libLLVM.so that's created with that dylib option, and when BUILD_SHARED_LIBS is enabled that library is not created, in fact the option to create that library conflicts with BUILD_SHARED_LIBS.

That conflict is because of how I implemented libLLVM, and is something I'm looking to change. I'm hoping to put up a patch this week that will update how we generate libLLVM to be more like how libclang_shared is built.

Adding "libcxxabi" to LLVM_ENABLE_RUNTIMES is fine, but the other changes to the DistributionExample are only needed because you chose to use gold, which is a configuration-specific decision that is not representative of how most people will build, therefore it shouldn't be in the example.

One small change needed, otherwise LGTM.

@Bigcheese, ping?

It is silly that CMake doesn't expose the ninja version, but it is easy enough to get it. It should be something like:

This has had plenty of time to percolate, so I'm going to land it shorty. Any additional feedback we can incorporate in follow-up patches.

@E5ten, I just pushed r361271, which should resolve your issue.

@E5ten, I see what is going on. Give me 30 minutes and I’ll have a fix pushed.

I have a long story about this issue... Ask me about it sometime :).

@hintonda also, re-running the CMake command line and setting -D... is always a FORCE

@thakis Didn't we patch Ninja for this?

The CMake goop all looks reasonable to me. I think the new "correct" way to alter the compiler flags is add_compile_options, but since LLVM pretty heavily relies on modifying CMAKE_<LANG>_FLAGS it is probably better to be consistent than to start doing something new here.

Thank goodness @smeenai can spell because obviously I can't.

Looping in @EricWF.

A new take on how to do this, even more fun this time!

I just came up with a different and slightly crazier approach to this. I'm going to test it out and will upload a new version of this patch shortly.

Updates based on feedback from @smeenai

Updates based on review feedback.

Adding @zturner in case he has any thoughts.

@winksaville I just committed an update to the DistributionExamples a minute ago that should get them working as long as you aren't using a Darwin host. If you are using a Darwin host you need D62155 as well.

@winksaville, sorry those cache files pre-date the monorepo, and they haven’t been updated to support it. I will try and get a patch to update them today.

The main thing is to make sure that if you have proper integer sized members they are properly aligned and packed, which this seems to do well.

This is pretty cool. Since these are usually allocated as globals, this is saving pages of dirtied memory.

LGTM. Makes sense.

@sylvestre.ledru, I’ve added you on D62040, which is an attempt to document best practices for generating releases.

I would leave it out of any distribution (at least for now).

In D62040#1506832, @hans wrote:

Hmm, I've never set LLVM_ENABLE_RUNTIMES, I just build with a previously built clang. The script lives in utils/release/build_llvm_package.bat

In D62040#1506826, @hans wrote:

The Windows packages currently includes at least the asan runtime (and a bunch of other stuff like openmp that people ask for but I'm never quite sure if it works).

@hans I have some ideas on how to make package work. There is a simple solution to make it work as long as you're not using any runtime components, but I have an idea on how to make it work with runtime components.

Fixing the wording around LLVM_INSTALL_TOOLCHAIN_ONLY

In D62040#1506176, @hans wrote:

But the install target is kind of a standard cmake thing; wouldn't it be better if we made that target good? That's also what the cmake "package" target uses, which is what we use to build the Windows installer.