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).

LGTM

You don't need to add the library to dsymutil. It is not code in dsymutil that depends on libatomic, it is code in LLVMSupport that dsymutil is using. By fixing this in LLVMSupport, any library linked against LLVMSupport will have a linkage to libatomic added.

Everything about this change seems good to me, the only thing I'd suggest is that we add a documentation note about CMAKE_CXX_STANDARD to docs/CMake.rst.

LGTM

I'm not sure this is the correct fix.

In D68833#1706315, @ldionne wrote:

Yes, precisely. That's also the currently preferred way of building libc++: https://libcxx.llvm.org/docs/BuildingLibcxx.html

In D68833#1706104, @ldionne wrote:

CMake tracks dependencies between targets, but not between directories. If the CMakeLists.txt in some directory (e.g. <root>/libcxx/) needs a target defined in another directory (e.g. <root>/libcxxabi/), one has to make sure that libcxxabi's CMakeLists.txt is included before libcxx's CMakeLists.txt. This isn't new or vexing, IMO. If that is what this patch ensures (I don't know the runtimes build very well), I think this is good.

Two thoughts.

This patch causes lots of warning spews because it adds virtual methods to a class that doesn't have a virtual destructor.

Accidentally uploaded this

In D68448#1694284, @sylvestre.ledru wrote:

More dynamic, less static usage :)

LGTM.

fair enough

That's fine. That said, there are things that just can't be done, or don't work well, in the Xcode and Visual Studio generators, so we have precedent for disabling functionality based on those generators.