[libcxx-commits] [PATCH] D77370: [libunwind] Add LIBUNWIND_ENABLE_PIC

[Raul Tambre via Phabricator via libcxx-commits]

tambre added a comment.

In D77370#1960457 <https://reviews.llvm.org/D77370#1960457>, @ldionne wrote:

> In D77370#1960452 <https://reviews.llvm.org/D77370#1960452>, @tambre wrote:
>
> > In D77370#1960387 <https://reviews.llvm.org/D77370#1960387>, @compnerd wrote:
> >
> > > 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.

In D77370#1960454 <https://reviews.llvm.org/D77370#1960454>, @ldionne wrote:

> In D77370#1960387 <https://reviews.llvm.org/D77370#1960387>, @compnerd wrote:
>
> > 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.

In D77370#1960782 <https://reviews.llvm.org/D77370#1960782>, @ldionne wrote:

> In D77370#1960502 <https://reviews.llvm.org/D77370#1960502>, @compnerd wrote:
>
> > @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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77370/new/

https://reviews.llvm.org/D77370