[libc++] Always build libc++ and libc++abi with -fPIC

[ldionne]

Summary

Building the libraries with -fPIC ensures that we can link an executable
against the static libraries with -fPIE. Furthermore, there is apparently
basically no downside to building the libraries with position independent
code, since modern toolchains are sufficiently clever.

This commit enforces that we always build the runtime libraries with -fPIC.
This is another take on D104327, which instead makes the decision of whether
to build with -fPIC or not to the build script that drives libc++'s build.

Fixes http://llvm.org/PR43604.

TODO: Add a test.

[mstorsjo]

Furthermore, there is apparently basically no downside to building the libraries with position independent code, since modern toolchains are sufficiently clever.

FWIW it's not necessarily so much about modern toolchains, as it is about modern architectures. AFAIK position independent code is measurably less efficient on e.g. i386, while the overhead on e.g. x86_64 and arm64 is much much smaller (possibly also on 32 bit arm). Therefore I think the other patch (D104327) makes more sense out of these two, as the choice of PIC is more of a choice for how one configures and distributes it, than an intrinsic property of the project itself.

[compnerd]

libcxx/src/CMakeLists.txt

276

Why build the static library as PIC? It is going to be statically linked so it doesn't really need to be position independent. Even if you were to statically link the library into a shared library, it would basically be fully internalized, and the linker should be able to lay it out so that you don't need the PLT/GOT indirection.

libcxxabi/src/CMakeLists.txt

242

Similar

[ldionne]

In D104328#2821342, @mstorsjo wrote:

Furthermore, there is apparently basically no downside to building the libraries with position independent code, since modern toolchains are sufficiently clever.

FWIW it's not necessarily so much about modern toolchains, as it is about modern architectures. AFAIK position independent code is measurably less efficient on e.g. i386, while the overhead on e.g. x86_64 and arm64 is much much smaller (possibly also on 32 bit arm). Therefore I think the other patch (D104327) makes more sense out of these two, as the choice of PIC is more of a choice for how one configures and distributes it, than an intrinsic property of the project itself.

I also think the other patch makes more sense from a layering perspective.

Quoting @chandlerc from https://bugs.llvm.org/show_bug.cgi?id=43604:

IMO, there is a relevant layering distinction here, but I'd put it in a slightly different place.

The difference I would draw is between the build of libraries directly part of LLVM and Clang, and the *runtime* libraries that are used automatically by clang invocations of various forms.

Agreed, those are entirely different things and it makes sense to treat each group consistently (so apply the same treatment to all runtimes, whatever that is).

For runtime libraries, I think always using PIC is reasonable.

I personally don't care. Does @mstorsjo's comment change your mind?

But for non-runtime LLVM and Clang libraries, I don't think it's nearly as reasonable.

Unfortunately, neither patch is exactly this runtimes vs. non-runtimes split...
#1 (D104327) - covers all the runtimes but also non-runtimes...

If we were using the Runtimes build instead to produce LLVM releases, it might be possible to pass -DCMAKE_POSITION_INDEPENDENT_CODE=ON only to the build of the runtimes? @phosek

#2 (D104328) - doesn't impose on non-runtimes, but misses compiler-rt and libunwind.

If that's the way we want to go, that should be easy to fix. I can make the same change in libunwind, and I don't know about compiler-rt but I assume it's not difficult to do it either.

libcxx/src/CMakeLists.txt

276

This is precisely what the bug report here is asking for: http://llvm.org/PR43604

In fact, this patch builds the shared library with fPIC mostly for consistency with the static library. I think it would be useful to have a discussion with the reporter of that bug, and I also know @chandlerc cares about this use case.

Basically, I'm willing to apply whatever fix is going to make people happy, but I'm not sure what's the correct fix here - I'm a bit out of my depth.

[ldionne]

Rebase onto main.

If nobody objects to this approach, I'd like to land it in time for
the LLVM 13 branch. As I said before, I don't mind whether we go
with this solution or D104327, but I care that we fix it.

If nobody has a strong opinion, I'll go with the fix in this patch
because I understand it better -- I'm more familiar with libc++ than
with the release scripts.

[chandlerc]

FWIW, I think this patch makes sense to me.

Even if we start building releases with PIC, I think the static *runtimes* would still benefit from being explicitly PIC. Some users may reasonably disable PIC for the overall build but for runtime libraries specifically it seems desirable for them to support the static archive being linked into a DSO where PIC is required.

libcxx/src/CMakeLists.txt

276

Just to be clear -- I don't know how to build a .a file w/o PIC code and link it into a .so that is PIC... Even though it wouldn't need PLT/GOT indirection, the basic references would need to be IP/PC relative to support the outer .so being PIC... At least, as far as I know.

As long as we want runtime .as to be linked into user .sos, forcing PIC seems like the only realistic path.

[MaskRay]

Some obscure architectures don't support PIC, but none is supported by libunwind/libc++/libc++abi.

Is the change for -DCMAKE_POSITION_INDEPENDENT_CODE=off -DLLVM_ENABLE_PIC=off builds?
Note that LLVM_ENABLE_PIC is on by default => it adds -fPIC despite -DPOSITION_INDEPENDENT_CODE=off ...

So we need information from users why their LLVM_ENABLE_PIC is somehow off.
Do they explicitly set it to off?

If both CMAKE_POSITION_INDEPENDENT_CODE and LLVM_ENABLE_PIC are set to off, do we expect that libLLVM*.a can be linked into a shared object?

[ldionne]

In D104328#2905866, @MaskRay wrote:

Some obscure architectures don't support PIC, but none is supported by libunwind/libc++/libc++abi.

Is the change for -DCMAKE_POSITION_INDEPENDENT_CODE=off -DLLVM_ENABLE_PIC=off builds?
Note that LLVM_ENABLE_PIC is on by default => it adds -fPIC despite -DPOSITION_INDEPENDENT_CODE=off ...

So we need information from users why their LLVM_ENABLE_PIC is somehow off.
Do they explicitly set it to off?

If both CMAKE_POSITION_INDEPENDENT_CODE and LLVM_ENABLE_PIC are set to off, do we expect that libLLVM*.a can be linked into a shared object?

This change is exclusively for the runtimes, i.e. libunwind, libc++abi and libc++. It doesn't have any impact on libLLVM*.a & friends.

[miyuki]

In D104328#2821342, @mstorsjo wrote:

Furthermore, there is apparently basically no downside to building the libraries with position independent code, since modern toolchains are sufficiently clever.

FWIW it's not necessarily so much about modern toolchains, as it is about modern architectures. AFAIK position independent code is measurably less efficient on e.g. i386, while the overhead on e.g. x86_64 and arm64 is much much smaller (possibly also on 32 bit arm). Therefore I think the other patch (D104327) makes more sense out of these two, as the choice of PIC is more of a choice for how one configures and distributes it, than an intrinsic property of the project itself.

I agree. Using -fPIC does not make much sense for embedded targets where everything is linked statically: -fPIC will add overhead, with no particular benefits.

@ldionne, could you please make this feature configurable? Not necessarily using CMAKE_POSITION_INDEPENDENT_CODE like in D104327, but perhaps by a new CMake variable (e.g. LLVM_RUNTIMES_ENABLE_PIC).

[JamesNagurne]

Hi all,

This change has broken my team's downstream validations which utilize the ARM backend.

Specifically, our linker has a method to choose the most correct library to link against, given a sort of binary 'specification' pseudo-library that maps the qualities of a compilation to a particular real library.
One of these qualities is PIC.

Our runtimes build cache file already had to set the following variables to OFF to avoid building runtimes (specifically, using llvm/runtimes to build with just-built compiler) with PIC:

• CMAKE_POSITION_INDEPENDENT_CODE
• LLVM_ENABLE_PIC
• LIBCXXABI_ENABLE_PIC (Deleted recently)
• COMPILER_RT_BUILTINS_ENABLE_PIC

I do also vaguely recall having to make a downstream change to a configuration file so that -fPIC wasn't unconditionally added to some compilation commands

I note that @MaskRay asks why a toolchain would turn PIC support off for runtimes. For our team, the decision is historical (we are moving away from a proprietary backend and library), and may certainly need to be reviewed. However, I'm slightly frustrated by the number of things our team has to do to avoid something that, in my eyes, seems to be amenable to configuration customization.

[MaskRay]

I think there is some configuration confusion here. I think:

• Normally libc++.a should have -fPIC object files. This allows shared object links.
• LLVM_ENABLE_PIC=on (default) adds -fPIC to all object file compilation, including libc++ object files.
• POSITION_INDEPENDENT_CODE in libc++/libc++/libunwind targets just duplicate LLVM_ENABLE_PIC=on's -fPIC (I have checked build.ninja)
• When LLVM_ENABLE_PIC=off, shared object targets (e.g. cxx_shared, libLLVM-13git.so) do not make sense.
• Some ancient architectures don't support PIC (that's not a concern for libc++/libc++/libunwind because no such supported architecture exists). However, some users don't want forced -fPIC in runtime if they only build executables.

I think the original report of https://bugs.llvm.org/show_bug.cgi?id=43604 is due to they setting LLVM_ENABLE_PIC to off.
So I ask when LLVM_ENABLE_PIC=off, do we expect that libc++.a can be used to build shared objects.
I think no.
It probably loses orthogonality (LLVM_ENABLE_PIC controls both libLLVM*.a and libc++.a's shared object compatibility) but that is the status quo.

The distribution problems should be fixed by removing LLVM_ENABLE_PIC=off from their build commands.
That is not a llvm-project problem. We don't need any additional POSITION_INDEPENDENT_CODE.

On https://bugs.llvm.org/show_bug.cgi?id=43604 , I left a comment that LLVM_ENABLE_PIC and CMAKE_POSITION_INDEPENDENT_CODE both control the same property.
We could consider simplifying CMake by preferring CMAKE_POSITION_INDEPENDENT_CODE and letting LLVM_ENABLE_PIC provide a default value for CMAKE_POSITION_INDEPENDENT_CODE.
Using CMAKE_POSITION_INDEPENDENT_CODE should address the orthogonality concern.

[ldionne]

I get way too many emails :-(

I stumbled upon this discussion while making sure I hadn't forgotten anything, and I opened https://reviews.llvm.org/D110261 as a way forward. Please chime in. I don't have a horse in this race, I just want people to be happy with whatever outcome we choose.

libcxx/src/CMakeLists.txt

if (LIBCXX_ENABLE_SHARED)
   set_target_properties(cxx_shared
     PROPERTIES
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "c++"
       VERSION       "${LIBCXX_ABI_VERSION}.0"
       SOVERSION     "${LIBCXX_ABI_VERSION}"
       DEFINE_SYMBOL ""
+      POSITION_INDEPENDENT_CODE ON
   )
   cxx_add_common_build_flags(cxx_shared)
   cxx_set_common_defines(cxx_shared)
 
   # Link against LLVM libunwind
   if (LIBCXXABI_USE_LLVM_UNWINDER)
     if (NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
       target_link_libraries(cxx_shared PUBLIC unwind_shared)
if (LIBCXX_ENABLE_STATIC)
   add_library(cxx_static STATIC ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_link_libraries(cxx_static PUBLIC cxx-headers
                                    PRIVATE ${LIBCXX_LIBRARIES})
   set_target_properties(cxx_static
     PROPERTIES
       COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
       LINK_FLAGS    "${LIBCXX_LINK_FLAGS}"
       OUTPUT_NAME   "c++"
+      POSITION_INDEPENDENT_CODE ON
   )
   cxx_add_common_build_flags(cxx_static)
   cxx_set_common_defines(cxx_static)
 
   if (LIBCXX_HERMETIC_STATIC_LIBRARY)
     # If the hermetic library doesn't define the operator new/delete functions
     # then its code shouldn't declare them with hidden visibility.  They might
     # actually be provided by a shared library at link time.

libcxxabi/src/CMakeLists.txt

PROPERTIES
       CXX_STANDARD 20
       CXX_STANDARD_REQUIRED OFF
       COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}"
       LINK_FLAGS "${LIBCXXABI_LINK_FLAGS}"
       OUTPUT_NAME "c++abi"
       SOVERSION "1"
       VERSION "${LIBCXXABI_LIBRARY_VERSION}"
       DEFINE_SYMBOL ""
+      POSITION_INDEPENDENT_CODE ON
   )
 
   list(APPEND LIBCXXABI_BUILD_TARGETS "cxxabi_shared")
   if (LIBCXXABI_INSTALL_SHARED_LIBRARY)
     list(APPEND LIBCXXABI_INSTALL_TARGETS "cxxabi_shared")
   endif()
 
   add_library(cxxabi-reexports INTERFACE)
if (LIBCXXABI_ENABLE_STATIC)
   add_library(cxxabi_static STATIC ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS})
   target_link_libraries(cxxabi_static PRIVATE cxx-headers ${LIBCXXABI_STATIC_LIBRARIES} ${LIBCXXABI_LIBRARIES})
   if (TARGET pstl::ParallelSTL)
     target_link_libraries(cxxabi_static PUBLIC pstl::ParallelSTL)
   endif()
   set_target_properties(cxxabi_static
     PROPERTIES
       CXX_EXTENSIONS OFF
       CXX_STANDARD 20
       CXX_STANDARD_REQUIRED OFF
       COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}"
       LINK_FLAGS "${LIBCXXABI_LINK_FLAGS}"
       OUTPUT_NAME "c++abi"
+      POSITION_INDEPENDENT_CODE ON
     )
 
   if(LIBCXXABI_HERMETIC_STATIC_LIBRARY)
     append_flags_if_supported(CXXABI_STATIC_LIBRARY_FLAGS -fvisibility=hidden)
     # If the hermetic library doesn't define the operator new/delete functions
     # then its code shouldn't declare them with hidden visibility.  They might
     # actually be provided by a shared library at link time.
     if (LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS)

libunwind/src/CMakeLists.txt

PROPERTIES
       CXX_STANDARD 11
       CXX_STANDARD_REQUIRED ON
       COMPILE_FLAGS "${LIBUNWIND_COMPILE_FLAGS}"
       LINK_FLAGS "${LIBUNWIND_LINK_FLAGS}"
       LINKER_LANGUAGE C
       OUTPUT_NAME "unwind"
       VERSION "1.0"
       SOVERSION "1"
+      POSITION_INDEPENDENT_CODE ON
   )
   list(APPEND LIBUNWIND_BUILD_TARGETS "unwind_shared")
   if (LIBUNWIND_INSTALL_SHARED_LIBRARY)
     list(APPEND LIBUNWIND_INSTALL_TARGETS "unwind_shared")
   endif()
 endif()
 
 # Build the static library.
set_target_properties(unwind_static
     PROPERTIES
       CXX_EXTENSIONS OFF
       CXX_STANDARD 11
       CXX_STANDARD_REQUIRED ON
       COMPILE_FLAGS "${LIBUNWIND_COMPILE_FLAGS}"
       LINK_FLAGS "${LIBUNWIND_LINK_FLAGS}"
       LINKER_LANGUAGE C
       OUTPUT_NAME "unwind"
+      POSITION_INDEPENDENT_CODE ON
   )
 
   if(LIBUNWIND_HIDE_SYMBOLS)
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility=hidden)
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility-global-new-delete-hidden)
     target_compile_options(unwind_static PRIVATE ${UNWIND_STATIC_LIBRARY_FLAGS})
     target_compile_definitions(unwind_static PRIVATE _LIBUNWIND_HIDE_SYMBOLS)
   endif()