This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][libcxxabi][libunwind] Use libgcc on Android
ClosedPublic

Authored by smeenai on Apr 23 2020, 9:23 PM.

Details

Reviewers
danalbert
enh
ldionne
rprichard
srhines
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGcc259638cb41: [libcxx][libcxxabi][libunwind] Use libgcc on Android
Summary

Android doesn't have a libgcc_s and uses libgcc instead, so adjust the
build accordingly. This matches compiler-rt's build setup. libc++abi and
libunwind were already checking for libgcc but in a different context.
This change makes them search only for libgcc on Android now, but the
code to link against libgcc if it were present was already there.

Diff Detail

Event Timeline

smeenai created this revision.Apr 23 2020, 9:23 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 23 2020, 9:23 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

@srhines to confirm that this won't break our toolchain build, and @rprichard because he's the one looking more closely at libunwind these days.

If they don't have any objections then this is fine with me, though I do wonder if we can just make use of -nostdlib++ or -stdlib=libc++ as needed here (libc++ doesn't use these everywhere because it's a clang only thing, but Android is a clang only platform so we could do that if we have the option).

@srhines to confirm that this won't break our toolchain build, and @rprichard because he's the one looking more closely at libunwind these days.

If they don't have any objections then this is fine with me, though I do wonder if we can just make use of -nostdlib++ or -stdlib=libc++ as needed here (libc++ doesn't use these everywhere because it's a clang only thing, but Android is a clang only platform so we could do that if we have the option).

Not sure I understand. This is the build of libc++ itself, and it's already using -nostdlib++ (cos you don't wanna link libc++ against another C++ library). It still needs to link against the runtime library though, and on Android, that runtime library is gonna be libgcc (or compiler-rt, but that's already supported). I'm fixing the runtime library search to use libgcc instead of libgcc_s on Android, since the latter doesn't exist.

I suppose this change is probably OK. I don't think it will affect the Android organization, because I don't think we're using the CMake files for libc++/libc++abi/libunwind yet. I think we'll start using libunwind's CMake files soon.

Android doesn't ever use libgcc_s. We're trying to remove our use of libgcc and replace it with compiler-rt and libunwind.

It seems that these macros are used to determine which libraries to link into libc++/libc++abi/libunwind, which only matters when the output is a shared library? libc++ is the only one that the Android platform or NDK builds as a shared library. Our libc++[_shared].so always has libc++abi linked statically. For the default platform builds, we currently link against the unwinder from libc.so, but otherwise (for the NDK and for some APEXs), we link an unwinder into libc++[_shared].so statically. (If we move to using libc++[_shared].so from the Android toolchain build, we'll need three different outputs?)

Aside: On my gLinux/Debian system, it appears that libgcc.a doesn't have an unwinder in it. Instead, the unwinder is in libgcc_eh.a or libgcc_s.so.1. libgcc_s.so is a text file that pulls in libgcc_s.so.1 and libgcc.a.

Also, libcxxabi is detecting libgcc by looking for an ARM EABI symbol. Should that be something architecture-independent instead?

check_library_exists(gcc __aeabi_uldivmod "" LIBCXXABI_HAS_GCC_LIB)

I suppose this change is probably OK. I don't think it will affect the Android organization, because I don't think we're using the CMake files for libc++/libc++abi/libunwind yet. I think we'll start using libunwind's CMake files soon.

Note that you can still specify LIBUNWIND_USE_COMPILER_RT to use compiler-rt, as before.

Android doesn't ever use libgcc_s. We're trying to remove our use of libgcc and replace it with compiler-rt and libunwind.

Neat! If you can say, are you planning for a static or shared libunwind? The libunwind and libgcc mixup causes a bunch of issues on 32-bit ARM (https://android.googlesource.com/platform/ndk/+/refs/heads/master/docs/BuildSystemMaintainers.md#Unwinding); do you anticipate similar issues on other architectures?

It seems that these macros are used to determine which libraries to link into libc++/libc++abi/libunwind, which only matters when the output is a shared library? libc++ is the only one that the Android platform or NDK builds as a shared library. Our libc++[_shared].so always has libc++abi linked statically. For the default platform builds, we currently link against the unwinder from libc.so, but otherwise (for the NDK and for some APEXs), we link an unwinder into libc++[_shared].so statically. (If we move to using libc++[_shared].so from the Android toolchain build, we'll need three different outputs?)

Aside: On my gLinux/Debian system, it appears that libgcc.a doesn't have an unwinder in it. Instead, the unwinder is in libgcc_eh.a or libgcc_s.so.1. libgcc_s.so is a text file that pulls in libgcc_s.so.1 and libgcc.a.

Also, libcxxabi is detecting libgcc by looking for an ARM EABI symbol. Should that be something architecture-independent instead?

check_library_exists(gcc __aeabi_uldivmod "" LIBCXXABI_HAS_GCC_LIB)

Yup, this really only matters for libc++, but I figured I'd make all the runtimes consistent. (The sanitizers already have this logic.)

Note that, in this change, I'm adding ANDROID conditionals, so the behavior on other platforms will be unchanged. For the libc++abi detection, I'm assuming the intent of the current logic was to use libgcc in addition to libgcc_s on certain platforms. I'm changing it so that on Android you only look for libgcc, since that's covers everything (and libgcc_s doesn't exist). In the ANDROID case, I'm checking for __gcc_personality_v0 in libgcc, so that's architecture-agnostic. I'm intentionally leaving the existing logic alone for other platforms, because I don't want to modify the behavior for them.

rprichard added a subscriber: pcc.Apr 28 2020, 11:50 PM

Neat! If you can say, are you planning for a static or shared libunwind? The libunwind and libgcc mixup causes a bunch of issues on 32-bit ARM (https://android.googlesource.com/platform/ndk/+/refs/heads/master/docs/BuildSystemMaintainers.md#Unwinding); do you anticipate similar issues on other architectures?

For R+, I think the situation for other architectures will be the same as the current arm32 situation. i.e. The platform libc.so exports _Unwind_* symbols from libgcc.a, but the app uses an incompatible unwinder from LLVM's libunwind.

To reduce the trouble, maybe we can:

  • Only distribute one unwinder implementation with the NDK (e.g. just stop distributing the libgcc unwinder).
  • Mark the libunwind.a symbols hidden. It looks like this is what the libgcc_eh.a / libgcc.a libraries do on my gLinux system. (Some symbols in libunwind.a are already hidden, e.g. __unw_*, but unw_* and _Unwind_* have default visibility.) The NDK's libgcc_real.a _Unwind_* symbols use default visibility instead.

We had been thinking of having a shared libunwind.so in the platform, but decided to export the symbols from libc.so instead. (I don't really remember the rationale. The internal notes are at http://b/144430859#comment24. Maybe it was something about forwarding _Unwind_* APIs from libc.so -> libunwind.so on arm32?)

Maybe the NDK could have a shared libunwind.so, but:

  • I think we'd have trouble ensuring that all call sites consistently used an unwinder compatible with the current unwind context.
    • Even in theory, it'd be hard to prevent libc.so's unwinder from being used, in some situations. (e.g. If there are multiple app shared libraries, and only some of them throw exceptions, call _Unwind_Resume, etc. Maybe some libraries are built with -Wl,--as-needed. The dynamic linker might encounter libc.so before it encounters libunwind.so in a BFS walk of a dependency graph of an app library.)
    • If an app library links against another library exporting an incompatible unwinder, then things break.
  • Presumably libc++_shared.so would have to use the NDK libunwind.so, and I'd guess the extra library would break the Android Gradle plugin? I'd guess the extra library would break other NDK users.

Maybe symbol versioning helps:

  • Tag the public symbols in libunwind.so with a LIBUNWIND version.
  • The _Unwind_* symbols in libc.so are tagged with LIBC_PRIVATE or (as of R) LIBC_R.
  • For Android M and up, a relocation targeting _Unwind_*@@LIBUNWIND should never match an _Unwind_*@@LIBC_* in libc.so.
  • Prior to Android M, the linker didn't implement symbol versioning, but symbol lookup ignores transitive DT_NEEDED dependencies, so as long as libunwind.so appears before libc.so, the linker should find the right symbol.
  • There will be warnings on L-MR1, but that's a known issue and maybe it can't be fixed.

My current plan is to continue linking a non-exported unwinder into every shared library that needs the unwinder. To do this, I'm assuming that it's safe to have duplicate copies of the same version of libunwind operate on the same unwind context. I think that's currently OK for libunwind and mostly OK for libgcc. For libgcc, there was a problem where something called _Unwind_GetGR without first calling uw_init_context to initialize dwarf_reg_size_table. I think(?) that was only a problem with HWASan, maybe? (internal link: http://b/144430859#comment8)

Neat! If you can say, are you planning for a static or shared libunwind? The libunwind and libgcc mixup causes a bunch of issues on 32-bit ARM (https://android.googlesource.com/platform/ndk/+/refs/heads/master/docs/BuildSystemMaintainers.md#Unwinding); do you anticipate similar issues on other architectures?

For R+, I think the situation for other architectures will be the same as the current arm32 situation. i.e. The platform libc.so exports _Unwind_* symbols from libgcc.a, but the app uses an incompatible unwinder from LLVM's libunwind.

Hmm. On an Android 10 device (a Pixel 3), my /system/lib64/libc.so doesn't appear to export any unwind symbols. Both the x86 and x86_64 libc.so on my API 29 emulator don't appear to export any unwind symbols either. Was this a problem on older OS versions?

To reduce the trouble, maybe we can:

  • Only distribute one unwinder implementation with the NDK (e.g. just stop distributing the libgcc unwinder).
  • Mark the libunwind.a symbols hidden. It looks like this is what the libgcc_eh.a / libgcc.a libraries do on my gLinux system. (Some symbols in libunwind.a are already hidden, e.g. __unw_*, but unw_* and _Unwind_* have default visibility.) The NDK's libgcc_real.a _Unwind_* symbols use default visibility instead.

We had been thinking of having a shared libunwind.so in the platform, but decided to export the symbols from libc.so instead. (I don't really remember the rationale. The internal notes are at http://b/144430859#comment24. Maybe it was something about forwarding _Unwind_* APIs from libc.so -> libunwind.so on arm32?)

Makes sense.

Maybe the NDK could have a shared libunwind.so, but:

  • I think we'd have trouble ensuring that all call sites consistently used an unwinder compatible with the current unwind context.
    • Even in theory, it'd be hard to prevent libc.so's unwinder from being used, in some situations. (e.g. If there are multiple app shared libraries, and only some of them throw exceptions, call _Unwind_Resume, etc. Maybe some libraries are built with -Wl,--as-needed. The dynamic linker might encounter libc.so before it encounters libunwind.so in a BFS walk of a dependency graph of an app library.)
    • If an app library links against another library exporting an incompatible unwinder, then things break.
  • Presumably libc++_shared.so would have to use the NDK libunwind.so, and I'd guess the extra library would break the Android Gradle plugin? I'd guess the extra library would break other NDK users.

Maybe symbol versioning helps:

  • Tag the public symbols in libunwind.so with a LIBUNWIND version.
  • The _Unwind_* symbols in libc.so are tagged with LIBC_PRIVATE or (as of R) LIBC_R.
  • For Android M and up, a relocation targeting _Unwind_*@@LIBUNWIND should never match an _Unwind_*@@LIBC_* in libc.so.
  • Prior to Android M, the linker didn't implement symbol versioning, but symbol lookup ignores transitive DT_NEEDED dependencies, so as long as libunwind.so appears before libc.so, the linker should find the right symbol.
  • There will be warnings on L-MR1, but that's a known issue and maybe it can't be fixed.

My current plan is to continue linking a non-exported unwinder into every shared library that needs the unwinder. To do this, I'm assuming that it's safe to have duplicate copies of the same version of libunwind operate on the same unwind context. I think that's currently OK for libunwind and mostly OK for libgcc. For libgcc, there was a problem where something called _Unwind_GetGR without first calling uw_init_context to initialize dwarf_reg_size_table. I think(?) that was only a problem with HWASan, maybe? (internal link: http://b/144430859#comment8)

The other complication is that the C++ personality routine (__gxx_personality_v0) calls back into the unwinder, so you have to ensure the two always get along as well. (That shouldn't be a problem for anyone using libc++ though.)

Symbol versioning would work, but yeah, it's a bunch of complication. On our end, we've successfully used linker symbol wrapping to work around similar issues, though of course that's not workable for the NDK libraries. In an ideal world, ELF would implement two-level namespaces and we wouldn't have any of these issues, but eh.

Also, does this change make sense now?

Hmm. On an Android 10 device (a Pixel 3), my /system/lib64/libc.so doesn't appear to export any unwind symbols. Both the x86 and x86_64 libc.so on my API 29 emulator don't appear to export any unwind symbols either. Was this a problem on older OS versions?

Prior to the upcoming Android R (Android 11) release, only the arm32 libc.so exported unwinder symbols. The arm32 libc.so exported libgcc's unwinder and many of the internal unwinder symbols.

As of R, unwinder symbols are exported from libc.so for all architectures. They're visible in the current R AVD image available through Android Studio. Only the public unwinder API is exported. arm32 now exports LLVM's libunwind, but other architectures export the libgcc unwinder.

Also, does this change make sense now?

Yeah, I'm OK with the change. It definitely makes more sense than linking with gcc_s on Android, which doesn't exist.

(I'm not sure who needs to accept the change.)

Hmm. On an Android 10 device (a Pixel 3), my /system/lib64/libc.so doesn't appear to export any unwind symbols. Both the x86 and x86_64 libc.so on my API 29 emulator don't appear to export any unwind symbols either. Was this a problem on older OS versions?

Prior to the upcoming Android R (Android 11) release, only the arm32 libc.so exported unwinder symbols. The arm32 libc.so exported libgcc's unwinder and many of the internal unwinder symbols.

As of R, unwinder symbols are exported from libc.so for all architectures. They're visible in the current R AVD image available through Android Studio. Only the public unwinder API is exported. arm32 now exports LLVM's libunwind, but other architectures export the libgcc unwinder.

Ah. Is the plan to eventually transition those other architectures to LLVM's libunwind as well (at which point the libc for those architectures would also expose LLVM's unwinder)?

Also, does this change make sense now?

Yeah, I'm OK with the change. It definitely makes more sense than linking with gcc_s on Android, which doesn't exist.

(I'm not sure who needs to accept the change.)

Given that this is Android-specific, I think it'd make sense for an Android person to sign off.

rprichard accepted this revision.Apr 30 2020, 12:44 PM

Ah. Is the plan to eventually transition those other architectures to LLVM's libunwind as well (at which point the libc for those architectures would also expose LLVM's unwinder)?

Yeah, that's the current plan. I'm currently working on switching non-arm32 Android over to libunwind.

srhines accepted this revision.Apr 30 2020, 3:09 PM

I'm also satisfied with this from the Android side. Thank you for discussing this with Ryan.

ldionne accepted this revision as: Restricted Project.Apr 30 2020, 3:19 PM
ldionne accepted this revision as: Restricted Project, Restricted Project.Apr 30 2020, 3:27 PM
This revision is now accepted and ready to land.Apr 30 2020, 3:27 PM
This revision was automatically updated to reflect the committed changes.