Page MenuHomePhabricator

[libc++abi] Don't set POSITION_INDEPENDENT_CODE when building static library
ClosedPublic

Authored by sbc100 on Mar 29 2019, 11:55 AM.

Details

Summary

With the current WebAssembly backend, objects build with -fPIC are not
compatible with static linking. libc++abi was (mistakenly?) adding
-fPIC to the objects it was including in a static library.

IIUC this change should also mean the static build can be more efficient
on all platforms.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Mar 29 2019, 11:55 AM

@ldionne recently addressed this for libc++ in D59248. I already mentioned in the review that we should make the same change for libc++abi and libunwind. This would address your problem but also make libc++abi and libunwind build more aligned. I'm happy to prepare that change unless you want to do it?

@ldionne recently addressed this for libc++ in D59248. I already mentioned in the review that we should make the same change for libc++abi and libunwind. This would address your problem but also make libc++abi and libunwind build more aligned. I'm happy to prepare that change unless you want to do it?

Sure thing please go ahead. Is there something you would different to this change? If not, any reason to land this first/now?

phosek accepted this revision.Mar 29 2019, 2:49 PM

LGTM, I think it's fine to land this one in the meantime,

This revision is now accepted and ready to land.Mar 29 2019, 2:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 3:08 PM
pcc added a subscriber: pcc.Mar 29 2019, 3:39 PM

It's quite common to statically link libc++abi.a into a PIE or shared object. Doesn't this change break that scenario?

In D60005#1448534, @pcc wrote:

It's quite common to statically link libc++abi.a into a PIE or shared object. Doesn't this change break that scenario?

Oh. I see. Is that true of libc++abi.a more than it is true for other libraries? Presumably that already doesn't work for libc++?

pcc added a subscriber: danalbert.Mar 29 2019, 3:57 PM

It applies to libc++ as well. Many Android apps contain shared objects that statically link libc++ and libc++abi, for example.

That said, I don't know how Android builds libc++ for NDK users. Maybe they aren't using CMake? @danalbert would probably know.

Android maintains its own build scripts, so this won't harm us, but I think the problem in general remains. Seems like it would be better for the webasm target to opt out of the behavior than for all targets to do it (or maybe just provide an option so maintainers can choose what they want for their platform).

After this patch, libcxx failed to compile when -DLLVM_ENABLE_PIC=OFF and -DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON.

Maybe we should add an option to set PIC of libcxxabi. Something like

option(LIBCXXABI_ENABLE_PIC "Build Position-Independent Code, which is required if LIBCXX_ENABLE_STATIC_ABI_LIBRARY is set in libcxx" ON)

and add some checks in libcxx CMakeLists.txt to fail in cmake configure step if condition does not meet.

After this patch, libcxx failed to compile when -DLLVM_ENABLE_PIC=OFF and -DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON.

Maybe we should add an option to set PIC of libcxxabi. Something like

option(LIBCXXABI_ENABLE_PIC "Build Position-Independent Code, which is required if LIBCXX_ENABLE_STATIC_ABI_LIBRARY is set in libcxx" ON)

and add some checks in libcxx CMakeLists.txt to fail in cmake configure step if condition does not meet.

That sounds like a reasonable approach: https://reviews.llvm.org/D60049