Page MenuHomePhabricator

ARM: Don't emit R_ARM_NONE relocations to compact unwinding decoders in .ARM.exidx on Android.
ClosedPublic

Authored by pcc on Nov 8 2019, 1:35 PM.

Details

Summary

These relocations are specified by the ARM EHABI (section 6.3). As I understand
it, their purpose is to accommodate unwinder implementations that wish to
reduce code size by placing the implementations of the compact unwinding
decoders in a separate translation unit, and using extern weak symbols to
refer to them from the main unwinder implementation, so that they are only
linked when something in the binary needs them in order to unwind.

However, neither of the unwinders used on Android (libgcc, LLVM libunwind)
use this technique, and in fact emitting these relocations ends up being
counterproductive to code size because they cause a copy of the unwinder
to be statically linked into most binaries, regardless of whether it is
actually needed. Furthermore, these relocations create circular dependencies
(between libc and the unwinder) in cases where the unwinder is dynamically
linked and libc contains compact unwind info.

Therefore, deviate from the EHABI here and stop emitting these relocations
on Android.

Diff Detail

Event Timeline

pcc created this revision.Nov 8 2019, 1:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2019, 1:35 PM

The idea seems good to me.

It would also allow removing the __aeabi_unwind_cpp_pr0 hacks from Bionic's libdl/libdl.cpp, libdl/libdl_android.cpp, and linker/ld_android.cpp.

Other than a few suggestions for some comments, this is Android specific so no objections from me.

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
1275–1280

Is it worth adding a comment like:

The EHABI requires a dependency preserving R_ARM_NONE relocation to the personality routine to protect it from an arbitrary platforms static linker garbage collection. We disable this for Android where the unwinder is dynamically linked.
llvm/test/MC/ARM/eh-compact-pr0.s
65–66

Can we update the comment to account for the Android case? there is a similar comment for TEST2.

pcc updated this revision to Diff 228711.Nov 11 2019, 9:21 AM
  • Address review comments
pcc marked 2 inline comments as done.Nov 11 2019, 9:24 AM
smeenai added inline comments.
llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
1275–1280

My understanding is that the unwinder isn't dynamically linked on Android, at least by default ... it's part of libgcc (or libunwind for armv7), both of which are static in the NDK.

pcc marked an inline comment as done.Nov 11 2019, 8:52 PM
pcc added inline comments.
llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
1275–1280

Correct, although this change is motivated by an effort to make it dynamically linked, at least in some circumstances.

peter.smith accepted this revision.Nov 12 2019, 1:15 AM

Thanks for the extra comments. Looks good to me.

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