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.

In D68430#1693964, @jordan_rose wrote:

I'm not quite sure what it's doing. The executable targets end up trying to link against the static libraries anyway, which of course haven't been built. It's possible that this is because the LIBTYPE is both STATIC and OBJECT and if it were just OBJECT we might be better off, but I'm not sure if Xcode's IDE features will be happy with a target that doesn't actually produce a library. I can try it if you want, though.

Are these tools intended to be installed? I thought they were developer focused tools. I don't think we should make install targets for things that the project doesn't want to support publicly.

LGTM

This has the side-effect of making libclang_cpp effectively empty when you build with Xcode.

This isn't really possible to do because CMake doesn't have a mechanism for listing all targets. A more correct approach for this would require generating a ninja build, then you could use ninja -t list to dump all the targets, then you can post-process that list to find a full list of components.

In D67585#1670491, @mgorny wrote:

But in that case, we should aim for consistency, i.e. remove the matching option from LLVM.

In D67585#1670433, @mgorny wrote:

This is really much more work than disabling the one component I don't need or care for, especially when I do shared lib build and therefore the additional library is entirely useless.

In D67585#1670420, @mgorny wrote:

Sure but I want to build-test practically everything else. I wouldn't mind if this didn't basically kill my test system by resource exhaustion

Please no. You don’t need to build the library. ‘check-clang’ doesn’t depend on it, so it should not impact your build and test cycles. We want it included in the ‘all’ target for every possible configuration so that it gets tested and not broken as frequently as the LLVM dylib target has in the past.

Updates based on feedback from Lang.

@aaronpuchert sounds like a reasonable approach

I see. Sorry for the noise. This looks fine.

@compnerd, it doesn't look like LLVM_ENABLE_RUNTIMES passed globally is still passed through if you are generating runtimes for multiple targets. It looks like that only gets passed through if you specify RUNTIMES_${name}_LLVM_ENABLE_RUNTIMES.

I feel like the default behavior should be that if you set LLVM_ENABLE_RUNTIMES it builds them for all targets you request. If you want to disable some runtimes for certain targets that should be an opt-out setting rather than an opt-in to build for targets.

Landed in rL369486

I'm happy to commit this for you. Will do so shortly.

Seems reasonable to me. @anemet any concerns?

In D66068#1636240, @jvesely wrote:

sorry for the delay. I fully understand the need to reduce the number of options. Having always used SHARED_LIBS build I remember weekly shared build breakages.

In D65983#1633766, @cbiesinger wrote:

It actually worked with the env for me.

Sadly I'm not sure that there is a better way than generating the shebang line, but I can imagine some problems with the way this patch does it. PYTHON_EXECUTABLE could be a full path, which means that this can't be used to generate binary distributions because you'd be relying on python being in the same place on disk, which defeats the purpose of env being on the shebang.

This fix looks good, although I would suggest that when building libcxx, using the LLVM CMake exports should be more reliable than llvm-confit.

In D66068#1628617, @jvesely wrote:

It duplicates functionality provided by separate/component libraries.