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

[pcc]

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.

[rprichard]

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.

[peter.smith]

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]

• Address review comments

[smeenai]

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]

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]

Thanks for the extra comments. Looks good to me.

llvm/include/llvm/MC/MCELFStreamer.h

private:
   /// instructions.
   SmallVector<MCDataFragment *, 4> BundleGroups;
 };
 
 MCELFStreamer *createARMELFStreamer(MCContext &Context,
                                     std::unique_ptr<MCAsmBackend> TAB,
                                     std::unique_ptr<MCObjectWriter> OW,
                                     std::unique_ptr<MCCodeEmitter> Emitter,
-                                    bool RelaxAll, bool IsThumb);
+                                    bool RelaxAll, bool IsThumb, bool IsAndroid);
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCELFSTREAMER_H

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

 ///
 /// As a result this system is orthogonal to the DataRegion infrastructure used
 /// by MachO. Beware!
 class ARMELFStreamer : public MCELFStreamer {
 public:
   friend class ARMTargetELFStreamer;
 
   ARMELFStreamer(MCContext &Context, std::unique_ptr<MCAsmBackend> TAB,
-                 std::unique_ptr<MCObjectWriter> OW, std::unique_ptr<MCCodeEmitter> Emitter,
-                 bool IsThumb)
-      : MCELFStreamer(Context, std::move(TAB), std::move(OW), std::move(Emitter)),
-        IsThumb(IsThumb) {
+                 std::unique_ptr<MCObjectWriter> OW,
+                 std::unique_ptr<MCCodeEmitter> Emitter, bool IsThumb,
+                 bool IsAndroid)
+      : MCELFStreamer(Context, std::move(TAB), std::move(OW),
+                      std::move(Emitter)),
+        IsThumb(IsThumb), IsAndroid(IsAndroid) {
     EHReset();
   }
 
   ~ARMELFStreamer() override = default;
 
   void FinishImpl() override;
 
   // ARM exception handling directives
private:
   void SwitchToEHSection(StringRef Prefix, unsigned Type, unsigned Flags,
                          SectionKind Kind, const MCSymbol &Fn);
   void SwitchToExTabSection(const MCSymbol &FnStart);
   void SwitchToExIdxSection(const MCSymbol &FnStart);
 
   void EmitFixup(const MCExpr *Expr, MCFixupKind Kind);
 
   bool IsThumb;
+  bool IsAndroid;
   int64_t MappingSymbolCounter = 0;
 
   DenseMap<const MCSection *, std::unique_ptr<ElfMappingSymbolInfo>>
       LastMappingSymbols;
 
   std::unique_ptr<ElfMappingSymbolInfo> LastEMSInfo;
 
   // ARM Exception Handling Frame Information
void ARMELFStreamer::emitFnEnd() {
 
   // Emit unwind opcodes if there is no .handlerdata directive
   if (!ExTab && !CantUnwind)
     FlushUnwindOpcodes(true);
 
   // Emit the exception index table entry
   SwitchToExIdxSection(*FnStart);
 
-  if (PersonalityIndex < ARM::EHABI::NUM_PERSONALITY_INDEX)
+  // The EHABI requires a dependency preserving R_ARM_NONE relocation to the
+  // personality routine to protect it from an arbitrary platform's static
+  // linker garbage collection. We disable this for Android where the unwinder
+  // is either dynamically linked or directly references the personality
+  // routine.
+  if (PersonalityIndex < ARM::EHABI::NUM_PERSONALITY_INDEX && !IsAndroid)
     EmitPersonalityFixup(GetAEABIUnwindPersonalityName(PersonalityIndex));
 
   const MCSymbolRefExpr *FnStartRef =
     MCSymbolRefExpr::create(FnStart,
                             MCSymbolRefExpr::VK_ARM_PREL31,
                             getContext());
 
   EmitValue(FnStartRef, 4);
if (TT.isOSBinFormatELF())
     return new ARMTargetELFStreamer(S);
   return new ARMTargetStreamer(S);
 }
 
 MCELFStreamer *createARMELFStreamer(MCContext &Context,
                                     std::unique_ptr<MCAsmBackend> TAB,
                                     std::unique_ptr<MCObjectWriter> OW,
                                     std::unique_ptr<MCCodeEmitter> Emitter,
-                                    bool RelaxAll, bool IsThumb) {
-  ARMELFStreamer *S = new ARMELFStreamer(Context, std::move(TAB), std::move(OW),
-                                         std::move(Emitter), IsThumb);
+                                    bool RelaxAll, bool IsThumb,
+                                    bool IsAndroid) {
+  ARMELFStreamer *S =
+      new ARMELFStreamer(Context, std::move(TAB), std::move(OW),
+                         std::move(Emitter), IsThumb, IsAndroid);
   // FIXME: This should eventually end up somewhere else where more
   // intelligent flag decisions can be made. For now we are just maintaining
   // the status quo for ARM and setting EF_ARM_EABI_VER5 as the default.
   S->getAssembler().setELFHeaderEFlags(ELF::EF_ARM_EABI_VER5);
 
   if (RelaxAll)
     S->getAssembler().setRelaxAll(true);
   return S;
 }
 
 } // end namespace llvm

llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

 
 static MCStreamer *createELFStreamer(const Triple &T, MCContext &Ctx,
                                      std::unique_ptr<MCAsmBackend> &&MAB,
                                      std::unique_ptr<MCObjectWriter> &&OW,
                                      std::unique_ptr<MCCodeEmitter> &&Emitter,
                                      bool RelaxAll) {
   return createARMELFStreamer(
       Ctx, std::move(MAB), std::move(OW), std::move(Emitter), false,
-      (T.getArch() == Triple::thumb || T.getArch() == Triple::thumbeb));
+      (T.getArch() == Triple::thumb || T.getArch() == Triple::thumbeb),
+      T.isAndroid());
 }
 
 static MCStreamer *
 createARMMachOStreamer(MCContext &Ctx, std::unique_ptr<MCAsmBackend> &&MAB,
                        std::unique_ptr<MCObjectWriter> &&OW,
                        std::unique_ptr<MCCodeEmitter> &&Emitter, bool RelaxAll,
                        bool DWARFMustBeAtTheEnd) {
   return createMachOStreamer(Ctx, std::move(MAB), std::move(OW),

llvm/test/MC/ARM/eh-compact-pr0.s

 @ RUN: llvm-mc %s -triple=armv7-unknown-linux-gnueabi -filetype=obj -o - \
 @ RUN:   | llvm-readobj -S --sd --sr > %t
 @ RUN: FileCheck %s < %t
-@ RUN: FileCheck --check-prefix=RELOC %s < %t
+@ RUN: FileCheck --check-prefixes=RELOC,RELOC-NOAND %s < %t
+@ RUN: llvm-mc %s -triple=armv7-unknown-linux-androideabi -filetype=obj -o - \
+@ RUN:   | llvm-readobj -S --sd --sr | FileCheck --check-prefix=RELOC %s
 
 @ Check the compact pr0 model
 
   .syntax unified
 
   .section  .TEST1
   .globl  func1
   .align  2

 @ 0x80   = Compact model 0, personality routine: __aeabi_unwind_cpp_pr0
 @ 0x9B   = $sp can be found in $r11
 @ 0x8480 = pop {r11, r14}
 @-------------------------------------------------------------------------------
 @ CHECK:     SectionData (
 @ CHECK:       0000: 00000000 80849B80                    |........|
 @ CHECK:     )
 @ CHECK:   }
 @-------------------------------------------------------------------------------
-@ The first word should be relocated to .TEST1 section.  Besides, there is
-@ another relocation entry for __aeabi_unwind_cpp_pr0, so that the linker
-@ will keep __aeabi_unwind_cpp_pr0.
+@ The first word should be relocated to .TEST1 section.  Besides, on non-Android
+@ there is another relocation entry for __aeabi_unwind_cpp_pr0, so that the
+@ linker will keep __aeabi_unwind_cpp_pr0.
 @-------------------------------------------------------------------------------
-@ RELOC:   Section {
-@ RELOC:     Name: .rel.ARM.exidx.TEST1
-@ RELOC:     Relocations [
-@ RELOC:       0x0 R_ARM_NONE __aeabi_unwind_cpp_pr0 0x0
-@ RELOC:       0x0 R_ARM_PREL31 .TEST1 0x0
-@ RELOC:     ]
-@ RELOC:   }
-
+@ RELOC:        Section {
+@ RELOC:          Name: .rel.ARM.exidx.TEST1
+@ RELOC:          Relocations [
+@ RELOC-NOAND-NEXT: 0x0 R_ARM_NONE __aeabi_unwind_cpp_pr0 0x0
+@ RELOC-NEXT:       0x0 R_ARM_PREL31 .TEST1 0x0
+@ RELOC-NEXT:     ]
 
 @-------------------------------------------------------------------------------
 @ Check .TEST2 section
 @-------------------------------------------------------------------------------
 @ CHECK:   Section {
 @ CHECK:     Name: .TEST2
 @ CHECK:     SectionData (
 @ CHECK:       0000: 00482DE9 0088BDE8                    |.H-.....|

 @ 0x8480 = pop {r11, r14}
 @ 0xB0   = finish
 @-------------------------------------------------------------------------------
 @ CHECK:     SectionData (
 @ CHECK:       0000: 00000000 B0808480                    |........|
 @ CHECK:     )
 @ CHECK:   }
 @-------------------------------------------------------------------------------
-@ The first word should be relocated to .TEST2 section.  Besides, there is
-@ another relocation entry for __aeabi_unwind_cpp_pr0, so that the linker
-@ will keep __aeabi_unwind_cpp_pr0.
+@ The first word should be relocated to .TEST2 section.  Besides, on non-Android
+@ there is another relocation entry for __aeabi_unwind_cpp_pr0, so that the
+@ linker will keep __aeabi_unwind_cpp_pr0.
 @-------------------------------------------------------------------------------
-@ RELOC:   Section {
-@ RELOC:     Name: .rel.ARM.exidx.TEST2
-@ RELOC:     Relocations [
-@ RELOC:       0x0 R_ARM_NONE __aeabi_unwind_cpp_pr0 0x0
-@ RELOC:       0x0 R_ARM_PREL31 .TEST2 0x0
-@ RELOC:     ]
-@ RELOC:   }
+@ RELOC:        Section {
+@ RELOC:          Name: .rel.ARM.exidx.TEST2
+@ RELOC:          Relocations [
+@ RELOC-NOAND-NEXT: 0x0 R_ARM_NONE __aeabi_unwind_cpp_pr0 0x0
+@ RELOC-NEXT:       0x0 R_ARM_PREL31 .TEST2 0x0
+@ RELOC-NEXT:     ]