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.

LLVM generally has a policy of not leaving dead or unused code in tree. Changing to an error seems like the right approach given our coding guidelines.

Updates based on @winksaville's feedback

I don't know of anywhere that we use llvm_add_library to generate both static and shared libraries of the same code. The runtime libraries that are generated both ways roll their own solution to the problem.

I don't know that we want to support building component libraries as both STATIC and SHARED except in very specific and limited situations.

Adding information about distribution targets and LLVM_INSTALL_TOOLCHAIN_ONLY.

In D62040#1505692, @dschuff wrote:

This is great! Might it be worth mentioning LLVM_INSTALL_TOOLCHAIN_ONLY somewhere?

Why are the clang libraries being built STATIC and SHARED?

I will land this patch as-is today. I'll try and get a doc patch out for review today or tomorrow. @winksaville, I'm going to add a section to the document about building LLVM as a shared library for inclusion with tool distributions.

LGTM.

In D61909#1505083, @winksaville wrote:

Please add documentation if you want people to use it :)

I should note, this and many more aspects of the build system were topics I discussed in a talk at the 2016 LLVM Dev meeting when we switched off autoconf:
https://www.youtube.com/watch?v=StF77Cx7pz8

There is a simpler example distribution configuration, but sadly there isn't documentation. That is something I can fix.

In D61909#1503642, @winksaville wrote:

Sorry, maybe I didn't make myself clear.

In D61870#1503490, @serge-sans-paille wrote:

@beanz
As pointed out by @hintonda, current default argument is *never* explicitly used in the codebase.

In D61909#1503433, @winksaville wrote:

IMHO "BUILD_CLANG_DYLIB" is needed. As you have it now libclang_shared.so is always builds on UNIX systems, which I believe means that all linux distros would have both increasing their sizes.

This patch doesn't actually include how the new code will be used, and the review doesn't include enough context for me to understand what is going on.

In D61909#1502446, @winksaville wrote:

Questions:

• Should we only build libclang_shared.so if LLVM_BUILD_LLVM_DYLIB is ON?

LLVM_BUILD_LLVM_DYLIB is actually not the important option to think about because it has no impact on this, rather LLVM_LINK_LLVM_DYLIB. That really depends on what tradeoffs you want. With that option off LLVM will be linked statically into the Clang library, which in the common case is fine because you can resolve all the LLVM & Clang APIs against it. That will be faster to load, but a larger binary distribution. With the option on, libclang_shared will link libLLVM, which will result in slower loading times, but smaller distributions. Both are workable situations.

Changed to lowercase 'c' to match other clang libraries.

@winksaville whether or not PIC is required for shared libraries varies by platform. These days LLVM defaults to -fPIC, and I'm not sure we actually support running LLVM without -fPIC on systems that require shared libraries to be PIC.

@winksaville that was my bad. I left off the new files in the commit because I forgot git-diff doesn't grab untracked files...

My change should not have decreased build time from trunk, nor should it have reduced the number of build steps. That patch should generate lib/libClang_shared.so, which will export the C++ API. libClang is unchanged since changing it would break existing clients of the library.

In D61804#1499275, @winksaville wrote:

When you say you don't think the build system should do this, what do you mean?

As an additional note, Arch linux should not be building clang with BUILD_SHARED_LIBS nor should it be distributing those ,so files. That isn't a supported configuration for Clang deployment.

I question the general utility of this patch, and I don't really think this does anything we want the build system doing.

Using a macro seems reasonable if a bit icky.

I think the two of you are speaking past each other with different goals.

Could you change the title and commit message to reference that this isn't just about using llvm_add_library it is more broadly about making the benchmark library behave like a proper LLVM library target when built in-tree.

Yep. This looks like the right solution. Un-used install destinations are effectively ignored by CMake, so there is no reason to selectively choose only one.

Libc++'s test cases aren't buildable without LLVM. Why should the benchmarks be?

That kinda makes it worse. We don't duplicate gtest in all the tools that use it, why are we duplicating this library?

@lebedev.ri I actually disagree with prefixing capabilities check variables. If other projects are checking the same thing you want them to use the same variable so that they don't all check the same thing over and over again.

Updating based on review feedback.

This all looks reasonable to me. It is kinda frustrating that CMake doesn't support any way to control the output name of an object file, or even the install name of one. I suspect that is probably because some of CMake's generation targets (like Xcode) don't give CMake explicit control over output object file names.

Seems reasonable to me.

Is there a specific version range of GCC that this miscompile occurs with?

I really like this. Using + to add variants seems much widely useful.

Makes sense

@phosek, does LLVM’s runtime directory do this correctly?

LGTM

LGTM

It is expected that CMake will process all projects put under llvm/tools unless explicitly told not to. So, it is expected that it would process clang there. If you don't want it to process clang you need to set LLVM_TOOL_CLANG_BUILD=Off when configuring.

LGTM.

Sorry for being behind on this, but I don't think this is the right solution to the problem.

My $0.02 on the yaml discussion for tests:

From an Apple perspective this almost certainly needs @mtrent's eyes, as he owns our binary tools.

How are you instructing lit to not run the tests that rely on these tools? Doesn't seem like you are, so I would expect check-llvm to fail on a clean build configuration due to missing tools.

LGTM.

LGTM!