Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[llvm-libgcc] initial commit
ClosedPublic

Authored by cjdb on Aug 19 2021, 3:22 PM.

Details

Summary

Note: the term "libgcc" refers to the all of libgcc.a, libgcc_eh.a,
and libgcc_s.so.

Enabling libunwind as a replacement for libgcc on Linux has proven to be
challenging since libgcc_s.so is a required dependency in the [Linux
standard base][5]. Some software is transitively dependent on libgcc
because glibc makes hardcoded calls to functions in libgcc_s. For example,
the function __GI___backtrace eventually makes its way to a [hardcoded
dlopen to libgcc_s' _Unwind_Backtrace][1]. Since libgcc_{eh.a,s.so} and
libunwind have the same ABI, but different implementations, the two
libraries end up [cross-talking, which ultimately results in a
segfault][2].

To solve this problem, libunwind needs to build a “libgcc”. That is, link
the necessary functions from compiler-rt and libunwind into an archive
and shared object that advertise themselves as libgcc.a, libgcc_eh.a,
and libgcc_s.so, so that glibc’s baked calls are diverted to the
correct objects in memory. Fortunately for us, compiler-rt and libunwind
use the same ABI as the libgcc family, so the problem is solvable at the
llvm-project configuration level: no program source needs to be edited.
Thus, the end result is for a user to configure their LLVM build with a
flag that indicates they want to archive compiler-rt/unwind as libgcc.
We achieve this by compiling libunwind with all the symbols necessary
for compiler-rt to emulate the libgcc family, and then generate symlinks
named for our "libgcc" that point to their corresponding libunwind
counterparts.

We alternatively considered patching glibc so that the source doesn't
directly refer to libgcc, but rather _defaults_ to libgcc, so that a
system preferring compiler-rt/libunwind can point to these libraries
at the config stage instead. Even if we modified the Linux standard
base, this alternative won't work because binaries that are built using
libgcc will still end up having crosstalk between the differing
implementations.

This problem has been solved in this manner for [FreeBSD][3], and this
CL has been tested against [Chrome OS][4].

[1]: https://github.com/bminor/glibc/blob/master/sysdeps/arm/backtrace.c#L68
[2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1162190#c16
[3]: https://github.com/freebsd/freebsd-src/tree/main/lib/libgcc_s
[4]: https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2945947
[5]: https://refspecs.linuxbase.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/libgcc-s.html

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cjdb updated this revision to Diff 404168.Jan 28 2022, 2:33 PM

changes how the version script is laid out so that global symbols are all listed first, and exclusive symbols are listed afterward

MaskRay added inline comments.Jan 28 2022, 7:34 PM
llvm/tools/llvm-libgcc/lib/gcc_s.ver
16 ↗(On Diff #404168)

You may use __LP64__

20 ↗(On Diff #404168)

__LP64__

Unsure anyone will use this for ILP64. Sticking with LP64 and !__LP64__ should be fine.

cjdb updated this revision to Diff 404607.Jan 31 2022, 10:39 AM
cjdb marked 2 inline comments as done.

changes how GLOBAL_32BIT and GLOBAL_64BIT are enabled

llvm/tools/llvm-libgcc/lib/gcc_s.ver
20 ↗(On Diff #404168)

Eh, it's not that much extra work to support it, especially if we use #else on the tail of 64-bit detection.

arichardson added inline comments.Jan 31 2022, 3:43 PM
llvm/tools/llvm-libgcc/lib/gcc_s.ver
20 ↗(On Diff #404168)

If you use __SIZEOF_POINTER__ >= 8 it should also do the right thing when compiling for Arm Morello (which is L64P128)

cjdb updated this revision to Diff 404773.Jan 31 2022, 5:37 PM

changes to SIZEOF_POINTER >= 8

cjdb marked an inline comment as done.Jan 31 2022, 5:37 PM
danielkiss added inline comments.Feb 1 2022, 3:02 AM
llvm/tools/llvm-libgcc/lib/gcc_s.ver
4 ↗(On Diff #403735)

__ARM_FP >= 0x04 is better because on some targets the 64bit operations are available.

cjdb updated this revision to Diff 405035.Feb 1 2022, 12:06 PM

broadens armhf detection

cjdb marked an inline comment as done.Feb 1 2022, 12:38 PM
MaskRay added a comment.EditedFeb 1 2022, 3:30 PM

Thanks for the update. From a high level,

  • llvm-libgcc looks useful to me
  • the refactored gcc_s.ver looks good to me.
  • I'd wish that some CMake experts (e.g. @phosek @compnerd) can review the CMake logic, which is a significant portion of this patch. I am hesitant to LGTM this part.
  • the motivation in the summary and the documentation focus on glibc loading libgcc_s.so.1, and people may wonder whether other systems (e.g. Linux musl) need this? If glibc is patched to not dlopen libgcc_so.so.1, is it still a problem? I think the symbol version will remain as a problem for GCC prebuilt objects. The documentation just doesn't make this as a clear motivation.
  • apparently many folks are interested (therefore the comments). Ideally hope someone can stand up to test that the symbol versioning result is indeed closer to libgcc_s.so.1 ... (Sorry, it may not be me.)
tschuett added inline comments.Feb 1 2022, 5:22 PM
llvm/tools/llvm-libgcc/generate_version_script.py
1 ↗(On Diff #405035)

#!/usr/bin/env python3 looks safer.

compnerd requested changes to this revision.Feb 7 2022, 11:07 AM
compnerd added inline comments.
llvm/tools/llvm-libgcc/CMakeLists.txt
7 ↗(On Diff #405035)

Perhaps we can default the path since the layout is now fairly uniform due to the merged repository layout.

36 ↗(On Diff #405035)

I think that bin rather than lib is appropriate. The CMAKE_RUNTIME_OUTPUT_DIRECTORY is used for the executables and dlls on Windows.

llvm/tools/llvm-libgcc/FindCompilerRT.cmake
1 ↗(On Diff #405035)

I think that this is not really the way to do this. This should follow the standard CMake expectations around the behaviour of Find*.cmake. Alternatively, you could name this something else and completely elide the find_* usage.

91 ↗(On Diff #405035)

This should be hoisted into the top level CMake. The FindCompilerRT.cmake should only search for the prebuilt version.

llvm/tools/llvm-libgcc/FindLLVMLibunwind.cmake
1 ↗(On Diff #405035)

Similar

llvm/tools/llvm-libgcc/lib/CMakeLists.txt
17 ↗(On Diff #405035)

add_custom_command by itself is difficult to use, please create a target to track the output so that dependencies can be wired properly.

30 ↗(On Diff #405035)

Is there a reason to not use -nostdlib as a driver level option?

33 ↗(On Diff #405035)

Is there a guarantee that we are not building these libraries? If we are building these libraries, I think that it would be better to use the target objects (i.e. $<TARGET_OBJECTS:unwind> rather than the --whole-archive, --no-whole-archive path.

34 ↗(On Diff #405035)

I think that this should be outside of the -(, -).

38 ↗(On Diff #405035)

This seems like an odd check. I think that we should drop it, libc is required and unless you are suggesting that it is possible to have a freestanding build of this library, we should assume that it is available. (I think that it is still problematic for non-Unix platforms due to spelling but we can deal with that if/when necessary as libgcc is not something that really applies there).

55 ↗(On Diff #405035)

I would rather use ${CMAKE_SHARED_LIBRARY_PREFIX} and ${CMAKE_SHARED_LIBRARY_SUFFIX}. rather than lib and .so.

59 ↗(On Diff #405035)

I think that the versioned symlinks should point to the same names not to the peers.

This revision now requires changes to proceed.Feb 7 2022, 11:07 AM
cjdb updated this revision to Diff 408689.Feb 14 2022, 6:58 PM
cjdb marked 4 inline comments as done.

gets CMake scripts into a decent state

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, Restricted Project, sstefan1. · View Herald Transcript
cjdb updated this revision to Diff 408690.Feb 14 2022, 7:16 PM

quick CMake fix

compnerd added inline comments.Feb 15 2022, 8:10 AM
llvm-libgcc/CMakeLists.txt
11

Any reason for insert rather that append?

37

I tend to prefer all caps for the On and Off, but its equivalent really.

llvm-libgcc/lib/CMakeLists.txt
4

newline before this line would be nice.

9

I tend to prefer the custom command before the custom target so you can see what the target is generating.

18

What differentiates this from the gcc_s_ver target? I think that you have two targets for the same output.

20

I think that breaking after the ( is a bit unnecessary:

set_target_properties(gcc_s_version_script PROPERTIES
  OUTPUT_PATH "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver")

Similar throughout.

55

Dead code?

57

Why is this cached?

compnerd accepted this revision.Feb 15 2022, 8:12 AM

I'd like to see at least https://reviews.llvm.org/D108416#inline-1146215, https://reviews.llvm.org/D108416#inline-1146214, and https://reviews.llvm.org/D108416#inline-1146209 addressed pre-commit. Would be nice to have the other ones resolved as well, but they are more stylistic.

zero9178 added inline comments.
llvm-libgcc/docs/LLVMLibgcc.rst
97

neither compiler-rt, nor llvm-libgcc, are listed in LLVM_ENABLE_RUNTIMES

Is this a typo and is meant to be libunwind instead of llvm-libgcc?

EricWF accepted this revision.Feb 15 2022, 10:35 AM
EricWF added a subscriber: EricWF.

LGTM on behalf of libc++. But please address the comments left by the other reviewers.

This revision is now accepted and ready to land.Feb 15 2022, 10:35 AM
MaskRay added inline comments.Feb 15 2022, 10:35 AM
llvm-libgcc/docs/LLVMLibgcc.rst
98

I think some folks prefer that compiler-rt is added to LLVM_ENABLE_RUNTIMES. How difficult is it to make this work?

Note that current plan is that libunwind in DLLVM_ENABLE_PROJECTS will be unsupported.

llvm-libgcc/generate_version_script.py
2

/usr/bin/env python3

Add a file-level comment what this script does.

MaskRay accepted this revision.Feb 15 2022, 10:36 AM
ldionne added inline comments.
runtimes/CMakeLists.txt
181–200

I think this can be simplified like this:

if("llvm-libgcc" IN_LIST runtimes)
  if("compiler-rt" IN_LIST runtimes OR "compiler-rt" IN_LIST LLVM_ENABLE_PROJECTS)
    message(FATAL_ERROR
      "Attempting to build both compiler-rt and llvm-libgcc will cause irreconcilable "
      "target clashes. Please choose one or the other, but not both.")
  endif()

  if("libunwind" IN_LIST runtimes)
    message(
      FATAL_ERROR
      "Attempting to build both libunwind and llvm-libgcc will cause irreconcilable "
      "target clashes. Please choose one or the other, but not both.")
  endif()
endif()
cjdb updated this revision to Diff 409017.Feb 15 2022, 12:43 PM
cjdb marked 8 inline comments as done.

responds to feedback

llvm-libgcc/CMakeLists.txt
11

I copied this directly from either libunwind/CMakeLists.txt or runtimes/CMakeLists.txt. Looks as if it's being prepended though?

37

Acknowledged.

llvm-libgcc/docs/LLVMLibgcc.rst
97

Thanks!

98

compiler-rt must be listed in neither LLVM_ENABLE_RUNTIMES, nor LLVM_ENABLE_PROJECTS when llvm-libgcc is present. llvm-libgcc will take care of that for you. You can still have LLVM_ENABLE_RUNTIMES=compiler-rt, but not when llvm-libgcc is also in the list.

llvm-libgcc/generate_version_script.py
2

Done, but what's the benefit of this?

llvm-libgcc/lib/CMakeLists.txt
20

I find the current way to improve readability a lot.

57

I think this was user-settable at one point, but per yesterday, this entire variable is redundant.

runtimes/CMakeLists.txt
181–200

Thanks, this is way simpler!

compnerd accepted this revision.Feb 15 2022, 12:49 PM

I believe that the break after ( in CMake is counter the style of the rest of the project. I'd recommend against it.

cjdb updated this revision to Diff 409026.Feb 15 2022, 1:01 PM

cleans up a few things

cjdb updated this revision to Diff 409066.Feb 15 2022, 2:51 PM
  • makes it harder to accidentally build llvm-libgcc by removing it from LLVM_ENABLE_RUNTIMES=all
  • installs shared objects properly
cjdb updated this revision to Diff 409158.Feb 15 2022, 11:04 PM

rebases to activate CI

phosek accepted this revision.Feb 15 2022, 11:59 PM

LGTM

This revision was landed with ongoing or failed builds.Feb 16 2022, 9:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2023, 10:26 AM
Herald added a subscriber: Enna1. · View Herald Transcript