ELF: Don't reuse a thunk in a different loadable partition.

[pcc]

Summary

There's no guarantee that the other partition will be loaded, so it
can't be reused.

Depends on D60353

[MaskRay]

@pcc There are many partition related revisions. Do you know how to apply them all at once to see the whole picture? I don't know much about arc...

[pcc]

I use this script to download patches from Phab (I think there's an arc patch command, but I've never had it work correctly):

#!/bin/sh

wget -O "/tmp/$1.diff" "https://reviews.llvm.org/$1?download=true"

I use it like this: arcdl D12345. Then I just do patch -p0 < /tmp/D12345.diff in my monorepo source tree. I think git apply should work as well, but I normally just use patch.

[peter.smith]

I think this is the right thing to do. I've left a suggestion to abstract the names a bit as I think that there are some other cases where we don't want to reuse Thunks across OutputSection boundaries.

A theoretical test case

        .section .sec1, "ax", %progbits
        bl far

        .section .sec2, "ax", %progbits
        bl far

        .section .sec3, "ax", %progbits
        .globl far
        .type far, %function
far:    bx lr

SECTIONS {
  .sec1 0x1000 (OVERLAY) : AT (0x1000) { *(.sec1) }
  .sec2 0x1000 (OVERLAY) : AT (0x2000) { *(.sec2) }
  .far  0x10000000 : { *(.sec3) }
}

ld.bfd correctly produces a Thunk in both of .sec1 and .sec2 as .sec1 and .sec2 will not be in memory at the same time. It looks like LLD isn't handling this test case well at the moment. I get relocation out of range errors and .sec1 and .sec2 being given a VMA and LMA of 0. I think that this is because the OVERLAY also has the property of removing SHF_ALLOC from the OutputSection which seems to be having some other side-effects. I'll raise a PR.

lld/ELF/Relocations.cpp

1626 ↗

(On Diff #201119)

There are some other cases where we won't want to reuse across an OutputSection boundary, for example when the OutputSections are overlays. Would it be worth extracting into something like if (CompatibleOutputSections(T->getThunkTargetSym()->Section, IS)) where we could add to these conditions.

[peter.smith]

Raised PR42006 for the OVERLAY behaviour https://bugs.llvm.org/show_bug.cgi?id=42006

[pcc]

Address review comments

lld/ELF/Relocations.cpp

1626 ↗

(On Diff #201119)

Makes sense. While moving this out to a function I took the opportunity to introduce a small optimization: if the thunk is in the main partition, we can always call it because it will always be loaded.

[peter.smith]

Thanks for the update. I've marked as accepted as I think only mechanical changes will be needed if D60353 changes Partition representation.

[ruiu]

LGTM

lld/trunk/ELF/Relocations.h

private:
   ThunkSection *getISDThunkSec(OutputSection *OS, InputSection *IS,
                                InputSectionDescription *ISD, uint32_t Type,
                                uint64_t Src);
 
   ThunkSection *getISThunkSec(InputSection *IS);
 
   void createInitialThunkSections(ArrayRef<OutputSection *> OutputSections);
 
-  std::pair<Thunk *, bool> getThunk(Symbol &Sym, RelType Type, uint64_t Src);
+  std::pair<Thunk *, bool> getThunk(InputSection *IS, Symbol &Sym, RelType Type,
+                                    uint64_t Src);
 
   ThunkSection *addThunkSection(OutputSection *OS, InputSectionDescription *,
                                 uint64_t Off);
 
   bool normalizeExistingThunk(Relocation &Rel, uint64_t Src);
 
   // Record all the available Thunks for a Symbol
   llvm::DenseMap<std::pair<SectionBase *, uint64_t>, std::vector<Thunk *>>

lld/trunk/ELF/Relocations.cpp

forEachInputSectionDescription(
         addThunkSection(OS, ISD, ISLimit);
       });
 }
 
 ThunkSection *ThunkCreator::addThunkSection(OutputSection *OS,
                                             InputSectionDescription *ISD,
                                             uint64_t Off) {
   auto *TS = make<ThunkSection>(OS, Off);
+  TS->Partition = OS->Partition;
   ISD->ThunkSections.push_back({TS, Pass});
   return TS;
 }
 
-std::pair<Thunk *, bool> ThunkCreator::getThunk(Symbol &Sym, RelType Type,
-                                                uint64_t Src) {
+static bool isThunkSectionCompatible(InputSection *Source,
+                                     SectionBase *Target) {
+  // We can't reuse thunks in different loadable partitions because they might
+  // not be loaded. But partition 1 (the main partition) will always be loaded.
+  if (Source->Partition != Target->Partition)
+    return Target->Partition == 1;
+  return true;
+}
+
+std::pair<Thunk *, bool> ThunkCreator::getThunk(InputSection *IS, Symbol &Sym,
+                                                RelType Type, uint64_t Src) {
   std::vector<Thunk *> *ThunkVec = nullptr;
 
   // We use (section, offset) pair to find the thunk position if possible so
   // that we create only one thunk for aliased symbols or ICFed sections.
   if (auto *D = dyn_cast<Defined>(&Sym))
     if (!D->isInPlt() && D->Section)
       ThunkVec = &ThunkedSymbolsBySection[{D->Section->Repl, D->Value}];
   if (!ThunkVec)
     ThunkVec = &ThunkedSymbols[&Sym];
 
   // Check existing Thunks for Sym to see if they can be reused
   for (Thunk *T : *ThunkVec)
-    if (T->isCompatibleWith(Type) &&
+    if (isThunkSectionCompatible(IS, T->getThunkTargetSym()->Section) &&
+        T->isCompatibleWith(Type) &&
         Target->inBranchRange(Type, Src, T->getThunkTargetSym()->getVA()))
       return std::make_pair(T, false);
 
   // No existing compatible Thunk in range, create a new one
   Thunk *T = addThunk(Type, Sym);
   ThunkVec->push_back(T);
   return std::make_pair(T, true);
 }
forEachInputSectionDescription(
               continue;
 
             if (!Target->needsThunk(Rel.Expr, Rel.Type, IS->File, Src,
                                     *Rel.Sym))
               continue;
 
             Thunk *T;
             bool IsNew;
-            std::tie(T, IsNew) = getThunk(*Rel.Sym, Rel.Type, Src);
+            std::tie(T, IsNew) = getThunk(IS, *Rel.Sym, Rel.Type, Src);
 
             if (IsNew) {
               // Find or create a ThunkSection for the new Thunk
               ThunkSection *TS;
               if (auto *TIS = T->getTargetInputSection())
                 TS = getISThunkSec(TIS);
               else
                 TS = getISDThunkSec(OS, IS, ISD, Rel.Type, Src);

lld/trunk/test/ELF/partition-thunk-reuse.s

+// REQUIRES: arm
+// RUN: llvm-mc %s -o %t.o -filetype=obj --triple=armv7-unknown-linux -arm-add-build-attributes
+// RUN: ld.lld %t.o -o %t --export-dynamic --gc-sections
+// RUN: llvm-nm %t | FileCheck %s
+
+// CHECK: __Thumbv7ABSLongThunk__start
+// CHECK: __Thumbv7ABSLongThunk__start
+
+// CHECK: __Thumbv7ABSLongThunk_foo
+// CHECK-NOT: __Thumbv7ABSLongThunk_foo
+
+.thumb
+
+.section .llvm_sympart.g1,"",%llvm_sympart
+.asciz "part1"
+.4byte f1
+
+.section .llvm_sympart.g2,"",%llvm_sympart
+.asciz "part2"
+.4byte f2
+
+.section .text._start,"ax",%progbits
+.globl _start
+_start:
+bx lr
+foo:
+b f0
+.zero 17*1048576
+
+.section .text.f0,"ax",%progbits
+.globl f0
+f0:
+b foo
+
+.section .text.f1,"aw",%progbits
+.globl f1
+f1:
+b _start
+b foo
+
+.section .text.f2,"ax",%progbits
+.globl f2
+f2:
+b _start
+b foo