This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Remove the LIBCXXABI_ENABLE_PIC option
ClosedPublic

Authored by ldionne on Jun 9 2021, 10:06 AM.

Details

Reviewers
phosek
sbc100
Group Reviewers
Restricted Project
Restricted Project
Commits
rGa0ae3b078914: [libc++abi] Remove the LIBCXXABI_ENABLE_PIC option
Summary

Instead, people should be using CMAKE_POSITION_INDEPENDENT_CODE to control
whether they want to use PIC or not. We should try to avoid reinventing
the wheel whenever CMake natively supports something.

This makes libc++abi consistent with libc++ and libunwind.

Diff Detail

Event Timeline

ldionne created this revision.Jun 9 2021, 10:06 AM
ldionne requested review of this revision.Jun 9 2021, 10:06 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2021, 10:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: sbc100.

Note that this option was added in https://reviews.llvm.org/D60049.

I was at most lukewarm about adding it but we still did, because there seemed to be a use case. @phosek just asked me why libc++abi was defining its own knob for enabling PIC, which led me to reconsider D60049 and to this patch.

@sbc100 Is there no way you can get by with setting CMAKE_POSITION_INDEPENDENT_CODE instead? In D60049 you mention that it's going to impact all the other LLVM components you're building, but perhaps that is reasonable? Also, if you're building a toolchain, you should be using the Runtimes build. I don't know whether you are, however if so, it should be possible to specify that option only when building the runtimes, not the tools themselves (and if that's not possible, then we should make it possible cause I think it's a valid and useful thing to have).

phosek accepted this revision.Jun 9 2021, 10:14 AM

LGTM for consistency with other runtimes.

ldionne updated this revision to Diff 350939.Jun 9 2021, 10:15 AM

Remove forgotten mention of LIBCXXABI_ENABLE_PIC

wasm-ld has since been improved so its now possible to link PIC objects into static binaries. This means this is way less of an issue for WebAssembly these days. Plus I no longer maintain the toolchain that was using these CMake files to build libc++abi.

So I think I fine with removing this option, especially in light the "Runtimes" build which IIUC any modern toolchain builders should be using anyway.

ldionne accepted this revision as: Restricted Project.Jun 10 2021, 9:25 AM
This revision is now accepted and ready to land.Jun 10 2021, 9:25 AM

wasm-ld has since been improved so its now possible to link PIC objects into static binaries. This means this is way less of an issue for WebAssembly these days. Plus I no longer maintain the toolchain that was using these CMake files to build libc++abi.

So I think I fine with removing this option, especially in light the "Runtimes" build which IIUC any modern toolchain builders should be using anyway.

Thanks for your input!

This revision was automatically updated to reflect the committed changes.