This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] rework CMakeLists.txt into modules
ClosedPublic

Authored by martell on May 28 2017, 12:29 PM.

Details

Summary

libcxxabi was having issues with detecting flags in config-ix.cmakecorrectly when cross compiling.
I narrowed it down to changes as a result of including

list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_PATH}")
include("${LLVM_CMAKE_PATH}/AddLLVM.cmake")
include("${LLVM_CMAKE_PATH}/HandleLLVMOptions.cmake")

The best way to solve this was to rework CMake to use modules to mirror libcxx.
This makes the cmake source more readable and maintainable

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.May 28 2017, 12:29 PM
martell updated this revision to Diff 100562.May 28 2017, 12:40 PM

removed needless check and update comment

martell updated this revision to Diff 100563.May 28 2017, 12:54 PM

Added CMAKE_RUNTIME_OUTPUT_DIRECTORY like libcxx

martell updated this revision to Diff 100566.May 28 2017, 1:59 PM

remove unnecessary cmake policy

Sorry about the bumps with small changes.
Should be in good shape now.
Please Review.

CMakeLists.txt
325 ↗(On Diff #100566)

Just a note here. This doesn't do anything since rL288692 so removing it here.
I have put together a patch to enable different options for static and shared builds when they are both requested in the one cmake run so I will add that in a follow up differential.

martell added inline comments.May 28 2017, 3:48 PM
CMakeLists.txt
325 ↗(On Diff #100566)

I fixed this in rL304110.
Will update without removing this block for now to make this commit cleaner

martell updated this revision to Diff 100571.May 28 2017, 3:55 PM

rebased to master and refined commit to only change what it is intended to do

EricWF accepted this revision.May 31 2017, 3:19 PM

This is supposed to be NFC, right? If so it LGTM. @martell Do you have commit access or should I commit this for you?

This revision is now accepted and ready to land.May 31 2017, 3:19 PM

@EricWF What does NFC mean?
I have commit access :)

NFC == No functionality change.

This patch just re-factors the CMake stuff without adding or removing anything at the same time, right?

Thanks for explaining the acronym.
With respect to the project itself yes there is NFC but it does change the cmake functionality which fixes a few bugs because we do not depend on LLVM cmake modules anymore.

This revision was automatically updated to reflect the committed changes.

With respect to the project itself yes there is NFC but it does change the cmake functionality which fixes a few bugs because we do not depend on LLVM cmake modules anymore.

I'm not sure what this means? What modules do we no longer depend on?

@EricWF The HandleLLVMOptions.cmake module is no longer loaded

Urg... so all of the configuration done in that file is no longer handled?!? That's the opposite of a change that has no effect on functionality...

Urg... so all of the configuration done in that file is no longer handled?!? That's the opposite of a change that has no effect on functionality...

Correct. I would note however that libc++ does not depend on that module either.
I believe keeping libc++abi consistent with libc++ makes sense here.
Unless someone wants to reintroduce that dependency into libc++?

Urg... so all of the configuration done in that file is no longer handled?!? That's the opposite of a change that has no effect on functionality...

Correct. I would note however that libc++ does not depend on that module either.
I believe keeping libc++abi consistent with libc++ makes sense here.
Unless someone wants to reintroduce that dependency into libc++?

Libc++ also has some other configuration changes to make that work, like handling LLVM_USE_SANITIZER, which libc++abi doesn't right now but did before...

! In D33635#769584, @EricWF wrote:
Libc++ also has some other configuration changes to make that work, like handling LLVM_USE_SANITIZER, which libc++abi doesn't right now but did before...

Ahh I see, In that case we could add include(HandleLLVMOptions OPTIONAL) below include(AddLLVM OPTIONAL) in HandleOutOfTreeLLVM.cmake
I can either then re-evaluate why I have issues in cross compiling and either migrate the LLVM_USE_SANITIZER code to libcxxabi later or fix in another way.

I would note however that I haven't gotten a buildbot error from this change.
Is the sanitizer actually tested with libc++abi?

Ignore my previous comment, I misread your note about it not needing it anymore.

Ignore my previous comment, I misread your note about it not needing it anymore.

I didn't intend to say it wasn't needed anymore. It's still needed for out-of-tree builds.

I created D33753 to re add that support.
If you can approve I will push it on top.
Thanks for the feedback btw.