This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Copy C++ headers before configuring runtimes build
ClosedPublic

Authored by beanz on May 20 2019, 11:04 AM.

Details

Summary

On some platforms C++ headers are packaged with the compiler not the sysroot. If you don't copy C++ headers into the build include directory during configuraiton of the outer build the C++ check during the runtime configuration may get inaccurate results.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.May 20 2019, 11:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 11:04 AM
Herald added a subscriber: mgorny. · View Herald Transcript
smeenai added inline comments.May 20 2019, 11:15 AM
llvm/runtimes/CMakeLists.txt
9 ↗(On Diff #200336)

Super nit: I always either leave off the ${} or enclose it in quotes inside if to avoid potentially running afoul of CMake's weird expansion rules for if. It's probably unnecessary most of the time, bit it's easy enough to do.

43 ↗(On Diff #200336)

Typo: plae

46 ↗(On Diff #200336)

Any reason to prefer this to either cmake -E copy_directory or file(COPY)?

phosek added inline comments.May 20 2019, 11:47 AM
llvm/runtimes/CMakeLists.txt
44 ↗(On Diff #200336)

This is true for many other platforms, e.g. in our toolchain build it's the case as well. Can we do this unconditionally?

46 ↗(On Diff #200336)

Even better, could we factor out the logic for copying headers in https://github.com/llvm/llvm-project/blob/master/libcxx/include/CMakeLists.txt#L218 to a separate .cmake module (except for the generated __config part that can be only done after we've finished configuration) and then reuse it here?

beanz marked an inline comment as done.May 20 2019, 12:33 PM
beanz added inline comments.
llvm/runtimes/CMakeLists.txt
46 ↗(On Diff #200336)

I was using ditto because the code was Darwin-only and ditto has been on Darwin for every version of Darwin we support LLVM with, and ditto is way faster than cmake -E ...

If we want this generalized then it should be done with cmake -E ... I'll take a stab at that shortly.

beanz updated this revision to Diff 200345.May 20 2019, 12:42 PM

Updates based on review feedback.

compnerd added inline comments.May 20 2019, 1:35 PM
llvm/runtimes/CMakeLists.txt
213 ↗(On Diff #200345)

Can you add a TODO to migrate to file(COPY when we switch to CMake >= 3.7? That avoids the custom function.

beanz added a comment.May 20 2019, 3:56 PM

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.

beanz updated this revision to Diff 200372.May 20 2019, 4:07 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 4:07 PM
beanz added a subscriber: EricWF.

Looping in @EricWF.

beanz retitled this revision from [CMake] Copy C++ headers during config on Darwin to [CMake] Copy C++ headers before configuring runtimes build.May 20 2019, 4:09 PM
beanz edited the summary of this revision. (Show Details)

The only thing that Im worried about is cross-compilation accidentally picking up the libc++ headers that are built here (e.g. building on Linux for Linux with the host libc++ headers and building libc++ as a runtime). The rest looks pretty much a substitution and addition of runtime-libcxx-headers target to the runtimes build which is fine.

I dont see any problems with this change. But I'm not sure I have any interesting insight either.

smeenai added inline comments.May 20 2019, 5:23 PM
libcxx/include/CMakeLists.txt
215 ↗(On Diff #200372)

Typo: should be configuration

beanz updated this revision to Diff 200397.May 20 2019, 9:29 PM

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

compnerd accepted this revision.May 22 2019, 4:34 PM

I think that this makes sense - the one case that I had in mind is pretty convoluted and if it breaks, we can fix it then.

This revision is now accepted and ready to land.May 22 2019, 4:34 PM
This revision was automatically updated to reflect the committed changes.

Hello. I'm reasonably sure this broke build for me:
LLVM_ENABLE_PROJECTS=clang;clang-tools-extra;compiler-rt;libcxx;libcxxabi;lld;openmp
BUILD_SHARED_LIBS=ON

CMake Error at /build/libcxx/include/CMakeLists.txt:278 (add_custom_target):
  add_custom_target cannot create target "install-libcxx-headers" because
  another target with the same name already exists.  The existing target is a
  custom target created in source directory "/build/libcxx/include".  See
  documentation for policy CMP0002 for more details.


CMake Error at /build/libcxx/include/CMakeLists.txt:279 (add_custom_target):
  add_custom_target cannot create target "install-libcxx-headers-stripped"
  because another target with the same name already exists.  The existing
  target is a custom target created in source directory
  "/build/libcxx/include".  See documentation for policy CMP0002 for more
  details.

Is this expected? Do i need to purge the build dir? If not, can this please be reverted in the mean time?