ELF: Use preprocessed relocations for EhInputSection scanning

[MaskRay]

.eh_frame sections require special sub-section processing, specifically,
CIEs are de-duplicated and FDEs are garbage collected. Create a
specialized scanEhSection() function utilizing the just-added
EhInputSection::rels. OffsetGetter is moved to scanEhSection.

This improves separation of concerns between InputSection and
EhInputSection processing.

This removes another relsOrRelas call using supportsCrel=false.
DWARF.cpp now has the last call.

[llvmbot]

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

.eh_frame sections require special sub-section processing, specifically,
CIEs are de-duplicated and FDEs are garbage collected. Create a
specialized scanEhSection() function utilizing the just-added
EhInputSection::rels. OffsetGetter is moved to scanEhSection.

This improves separation of concerns between InputSection and
EhInputSection processing.

This removes another relsOrRelas call using supportsCrel=false.
DWARF.cpp now has the last call.

---

Full diff: https://github.com/llvm/llvm-project/pull/161091.diff

2 Files Affected:

• (modified) lld/ELF/InputSection.cpp (+2)
• (modified) lld/ELF/Relocations.cpp (+44-25)

diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index f933a36aa4768..c62a998e07597 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -1357,6 +1357,8 @@ SyntheticSection *EhInputSection::getParent() const {
 
 // .eh_frame is a sequence of CIE or FDE records.
 // This function splits an input section into records and returns them.
+// In rare cases (.eh_frame pieces are reordered by a linker script), the
+// relocations may be unordered.
 template <class ELFT> void EhInputSection::split() {
   const RelsOrRelas<ELFT> elfRels = relsOrRelas<ELFT>();
   if (elfRels.areRelocsCrel())
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index bd96c051d160d..95da604519896 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -405,22 +405,17 @@ namespace {
 class OffsetGetter {
 public:
   OffsetGetter() = default;
-  explicit OffsetGetter(InputSectionBase &sec) {
-    if (auto *eh = dyn_cast<EhInputSection>(&sec)) {
-      cies = eh->cies;
-      fdes = eh->fdes;
-      i = cies.begin();
-      j = fdes.begin();
-    }
+  explicit OffsetGetter(EhInputSection &sec) {
+    cies = sec.cies;
+    fdes = sec.fdes;
+    i = cies.begin();
+    j = fdes.begin();
   }
 
   // Translates offsets in input sections to offsets in output sections.
   // Given offset must increase monotonically. We assume that Piece is
   // sorted by inputOff.
   uint64_t get(Ctx &ctx, uint64_t off) {
-    if (cies.empty())
-      return off;
-
     while (j != fdes.end() && j->inputOff <= off)
       ++j;
     auto it = j;
@@ -450,13 +445,12 @@ class OffsetGetter {
 class RelocationScanner {
 public:
   RelocationScanner(Ctx &ctx) : ctx(ctx) {}
-  template <class ELFT>
-  void scanSection(InputSectionBase &s, bool isEH = false);
+  template <class ELFT> void scanSection(InputSectionBase &s);
+  template <class ELFT> void scanEhSection(EhInputSection &s);
 
 private:
   Ctx &ctx;
   InputSectionBase *sec;
-  OffsetGetter getter;
 
   // End of relocations, used by Mips/PPC64.
   const void *end = nullptr;
@@ -473,6 +467,9 @@ class RelocationScanner {
 
   template <class ELFT, class RelTy>
   void scanOne(typename Relocs<RelTy>::const_iterator &i);
+  template <class ELFT, class RelIt>
+  void scanOneAux(RelIt &i, RelExpr expr, RelType type, uint64_t offset,
+                  Symbol &sym, int64_t addend);
   template <class ELFT, class RelTy> void scan(Relocs<RelTy> rels);
 };
 } // namespace
@@ -1511,9 +1508,7 @@ void RelocationScanner::scanOne(typename Relocs<RelTy>::const_iterator &i) {
     }
   }
   // Get an offset in an output section this relocation is applied to.
-  uint64_t offset = getter.get(ctx, rel.r_offset);
-  if (offset == uint64_t(-1))
-    return;
+  uint64_t offset = rel.r_offset;
 
   RelExpr expr =
       ctx.target->getRelExpr(type, sym, sec->content().data() + offset);
@@ -1574,6 +1569,13 @@ void RelocationScanner::scanOne(typename Relocs<RelTy>::const_iterator &i) {
     }
   }
 
+  scanOneAux<ELFT>(i, expr, type, offset, sym, addend);
+}
+
+template <class ELFT, class RelIt>
+void RelocationScanner::scanOneAux(RelIt &i, RelExpr expr, RelType type,
+                                   uint64_t offset, Symbol &sym,
+                                   int64_t addend) {
   // If the relocation does not emit a GOT or GOTPLT entry but its computation
   // uses their addresses, we need GOT or GOTPLT to be created.
   //
@@ -1649,13 +1651,10 @@ void RelocationScanner::scan(Relocs<RelTy> rels) {
   if (ctx.arg.emachine == EM_PPC64)
     checkPPC64TLSRelax<RelTy>(*sec, rels);
 
-  // For EhInputSection, OffsetGetter expects the relocations to be sorted by
-  // r_offset. In rare cases (.eh_frame pieces are reordered by a linker
-  // script), the relocations may be unordered.
   // On SystemZ, all sections need to be sorted by r_offset, to allow TLS
   // relaxation to be handled correctly - see SystemZ::getTlsGdRelaxSkip.
   SmallVector<RelTy, 0> storage;
-  if (isa<EhInputSection>(sec) || ctx.arg.emachine == EM_S390)
+  if (ctx.arg.emachine == EM_S390)
     rels = sortRels(rels, storage);
 
   if constexpr (RelTy::IsCrel) {
@@ -1680,11 +1679,9 @@ void RelocationScanner::scan(Relocs<RelTy> rels) {
                       });
 }
 
-template <class ELFT>
-void RelocationScanner::scanSection(InputSectionBase &s, bool isEH) {
+template <class ELFT> void RelocationScanner::scanSection(InputSectionBase &s) {
   sec = &s;
-  getter = OffsetGetter(s);
-  const RelsOrRelas<ELFT> rels = s.template relsOrRelas<ELFT>(!isEH);
+  const RelsOrRelas<ELFT> rels = s.template relsOrRelas<ELFT>();
   if (rels.areRelocsCrel())
     scan<ELFT>(rels.crels);
   else if (rels.areRelocsRel())
@@ -1693,6 +1690,28 @@ void RelocationScanner::scanSection(InputSectionBase &s, bool isEH) {
     scan<ELFT>(rels.relas);
 }
 
+template <class ELFT> void RelocationScanner::scanEhSection(EhInputSection &s) {
+  sec = &s;
+  OffsetGetter getter(s);
+  auto rels = s.rels;
+  s.relocations.reserve(rels.size());
+  for (auto it = rels.begin(); it != rels.end();) {
+    auto i = it++;
+    // Ignore R_*_NONE and other marker relocations.
+    if (i->expr == R_NONE)
+      continue;
+    uint64_t offset = getter.get(ctx, i->offset);
+    Symbol *sym = i->sym;
+    // Skip if the relocation offset is within a dead piece.
+    if (offset == uint64_t(-1))
+      continue;
+    if (sym->isUndefined() &&
+        maybeReportUndefined(ctx, cast<Undefined>(*sym), *sec, offset))
+      continue;
+    scanOneAux<ELFT>(it, i->expr, i->type, offset, *sym, i->addend);
+  }
+}
+
 template <class ELFT> void elf::scanRelocations(Ctx &ctx) {
   // Scan all relocations. Each relocation goes through a series of tests to
   // determine if it needs special treatment, such as creating GOT, PLT,
@@ -1725,7 +1744,7 @@ template <class ELFT> void elf::scanRelocations(Ctx &ctx) {
       RelocationScanner scanner(ctx);
       for (Partition &part : ctx.partitions) {
         for (EhInputSection *sec : part.ehFrame->sections)
-          scanner.template scanSection<ELFT>(*sec, /*isEH=*/true);
+          scanner.template scanEhSection<ELFT>(*sec);
         if (part.armExidx && part.armExidx->isLive())
           for (InputSection *sec : part.armExidx->exidxSections)
             if (sec->isLive())

[smithp35]

IIUC this is the common tail between scanEhSection and scanSection.

As .ehframe sections describe code sections, I wouldn't expect to see any GOT generating or TLS relocations. Could we call processAux directly from scanEhSection instead of calling scanOneAux. Then we could fold this part back into scanOne.

[MaskRay]

Called process (renamed from processAux) directly now.

[smithp35]

We've now got scanOne, processAux and now scanOneAux. Could we write a small comment explaining what each one is supposed to do? I think we may be able to do without scanOne.

[MaskRay]

Created #161229 to rewrite the file-level comment and give an overview.

[smithp35]

LGTM. Thanks for the update.