This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Do not share an object library to create the static/shared libraries
ClosedPublic

Authored by phosek on Apr 2 2019, 8:11 PM.

Details

Summary

This change is similar to r356150, with the same motivation. The
only difference is that the method used to merge libunwind.a and
libc++abi.a had to be changed to use the same approach as libc++
since we no longer produce object libraries that could be linked
together as we did before. We reuse the libc++ script for merging
archives to avoid duplication between the two projects.

Diff Detail

Repository
rCXXA libc++abi

Event Timeline

phosek created this revision.Apr 2 2019, 8:11 PM
ldionne requested changes to this revision.Apr 3 2019, 7:56 AM

I will close https://reviews.llvm.org/D60166 as being superseded by this.

libcxxabi/CMakeLists.txt
49 ↗(On Diff #193422)

Why are we dropping this?

This revision now requires changes to proceed.Apr 3 2019, 7:56 AM
phosek marked an inline comment as done.Apr 3 2019, 9:57 AM
phosek added inline comments.
libcxxabi/CMakeLists.txt
49 ↗(On Diff #193422)

AFAICT CMake automatically includes -fPIC for both shared and static library (I checked the generated build.ninja files) so this shouldn't be needed, or do you want to have an explicit option to disable -fPIC? If so, should there be a similar option for libunwind and libc++ as well?

ldionne added inline comments.Apr 3 2019, 10:03 AM
libcxxabi/CMakeLists.txt
49 ↗(On Diff #193422)

I don't think it does for static libraries. From https://cmake.org/cmake/help/v3.14/prop_tgt/POSITION_INDEPENDENT_CODE.html:

The POSITION_INDEPENDENT_CODE property determines whether position independent executables or shared libraries will be created. This property is True by default for SHARED and MODULE library targets and False otherwise. This property is initialized by the value of the CMAKE_POSITION_INDEPENDENT_CODE variable if it is set when a target is created.

I'm personally fine with relying on CMAKE_POSITION_INDEPENDENT_CODE to control that, but other folks wanted more control.

phosek updated this revision to Diff 193546.Apr 3 2019, 10:53 AM
phosek marked 3 inline comments as done.
phosek added inline comments.
libcxxabi/CMakeLists.txt
49 ↗(On Diff #193422)

It seems like the -fPIC I saw in build.ninja is added by LLVM, if I do a standalone build of libc++abi, it's not there.

phosek marked an inline comment as done.Apr 3 2019, 11:18 AM
phosek added a subscriber: sbc100.
phosek added inline comments.
libcxxabi/src/CMakeLists.txt
193

Related to discussion on D60049, should this be set for shared as well or only static? It's unclear to me whether that option should control both or just static? @sbc100 what was your intention?

Personally, I'd prefer to introduce:

cmake_dependent_option(LIBCXXABI_ENABLE_PIC_SHARED_LIBRARY "LIBCXXABI_ENABLE_SHARED;LIBCXXABI_ENABLE_PIC" ON)
cmake_dependent_option(LIBCXXABI_ENABLE_PIC_STATIC_LIBRARY "LIBCXXABI_ENABLE_STATIC;LIBCXXABI_ENABLE_PIC" ON)

Then use those dependent options to control PIC independently for shared and static libraries. WDYT?

sbc100 added inline comments.Apr 3 2019, 11:33 AM
libcxxabi/src/CMakeLists.txt
193

From a WebAssembly perspective we only needed a way to prevent the static library being built as PIC. Which we now have in the form of the LIBCXXABI_ENABLE_PIC=OFF option.

During review somebody pointed out that there are times when one might also want to disable PIC in shared libraries too so we had this option effect both which is probably fine with us too.

I'm hoping to fix the issue that is currently present in WebAssembly where PIC code can't linked into static binaries. If I'm not able to do that I'll most likely end up building everything twice creating a complete fork of the lib directory. e.g:

  • lib/wasm32-unknown-unknown/pic (includes .so files and .a files built with PIC)
  • lib/wasm32-unknown-unknown/nopic (includes only .a files *all* built without PIC)

Then the linker would then choose the lib directory to use based weather its building relocatable output or not.

If we end up doing that we don't need that much flexibility within a given build because we will be running cmake twice, once for pic and once for non-pic.

So basically, no I need that fine grained control for what I'm doing (yet).

phosek updated this revision to Diff 193554.Apr 3 2019, 11:40 AM
phosek marked an inline comment as done.
phosek marked an inline comment as done.
phosek added inline comments.
libcxxabi/src/CMakeLists.txt
193

I've changed it so that ${LIBCXXABI_ENABLE_PIC} controls both the static and shared case.

ldionne requested changes to this revision.Apr 3 2019, 12:56 PM
ldionne added inline comments.
libcxxabi/src/CMakeLists.txt
192

You need to do

if (LIBCXXABI_ENABLE_PIC)
  target_set_properties(... ON)
endif()

Otherwise, you're overwriting the default that's being set through CMAKE_POSITION_INDEPENDENT_CODE.

This revision now requires changes to proceed.Apr 3 2019, 12:56 PM
phosek updated this revision to Diff 193580.Apr 3 2019, 1:17 PM
phosek marked an inline comment as done.
phosek marked an inline comment as done.
phosek added inline comments.
libcxxabi/src/CMakeLists.txt
192

@sbc100 mentioned in the other comment that some users may want to have a flexibility to also disable -fPIC even for the shared case where this won't be sufficient, but I'm fine to punt on that until it's needed.

ldionne accepted this revision.Apr 3 2019, 1:46 PM

Thanks for the refactoring!

libcxxabi/src/CMakeLists.txt
192

I'm that user. I use CMAKE_POSITION_INDEPENDENT_CODE=OFF and I don't touch those stinking fine-grained controls.

This revision is now accepted and ready to land.Apr 3 2019, 1:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 1:58 PM