This is an archive of the discontinued LLVM Phabricator instance.

[runtimes] Use LLVM libunwind from libc++abi and compiler-rt by default
ClosedPublic

Authored by ldionne on May 18 2023, 11:53 AM.

Details

Reviewers
MaskRay
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG8f90e6937a1f: [runtimes] Use LLVM libunwind from libc++abi by default (#77687)
Summary

I recently came across LIBCXXABI_USE_LLVM_UNWINDER and was surprised
to notice it was disabled by default. Since we build libunwind by
default and ship it in the LLVM toolchain, it would seem to make
sense that libc++ and libc++abi rely on libunwind for unwinding
instead of using the system-provided unwinding library (if any).

Diff Detail

Event Timeline

ldionne created this revision.May 18 2023, 11:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 11:53 AM
Herald added a subscriber: arichardson. · View Herald Transcript
ldionne requested review of this revision.May 18 2023, 11:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 18 2023, 11:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a reviewer: Restricted Project.May 18 2023, 11:54 AM
ldionne added subscribers: sylvestre.ledru, Restricted Project.

Adding libunwind and vendors. I am creating this patch but I expect that someone might be able to explain why we've always done it differently. Also adding @sylvestre.ledru since he handles some LLVM packages and I suspect he might know some context around this.

dim added a subscriber: dim.May 18 2023, 12:31 PM

As long as the setting "OFF" is also tested somehow, so it does not bitrot, this seems pretty much OK to me?

Dunno the why but i have been shipping with LIBCXXABI_USE_LLVM_UNWINDER=ON for a while now. It works great :)

MaskRay accepted this revision.May 18 2023, 1:08 PM
MaskRay added a subscriber: MaskRay.

LGTM from #libunwind side.

On Linux, both libgcc_s.so.1/libgcc_eh.a and libunwind.so/libunwind.a are popular. Considering also BSD, I think the majority of libc++ users have switched to using llvm-project/libunwind primarily, so this default switch makes a lot of sense.

The "Level 1: Base ABI of Itanium C++ ABI: Exception Handling" part of libunwind is very stable, so I don't worry too much that doing this change will degrade libgcc_s.so.1/libgcc_eh.a compatibility in the long run.
Having a LIBCXXABI_USE_LLVM_UNWINDER=ON build bot will definitely be great, but I am fine we don't have it due to resource limitation/etc.

GCC's libgcc_s/libgcc and llvm's libunwind are not ABI compatible, the code will compile/link but will crash at runtime for calls to functions like backtrace() because functions from libunwind and libgcc will get intermingled. So this is a risky change for the users. I'd strongly advise to make a note of this is release notes.

dim added a subscriber: emaste.May 18 2023, 1:32 PM

Considering also BSD, I think the majority of libc++ users have switched to using llvm-project/libunwind primarily, so this default switch makes a lot of sense.

On FreeBSD at least, we have been using LLVM libunwind since quite a while (@emaste might remember when we introduced it), but due to historical reasons we don't use libc++abi but libcxxrt as the ABI library. So this is bit of a strange situation, and why I am interested in keeping the "use the system libunwind case" still tested somehow.

So I would find it nice if the "With LLVM's libunwind" job that is going to be removed, would be replaced by another one called ""Without LLVM's libunwind" :)

Thanks everyone for chiming in, this is exactly what I was looking for!

GCC's libgcc_s/libgcc and llvm's libunwind are not ABI compatible, the code will compile/link but will crash at runtime for calls to functions like backtrace() because functions from libunwind and libgcc will get intermingled. So this is a risky change for the users. I'd strongly advise to make a note of this is release notes.

Do you mean that on platforms where system libraries are using libgcc_s as the unwinder by default, trying to then use libc++ (built against LLVM's libunwind) e.g. from a LLVM release would create these ABI issues? So essentially, a vendor shipping the LLVM toolchain on such a platform would want to *not* ship libunwind (and make sure that libc++ uses the system provided unwinder) for the sake of interoperability with the base system?

I think that makes sense, and indeed that would then merit a release note for sure.

What I would then suggest is:

  1. We make this change with a release note
  2. We introduce a new CI job called "with the system unwinder"
  3. In a separate change, I could refactor the way we select the unwinder to match what we do for libc++abi, i.e. we would have LIBCXXABI_UNWINDER_LIBRARY="libunwind|gcc|whatever"

That way, we'd ship a LLVM toolchain that uses its own stuff by default, but a vendor would still be free to select a different unwinder library. That would make the situation with the unwinder library choice consistent with how we handle picking the ABI library.

The plan sgtm.

GCC's libgcc_s/libgcc and llvm's libunwind are not ABI compatible, the code will compile/link but will crash at runtime for calls to functions like backtrace() because functions from libunwind and libgcc will get intermingled. So this is a risky change for the users. I'd strongly advise to make a note of this is release notes.

The _Unwind_* functions are ABI incompatible, but _Unwind_Context structs are not.

You probably mean glibc's arm port (https://bugs.chromium.org/p/chromium/issues/detail?id=1162190#c16). I have a write-up about when glibc invokes dlopen on libgcc_s.so.1 (pthread_exit/pthread_cancel, and backtrace-family functions):
https://gist.github.com/MaskRay/3d05f5613a2f8e774aad26ee3da4e696

For pthread_exit/pthread_cancel, dlopening libgcc_s.so.1 is undesired but benign. For backtrace, backtrace_symbols, and backtrace_symbols_fd declared in <execinfo.h>, I agree that they are problematic on arm and need attention on other ports (but I don't find another issue yet).

backtrace-family functions are not recommended, so the cross-walking problem should be limited. That said, I want to check whether this can be improved on glibc'c side.

phosek added a subscriber: phosek.May 19 2023, 12:13 AM

We've been using this configuration in our toolchain for years, both for Fuchsia and for Linux, without any issues. I share @manojgupta's concerns though regarding compatibility with system libraries.

Relatedly, I think we should also consider changing the defaults for COMPILER_RT_USE_LLVM_UNWINDER and potentially also LIBCXXABI_USE_COMPILER_RT, LIBCXX_USE_COMPILER_RT, COMPILER_RT_USE_BUILTINS_LIBRARY.

You probably mean glibc's arm port (https://bugs.chromium.org/p/chromium/issues/detail?id=1162190#c16

Yes, we had discovered real issues with libgcc/libunwind intertwining so this is not just hypothetical and this is why llvm-libgcc project now exists.

zibi added a subscriber: zibi.May 19 2023, 10:44 AM

On z/OS, we do build it with LIBCXXABI_USE_LLVM_UNWINDER=OFF so we will have to turn this off explicitly in cache.

ldionne updated this revision to Diff 533251.EditedJun 21 2023, 7:41 AM
ldionne retitled this revision from [runtimes] Use libunwind from libc++ and libc++abi by default to [runtimes] Use LLVM libunwind from libc++abi and compiler-rt by default.

Add a release note and also use LLVM libunwind by default when building compiler-rt.

Most vendors use a custom CMake cache that sets these settings appropriately. However, I don't think we use any CMake cache when we build the "upstream" LLVM releases. I wonder what this means for the upstream LLVM release, and in fact I wonder what we expect folks to do at all with the runtimes shipped alongside the LLVM release.

To expand on that: if our intent is that folks can use upstream LLVM releases to build a toolchain and have e.g. their .sos interoperate with the rest of the system out of the box, I don't think that works if we even ship libc++/libc++abi. In a sense, the upstream LLVM toolchain we ship should always use the system-provided runtimes (Linux: libstdc++ libgcc_s etc, Apple: libc++ libc++abi libunwind etc, FreeBSD: libc++ libcxxrt etc). On the other hand, if the goal of the "upstream LLVM release" is to ship a full toolchain that is consistent with itself, then I think this change gets us closer to that.

Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2023, 7:41 AM
Herald added subscribers: Restricted Project, Enna1, dberris. · View Herald Transcript
ldionne updated this revision to Diff 557169.Sep 21 2023, 4:11 AM

Rebase and try to fix CI.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jan 15, 1:30 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.