This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][Android] Improve workaround for PIE zero-dlpi_addr bug
ClosedPublic

Authored by rprichard on Oct 15 2019, 12:31 AM.

Details

Summary

The workaround added in https://reviews.llvm.org/rL299575 appears to be
working around a bug in Android JB 4.1.x and 4.2.x (API 16 and 17).

Starting in API 16, Android added support for PIE binaries, but the
dynamic linker failed to initialize dlpi_addr to the address that the
executable was loaded at. The bug was fixed in Android JB 4.3.x (API 18).

Improve the true load bias calculation:

  • The code was assuming that the first segment would be the PT_PHDR segment. I think it's better to be explicit and search for PT_PHDR. (It will be almost as fast in practice.)
  • It's more correct to use p_vaddr rather than p_offset. If a PIE executable is linked with a non-zero image base (e.g. lld's -Wl,--image-base=xxxx), then we must use p_vaddr here.

The "phdr->p_vaddr < image_base" condition seems unnecessary and maybe
slightly wrong. If the OS were to load a binary at an address smaller than
a vaddr in the binary, we would still want to do this workaround.

The workaround is safe when the linker bug isn't present, because it
should calculate an image_base equal to dlpi_addr. Note that with API 21
and up, this workaround should never activate for dynamically-linked
objects, because non-PIE executables aren't allowed.

Consolidate the fix into a single block of code that calculates the true
image base, and make it clear that the fix no longer applies after API 18.

See https://github.com/android/ndk/issues/505 for details.

Diff Detail

Event Timeline

rprichard created this revision.Oct 15 2019, 12:31 AM

Exclude an unrelated change from the base revision.

FWIW, Android's official toolchain currently only uses LLVM's libunwind for arm32, so this patch doesn't affect anything yet in the Android platform or NDK. I tested it with NDK r20 by:

  • building libunwind.a with -DLIBUNWIND_TARGET_TRIPLE=i686-linux-android16, then
  • building a PIE executable, linking libunwind.a in front of the libgcc.a unwinder that NDK r20 normally uses.

I verified that libunwind.a was broken on an API16 x86-32 emulator when I used a non-zero -Wl,--image-base=nnnn setting, but this patch fixes it.

(I also tried running the libunwind tests but didn't get them to run.)

compnerd accepted this revision.Oct 15 2019, 10:15 AM

Thanks, this should be more robust. Nice catch with the image base being explicitly set and using p_vaddr rather than p_offset, that was something that I failed to account for. I really wish that we had a more complete test suite, but that is *way* beyond this change.

This revision is now accepted and ready to land.Oct 15 2019, 10:15 AM
This revision was automatically updated to reflect the committed changes.

(I also tried running the libunwind tests but didn't get them to run.)

FWIW, I also built libunwind for the host and was able to run ninja check-unwind successfully.