Page MenuHomePhabricator

[CMake] don't build libLTO when LLVM_ENABLE_PIC is OFF
ClosedPublic

Authored by sugak on Feb 9 2016, 3:30 PM.

Details

Summary

When cmake is run with -DLLVM_ENABLE_PIC=OFF, build fails while
linking shared library libLTO.so, because its dependencies are built
with -fno-PIC. More details here: https://llvm.org/bugs/show_bug.cgi?id=26484.
This diff reverts r252652 (git 9fd4377ddb83aee3c049dc8757e7771edbb8ee71),
which removed check NOT LLVM_ENABLE_PIC before disabling build for libLTO.so.

I've verified that when cmake is run with -DLLVM_ENABLE_PIC=ON, libLTO.so is
still generated, as well as when this option is not specified and the default
value (ON) is used. Tested both on master and release_38.

Diff Detail

Repository
rL LLVM

Event Timeline

sugak updated this revision to Diff 47383.Feb 9 2016, 3:30 PM
sugak retitled this revision from to [CMake] don't build libLTO when LLVM_ENABLE_PIC is OFF.
sugak updated this object.
sugak added a reviewer: beanz.
sugak added subscribers: hans, cfe-commits.
sugak edited subscribers, added: llvm-commits; removed: cfe-commits.Feb 10 2016, 9:04 AM

That makes sense to me, but I'm not sure what Chris meant when he wrote: "Autoconf builds libLTO with -fPIC, CMake should be able to as well", it seems his change in r252652 enabled building libLTO *without* PIC?

sugak updated this object.Feb 10 2016, 9:19 AM
sugak added a comment.Feb 10 2016, 9:43 AM

@joker.eph: yes, in environments that don't set CYGWIN, after r252652 libLTO is always built, regardless of LLVM_ENABLE_PIC.

hans added a comment.Feb 10 2016, 11:03 AM

This seems reasonable to me too, but it would be good to hear what Chris thinks.

beanz accepted this revision.Feb 11 2016, 1:29 PM
beanz edited edge metadata.

The LLVM_ENABLE_PIC option is oddly implemented on Darwin, and is really just broken. I have on my todo list to make better sense of it on Darwin.

This patch looks good, and should land.

Thanks,
-Chris

This revision is now accepted and ready to land.Feb 11 2016, 1:29 PM
sugak added a comment.Feb 11 2016, 7:38 PM

I need someone to commit this for me.

Thank you!

This revision was automatically updated to reflect the committed changes.
hans added a comment.Feb 12 2016, 11:07 AM

And merged to 3.8 in r260704.