Page MenuHomePhabricator

[libunwind] Add LIBUNWIND_ENABLE_PIC
AbandonedPublic

Authored by tambre on Apr 3 2020, 12:46 AM.

Details

Reviewers
mstorsjo
phosek
compnerd
Group Reviewers
Restricted Project
Summary

This is allows statically linking libunwind in standalone libc++abi builds. See also D77296.
Default to OFF for now to keep the same behaviour as before.
GN builds already enable PIC unconditionally for shared libraries.

Diff Detail

Event Timeline

tambre created this revision.Apr 3 2020, 12:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 3 2020, 12:46 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

Would two flags LIBUNWIND_ENABLE_SHARED_PIC and LIBUNWIND_ENABLE_STATIC_PIC sound good to you? Then we could default shared to ON, but static to OFF.
Enabling PIC in static builds is something I need.

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

I agree the default should probably be to enable PIC, but FWIW some vendors do build with PIC disabled. For example, we build with PIC disabled because we link everything in a shared cache on iOS. I'm fine if we change the default, I'll just start specifying LIBUNWIND_ENABLE_PIC=OFF. I would just pick whatever is consistent with the other runtime libraries.

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

Would two flags LIBUNWIND_ENABLE_SHARED_PIC and LIBUNWIND_ENABLE_STATIC_PIC sound good to you? Then we could default shared to ON, but static to OFF.
Enabling PIC in static builds is something I need.

Can't you just enable it globally for libunwind and then only use the static library? The creation of several MEOW_SHARED and MEOW_STATIC options makes the build system a lot more complicated than it should be.

@ldionne yeah, thats why I would prefer that we just do the following in the top level CMakeLists.txt for libunwind:

set(CMAKE_POSITION_INDEPENDENT_CODE YES CACHE BOOL "")

The user can then specify this on the commandline to override the default behaviour.

@ldionne yeah, thats why I would prefer that we just do the following in the top level CMakeLists.txt for libunwind:

set(CMAKE_POSITION_INDEPENDENT_CODE YES CACHE BOOL "")

The user can then specify this on the commandline to override the default behaviour.

Setting that globally is definitely the wrong way to go. I would actually advocate for having no setting at all and then setting CMAKE_POSITION_INDEPENDENT_CODE to whatever you want in your own CMake cache, but we should never hardcode something in the CMakeLists.txt.

tambre updated this revision to Diff 255015.Apr 4 2020, 1:43 AM

Changed LIBUNWIND_ENABLE_PIC default to ON

tambre added a comment.Apr 4 2020, 1:52 AM

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

Would two flags LIBUNWIND_ENABLE_SHARED_PIC and LIBUNWIND_ENABLE_STATIC_PIC sound good to you? Then we could default shared to ON, but static to OFF.
Enabling PIC in static builds is something I need.

Can't you just enable it globally for libunwind and then only use the static library? The creation of several MEOW_SHARED and MEOW_STATIC options makes the build system a lot more complicated than it should be.

That suits me. I was rather thinking about some users preferring to choose.

I think that this really was an oversight, and that we should default to PIC builds, at least for shared libraries.

I agree the default should probably be to enable PIC, but FWIW some vendors do build with PIC disabled. For example, we build with PIC disabled because we link everything in a shared cache on iOS. I'm fine if we change the default, I'll just start specifying LIBUNWIND_ENABLE_PIC=OFF. I would just pick whatever is consistent with the other runtime libraries.

I've change the default.

@ldionne yeah, thats why I would prefer that we just do the following in the top level CMakeLists.txt for libunwind:

set(CMAKE_POSITION_INDEPENDENT_CODE YES CACHE BOOL "")

The user can then specify this on the commandline to override the default behaviour.

Setting that globally is definitely the wrong way to go. I would actually advocate for having no setting at all and then setting CMAKE_POSITION_INDEPENDENT_CODE to whatever you want in your own CMake cache, but we should never hardcode something in the CMakeLists.txt.

Setting CMAKE_POSITION_INDEPENDENT_CODE would be fine for me. Setting it to ON as a non-cache variable should mean it only applies to libunwind, thus changing the current default without introducing a new variable. Does that sound good? Should I change to that?

My preference would be to set nothing at all. Just let users say -DCMAKE_POSITION_INDEPENDENT_CODE=ON/OFF in their own caches. Why doesn't that work?

@ldionne - "hard coding" is the right solution. If the user specifies a value on the command line or through a cache file, that will be the value used. What I suggested works as a default value.

Ok, let's go step by step to make sure we don't talk past each other. Here's some facts (feel free to correct me if you see a mistake):

  1. CMake builds shared libraries with POSITION_INDEPENDENT_CODE=ON by default.
  2. CMake builds static libraries with POSITION_INDEPENDENT_CODE=OFF by default.
  3. Setting CMAKE_POSITION_INDEPENDENT_CODE=ON on the command-line or in a cache will cause CMake to build both shared libraries and static libraries with POSITION_INDEPENDENT_CODE=ON.

IIUC, the problem we're trying to solve is that we want to build the static libunwind library with POSITION_INDEPENDENT_CODE=ON. Given that, why do we need a patch at all? Why isn't it sufficient to set CMAKE_POSITION_INDEPENDENT_CODE=ON in the Cache of whoever wants that behaviour?

While I think it would have been a better choice for the CMake designers to use POSITION_INDEPENDENT_CODE=ON by default for static libraries, I don't think it's wise to change that default on a per-project basis. This leads to surprises for anyone that assumes we're using CMake the way it was intended to be used.

I'm fine with passing -DCMAKE_POSITION_INDEPENDENT_CODE=ON on the command line. I didn't think of that before @compnerd mentioned it.
I'll run a toolchain build tomorrow and try it.

compnerd added a comment.EditedApr 8 2020, 11:50 AM

Ok, let's go step by step to make sure we don't talk past each other. Here's some facts (feel free to correct me if you see a mistake):

  1. CMake builds shared libraries with POSITION_INDEPENDENT_CODE=ON by default.
  2. CMake builds static libraries with POSITION_INDEPENDENT_CODE=OFF by default.
  3. Setting CMAKE_POSITION_INDEPENDENT_CODE=ON on the command-line or in a cache will cause CMake to build both shared libraries and static libraries with POSITION_INDEPENDENT_CODE=ON.

@ldionne - I think that the facts you list are correct.

IIUC, the problem we're trying to solve is that we want to build the static libunwind library with POSITION_INDEPENDENT_CODE=ON. Given that, why do we need a patch at all? Why isn't it sufficient to set CMAKE_POSITION_INDEPENDENT_CODE=ON in the Cache of whoever wants that behaviour?

Hmm, then I misunderstood the intent. In my mind, the goal was to improve the default behaviour such that the static library is usable without having to explicitly specify it. The linker should be able to strength reduce the static library relocations, so there is no cost associated (beyond intermediate code size) for doing that.

While I think it would have been a better choice for the CMake designers to use POSITION_INDEPENDENT_CODE=ON by default for static libraries, I don't think it's wise to change that default on a per-project basis. This leads to surprises for anyone that assumes we're using CMake the way it was intended to be used.

Actually, if we want to do that, I think that we should redo the build to remove LIBUNWIND_ENABLE_SHARED/LIBUNWIND_ENABLE_STATIC - they are supposed to be specified by the user as BUILD_SHARED_LIBS=[YES|NO] not as a single build as it is currently. But, that is beyond the scope of this change and more about the philosophical approach that you are suggesting here.

[...]
Actually, if we want to do that, I think that we should redo the build to remove LIBUNWIND_ENABLE_SHARED/LIBUNWIND_ENABLE_STATIC - they are supposed to be specified by the user as BUILD_SHARED_LIBS=[YES|NO] not as a single build as it is currently. But, that is beyond the scope of this change and more about the philosophical approach that you are suggesting here.

I agree with that in principle, and it has been a source of frustration/confusion about how we build things in the runtimes.

So, @tambre, is there anything that makes specifying CMAKE_POSITION_INDEPENDENT_CODE=ON in your cache not a viable option? Would it work? Would it introduce other complexity or weirdness into your build? I'm trying to weigh the options we have here.

tambre abandoned this revision.Apr 10 2020, 1:24 AM

[...]
Actually, if we want to do that, I think that we should redo the build to remove LIBUNWIND_ENABLE_SHARED/LIBUNWIND_ENABLE_STATIC - they are supposed to be specified by the user as BUILD_SHARED_LIBS=[YES|NO] not as a single build as it is currently. But, that is beyond the scope of this change and more about the philosophical approach that you are suggesting here.

I agree with that in principle, and it has been a source of frustration/confusion about how we build things in the runtimes.

So, @tambre, is there anything that makes specifying CMAKE_POSITION_INDEPENDENT_CODE=ON in your cache not a viable option? Would it work? Would it introduce other complexity or weirdness into your build? I'm trying to weigh the options we have here.

I've tested it out now and CMAKE_POSITION_INDEPENDENT_CODE=ON suffices.
Thanks for reviewing and all the info.

It seems this same way should work for libc++abi.
Would it make sense to remove LIBCXXABI_ENABLE_PIC for consistency?

I've tested it out now and CMAKE_POSITION_INDEPENDENT_CODE=ON suffices.
Thanks for reviewing and all the info.

It seems this same way should work for libc++abi.
Would it make sense to remove LIBCXXABI_ENABLE_PIC for consistency?

Yes, I think it would make sense.

However, see the discussion in https://reviews.llvm.org/D60049. I think we should get @phosek and @sbc100 's inputs. Folks, is there a reason why controlling whether libc++abi is built with PIC with CMAKE_POSITION_INDEPENDENT_CODE in the cache is not sufficient?

I've tested it out now and CMAKE_POSITION_INDEPENDENT_CODE=ON suffices.
Thanks for reviewing and all the info.

It seems this same way should work for libc++abi.
Would it make sense to remove LIBCXXABI_ENABLE_PIC for consistency?

Yes, I think it would make sense.

However, see the discussion in https://reviews.llvm.org/D60049. I think we should get @phosek and @sbc100 's inputs. Folks, is there a reason why controlling whether libc++abi is built with PIC with CMAKE_POSITION_INDEPENDENT_CODE in the cache is not sufficient?

The reason this came up for WebAssembly is that our PIC ABI is still under development, almost all current users do static linking and including PIC code in static builds is currently undesirable. In fact -fPIC not currently supported at all with the default wasm target.

So as long as there is some way to build static libraries that don't include PIC code we (WebAssembly) will be happy. If non-PIC static libraries are the default (like they are in cmake) all the better for us.

However, see the discussion in https://reviews.llvm.org/D60049. I think we should get @phosek and @sbc100 's inputs. Folks, is there a reason why controlling whether libc++abi is built with PIC with CMAKE_POSITION_INDEPENDENT_CODE in the cache is not sufficient?

The reason this came up for WebAssembly is that our PIC ABI is still under development, almost all current users do static linking and including PIC code in static builds is currently undesirable. In fact -fPIC not currently supported at all with the default wasm target.

So as long as there is some way to build static libraries that don't include PIC code we (WebAssembly) will be happy. If non-PIC static libraries are the default (like they are in cmake) all the better for us.

Ok, so IIUC we could just remove the LIBCXXABI_ENABLE_PIC setting and that would still work for you? Static libraries would still be built without PIC (as is default in CMake), and if you wanted to make that more explicit (which I would suggest), you could even use CMAKE_POSITION_INDEPENDENT_CODE=OFF.

In light of this, would you (or someone else) be opposed to removing LIBCXXABI_ENABLE_PIC?

However, see the discussion in https://reviews.llvm.org/D60049. I think we should get @phosek and @sbc100 's inputs. Folks, is there a reason why controlling whether libc++abi is built with PIC with CMAKE_POSITION_INDEPENDENT_CODE in the cache is not sufficient?

The reason this came up for WebAssembly is that our PIC ABI is still under development, almost all current users do static linking and including PIC code in static builds is currently undesirable. In fact -fPIC not currently supported at all with the default wasm target.

So as long as there is some way to build static libraries that don't include PIC code we (WebAssembly) will be happy. If non-PIC static libraries are the default (like they are in cmake) all the better for us.

Ok, so IIUC we could just remove the LIBCXXABI_ENABLE_PIC setting and that would still work for you? Static libraries would still be built without PIC (as is default in CMake), and if you wanted to make that more explicit (which I would suggest), you could even use CMAKE_POSITION_INDEPENDENT_CODE=OFF.` setting and that would still work for yo

In light of this, would you (or someone else) be opposed to removing LIBCXXABI_ENABLE_PIC?

I think the problem is that there are other platforms/targets who want/expect the static library to built with PIC, at least for this particular library. I think you might find that you break them if you revert to the cmake defaults here.

In light of this, would you (or someone else) be opposed to removing LIBCXXABI_ENABLE_PIC?

I think the problem is that there are other platforms/targets who want/expect the static library to built with PIC, at least for this particular library. I think you might find that you break them if you revert to the cmake defaults here.

In that case, I think the problem stems from trying to build the runtimes alongside the rest of LLVM, instead of separately as they ought to (either manually or using the runtimes build). I'll put up a patch to remove the option.