Sorry for not seeing this. Ignoring macho_embedded is the right call. That is really just used for macho baremetal and not many people outside Apple would bother with it.

LGTM.

In D74588#2660401, @dsanders wrote:

I then went with the object library so that CMake would build the objects that I could then link as part of llvm-tblgen instead of being a proper library but the bots running older CMake's objected to that.

In D74588#2659067, @aaronpuchert wrote:

• maybe it would be confusing to have an LLVM component in llvm/utils instead of llvm/lib, and

In D74588#2658766, @aaronpuchert wrote:

So apparently we do import the LLVM dependency from LLVMTableGenGlobalISel .

In D74588#2654289, @aaronpuchert wrote:

In D74588#2654185, @beanz wrote:

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

The commit message of rL373651 says

Fixed the -DLLVM_LINK_LLVM_DYLIB=ON using DISABLE_LLVM_LINK_LLVM_DYLIB when creating the library. Apparently it automatically links to libLLVM.dylib and we don't want that from tablegen.

I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.

Yes, clang-tablegen could link libLLVM. That said, in the common case of building LLVM & Clang together (which is generally the advised way to develop), depending on libLLVM adds a bunch of extra unneeded dependencies on clang-tablegen, and constrains the build tool’s ability to parallelize builds.

Woo! Thanks for cleaning up one of my old messes :)

You can’t make tablegen link the LLVM dylib since parts of the dylib depend on tablegen.

The *_LTO and *_LLD variables are usable independently of each other even on non-Apple platforms, so it makes sense to need to set them both. LLVM’s LTO is supported in Gold, and I believe our build system supports MSVC’s LTCG (which is their version of LTO).

If you add include(CMakeDetermineSystem) to the top of the cache file, it should initialize CMAKE_HOST_SYSTEM_PROCESSOR, which also probably gives you a reasonable value to key off.

In D89942#2629184, @tbaeder wrote:

But that's not true. The 3-stage-base.cmake sets BOOTSTRAP_LLVM_ENABLE_LTO to ON, which doesn't work on SystemZ. This is the initial problem I had. I was not interested in having LTO enabled or not, but the cache enables it and that breaks on SystemZ.

I'm super sorry to be popping in so late on this. Is this really the direction we should take?

LGTM. I should have updated that in my patch.

This should not impact the Apple Clang releases since those are always clean builds.

In D98291#2615663, @delcypher wrote:

The fact that we have 3 different ways to build compiler-rt in LLVM makes me sad. There should only be 1. I don't have time to do it now but I've filed a radar to do this work at some point (rdar://75248447).

+1,000

So... this patch is fine. (marked as approved). That said, we should really be working to remove the LLVM_BUILD_EXTERNAL_COMPILER_RT option entirely in favor of just using LLVM_ENABLE_RUNTIMES=compiler-rt. That build process does support invoking the sub-project build on build invocation to make sure builds get re-run, and it supports better understanding of the dependencies between runtimes.

All of my major concerns about adding additional build systems are addressed by the support policy and classifying additional build systems as “peripheral”.

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.

In D89177#2332547, @ldionne wrote:

Are the runtimes expected to support this multi-distribution configuration? I don't think we'd want to move libc++ towards building multiple configurations at once in a single CMake invocation -- it's already too complicated to build just one configuration.

LGTM

In D88990#2317562, @serge-sans-paille wrote:

In D88990#2317447, @beanz wrote:

Using lld on Darwin isn't ideal, the system linker is desirable there.

What about making it the default on Linux and Windows?

Using lld on Darwin isn't ideal, the system linker is desirable there.

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.

The CMake code looks good to me. If @aaron.ballman is on board with it from the Windows perspective it sounds like this is the right fix.

Updated to also pass the linker -object_path_lto

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

Fixing bad test case.

The clang fix -> https://reviews.llvm.org/D84565

I was talking to @bogner about this. I think this patch is reasonable, but it is important to note that this patch (and the previous version) are both workarounds for a bug in clang. Anytime the clang driver is provided a flag to generate debug info and it is linking temporary object files, clang should insert a dsymutil step. Right now it only inserts the step if there is a compiler or assembler input to the compiler. I filed a bug to track it (https://bugs.llvm.org/show_bug.cgi?id=46841), and I will upload a patch to the clang driver to fix this issue.

In D83372#2172411, @lattner wrote:

What happens when someone creates a new managed static after this? This should work. Can you reset the mutex_init_flag somehow?

Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.

Logically your patch here looks fine. I'd rather see it use a std::lock_guard as the original code did, with a nested scope added, but that is pretty nitpicky.

There are a lot of downsides to global constructors. One is that they are difficult to debug because the code runs before main, so if anything goes wrong callstacks are odd, debuggers sometimes struggle to attach, and general headaches ensue.

In D83372#2139641, @lattner wrote:

Does this add a static constructor?

LGTM

It is kinda bad form to merge a change that has unaddressed feedback.

I have several concerns about this.

looks reasonable

Sorry for being late to the party on this.

I'm not sure this is actually the right approach. Adding the directory's INCLUDE_DIRECTORIES option also adds any path added with include_directories, which includes system paths and other locations that shouldn't ever have tablegen files.

I think this has some unintended consequences. If your tool wants to use libLLVM and libClang you really need libClang to be linked against libLLVM, otherwise you're actually just hiding the problem.

Yea... this is not how plugin linking should work.

Seems reasonable.

Looks totally reasonable to me, and like a good simplification of the logic.

In D72278#1806013, @DavidSpickett wrote:

I found this running lit tests on a Mac that doesn't have sysctl available. Not very familiar with it so I'm not sure why that is the case for our particular machine.

So this change is me assuming the intention was to just print and carry on if "sysctl" fails.

I think this is good, and it has been sitting a while.

LGTM.

I conferred off-list with @compnerd. He and I talked through my concerns, and I believe this patch is fine as-is, so LGTM.

@labath sorry for not replying on-list. I've actually been discussing offline with @compnerd. If llvm-config is printing an absolute path, I'm concerned about how that will interact with binary package distributions for the llvm-dev packages. Not sure if @tstellar has any thoughts on that.

I like the idea behind this. I provided more detailed feedback to Don off-list, but I really don't like the const void* parameter to the callback functions. I think we can make that match the parser data type, and have compile-time errors for type mismatches. That would be vastly preferable to casting void pointers.

Given that, this change LGTM. Using an extra argument would be nice, but this is a macro, not a function, and IIRC CMake argument parsing in macros doesn't really work, so that would be a big change.

When cross compiling any LLVM-based project we do not rely on installed copies of table gen, instead we configure and build host tablegen tools. That functionality in the LLVM build system is flexible enough to support any sub-project that provides its own tablegen tool.

Yea. Unfortunately the migration stalled out because there was no way to make the registration method work with the new pass manager.

This looks like a great cleanup to me.

LGTM.

LGTM.

LGTM

I very much dislike using EXCLUDE_FROM_ALL. I really believe all should be all, not most things I think are important. In general iteration cycles the check-* targets won't build or link libLLVM unless you specify LLVM_LINK_LLVM_DYLIB, and if you actually want to build everything as a means of testing your changes the all target should do that.

This should only be broken for CMake 3.14 and later. We should include that in the comments around this change.

https://gitlab.kitware.com/cmake/cmake/issues/19227

Reviewing the CMake change more closely, that is a bug. They shouldn't have changed the default behavior in that way. At the very least that change should have been a policy change.

I suspect this behavior change was caused by: https://gitlab.kitware.com/cmake/cmake/commit/b0b820ea39fe4edaf0672ba899a7042ef93a20d2

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

LGTM

In D69817#1734876, @vsapsai wrote:

I don't think that's the right fix as it moves a module map from representing a project structure towards having a random set of headers. But even after excluding 3 headers there are errors

Yea, that makes sense. Sorry again for the run around.

RPCUtils.h and RPCSerialization.h can both also be excluded.

Adding exclude header "ExecutionEngine/Orc/OrcError.h" to the module specification for LLVM_ExecutionEngine seems like a more appropriate fix.

Sorry, I had missed the atomic use directly in the tool. Sorry for my incorrect statements. This seems like a reasonable fix.

This confuses me. LLVMOrcError only directly depends on LLVMSupport. It doesn't include or need anything from ExecutionEngine.h.

Rebasing and updating to separate arm32 and arm64.

LGTM

A few comments inline.

In D69356#1727451, @daltenty wrote:

The intention of LLVM_SUPPORT_PLUGINS was as an internal option set by tools which may have plugins and need to be built in a specific way (such as avoiding deadstriping) if plugins are enabled.

In D69356#1726527, @hubert.reinterpretcast wrote:

For your second question: As the patch author indicated, the change to adjust the behaviour on the affected platform is forthcoming. Your question seems predicated upon that not happening.

In D69356#1726336, @hubert.reinterpretcast wrote:

Is there some documentation indicating these other use cases?

LLVM_NO_DEAD_STRIP isn't actually useful only for when plugins are enabled. It is also useful in a lot of contexts around building JIT host processes. The advantage of the old name was it specified exactly what it did (i.e. not dead strip code).