[libcxx-commits] [PATCH] D79904: [libcxx][libcxxabi] Link against libdl on Android

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added a comment.

In D79904#2035491 <https://reviews.llvm.org/D79904#2035491>, @rprichard wrote:

> Just to clarify, LIBCXX[ABI]_HAS_DL_LIB doesn't just mean "we can link against libdl.so", but more specifically, "do link libc++[abi].so against libdl.so"? i.e. On GNU/Linux, we could link libc++[abi].so against libdl.so, but we don't, because we don't need to, because the unwinder always comes from libgcc_s.so.


Right. I can come up with a better name.

> 
> 
>> libgcc's DWARF unwinder (used for all Android targets except armv7) uses
>>  dl_iterate_phdr, so we need to link against libdl to make the link
>>  succeed. The driver ordinarily injects libdl for you, but we prevent the
>>  driver's default library additions so we have to add it ourselves.
>>  Ideally the libgcc linker script in the NDK would be adjusted to add the
>>  libdl link, but until we have an NDK with that change, work around it in
>>  the libc++ and libc++abi build systems.
> 
> The logic here also applies to arm32 too, I think, where the arm32 libunwind needs dl_unwind_find_exidx from libdl.so (or libc.a). It looks like the NDK's arm32 linker script already includes -ldl, though, so maybe we can add it to other architectures.
> 
> It might do the wrong thing for static executables, though. For Android static executables, libdl.a has dummy implementations of dl* APIs that do nothing and sometimes report failure. dl_iterate_phdr (and dl_unwind_find_exidx), on the other hand, now live in libc.a and do something reasonable for unwinding when there is only one ELF binary. We had a complaint about the platform build system when libdl.a was automatically linked into static executables -- someone had code that called dlopen+dlsym, and they preferred a link-time error rather than a run-time error.
> 
> So, maybe the arm32 libgcc.a script shouldn't link against -ldl, and we should generally rely on the driver inserting -ldl for dynamic exes/libs.
> 
> I don't think we should make either change to libgcc.a, though, given that we want to migrate to compiler-rt + libunwind. e.g. If Android starts defaulting to -rtlib=compiler-rt, then there is no file named libgcc.a, and the driver implicitly links against libclang_rt.builtins.*.a instead. It's not obvious to me then whether the driver would implicitly link against libunwind.a, or whether it would simply omit the unwinder. LLVM's -rtlib=compiler-rt behavior seems to prefer omitting the unwinder.
> 
> e.g. I looked at the Fuchsia LLVM configuration, and it uses a libc++.so linker script that has -lunwind. There is also a libc++.a that subsumes the object files from libc++abi.a and libunwind.a. IIUC, a C program (or C++ program with no STL) isn't automatically linked against an unwinder. A C program that calls _Unwind_Backtrace must explicitly link -lunwind. Maybe the Android NDK would work the same way, maybe the driver would differ (e.g. have Linux::GetUnwindLibType() return ToolChain::UNW_CompilerRT). Or, we could even match FreeBSD 12.1, which seems to use compiler-rt + libunwind, but access them using the libgcc{.a,_eh.a,_s.so} names.

Good points. I would imagine linking against the unwinder explicitly when you use it is okay, though I haven't thought about it too hard.

The current driver logic to add `-ldl` to all non-static Android links is specific to libgcc right now though (it lives in `AddLibgcc`), so it'd need some adjustment either way (unless you want anyone linking against libunwind manually to also link against libdl manually, and assuming a static libunwind, which is what I understood from D78787 <https://reviews.llvm.org/D78787>).

In D79904#2035499 <https://reviews.llvm.org/D79904#2035499>, @rprichard wrote:

> In D79904#2035411 <https://reviews.llvm.org/D79904#2035411>, @smeenai wrote:
>
> > In D79904#2035404 <https://reviews.llvm.org/D79904#2035404>, @compnerd wrote:
> >
> > > Why can we not use `${CMAKE_DL_LIBS}` instead of explicitly detecting libdl and linking against it?
> >
> >
> > Because I wasn't aware of its existence :D Happy to switch to that.
>
>
> It looks like glibc/musl's libc.so exports dl_iterate_phdr, so even libgcc_s.so doesn't depend on libdl.so. Bionic's libc.so mostly doesn't expose the unwinding dl* APIs, but its libc.a does. (exception: arm32 libc.so exports __gnu_Unwind_Find_exidx)
>
> I would guess that using ${CMAKE_DL_LIBS} would link libc++abi.so against libdl.so on GNU/Linux systems, which is unnecessary and would make any C++ program implicitly link against libdl.so?
>
> Given that ${CMAKE_DL_LIBS} is specified by CMake, I'm wondering how well it works with cross-compilation. I'd guess that it ought to work.


Yeah, I'll think about how to make the logic here better (specific to Android, and ideally not breaking static builds).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79904