[BOLT] Search section based on relocation symbol

[yavtuk]

Summary

We need to search referenced section based on relocations symbol section
to properly match end section symbols. For example on some binaries we
can observe that init_array_end/fini_array_end might be "placed" in to
the gap and since no section could be found for address the relocation
would be skipped resulting in wrong ADRP imm after emitting new text
resulting in binary sigsegv.

Credits for the test to Vladislav Khmelevskii aka yota9.

[yota9]

nit

[yota9]

Gentle reminder

[yota9]

Gentle ping @maksfb @rafauler

[rafauler]

Thanks for the fix! Overall looks good, just a question on the test itself

bolt/test/AArch64/array_end.c

31

Could you move REQUIRES, RUN, CHECK line to line 5, at the top of this file, for consistency with other checks?

32–33

Is it possible to prune the script to only the relevant lines that trigger the problematic behavior?

[yota9]

bolt/test/AArch64/array_end.c

32–33

Sure, I was thinking to check init_array_end symboll too, but there is a problem that bolt creates symbols using address only without respecting section belonging. E.g init_array_end symbol address == __fini_array_start , so on disassemble we might get wrong symbol names. It is not currently a big issue, but in case we would move (in this exmaple) fini_array section separately from init_array - it might be the problem. Just you to know, but it is out of scope of this patch.

[yota9]

Address comments

[rafauler]

bolt/test/AArch64/Inputs/array_end.ld_script

2–5 ↗

(On Diff #495248)

Probably worth asking on discourse if there are any legal implications of checking in a GNU linkerscript into LLVM's codebase, since GNU works with a different license

[yota9]

@rafauler I've found out that we can make small linker scripts for lld, it uses it as a hint and adds missing sections by it self.

[yota9]

nit

[rafauler]

LGTM

[yavtuk]

Commit: 0776fc32b15dc76c6b43c41fc4471552833265de

bolt/lib/Rewrite/RewriteInstance.cpp

if (IsFromCode) {
     }
   }
 
   MCSymbol *ReferencedSymbol = nullptr;
   if (!IsSectionRelocation)
     if (BinaryData *BD = BC->getBinaryDataByName(SymbolName))
       ReferencedSymbol = BD->getSymbol();
 
-  ErrorOr<BinarySection &> ReferencedSection =
-      BC->getSectionForAddress(SymbolAddress);
+  ErrorOr<BinarySection &> ReferencedSection{std::errc::bad_address};
+  symbol_iterator SymbolIter = Rel.getSymbol();
+  if (SymbolIter != InputFile->symbol_end()) {
+    SymbolRef Symbol = *SymbolIter;
+    section_iterator Section =
+        cantFail(Symbol.getSection(), "cannot get symbol section");
+    if (Section != InputFile->section_end()) {
+      Expected<StringRef> SectionName = Section->getName();
+      if (SectionName && !SectionName->empty())
+        ReferencedSection = BC->getUniqueSectionByName(*SectionName);
+    }
+  }
+
+  if (!ReferencedSection)
+    ReferencedSection = BC->getSectionForAddress(SymbolAddress);
 
   const bool IsToCode = ReferencedSection && ReferencedSection->isText();
 
   // Special handling of PC-relative relocations.
   if (!IsAArch64 && Relocation::isPCRelative(RType)) {
     if (!IsFromCode && IsToCode) {
       // PC-relative relocations from data to code are tricky since the
       // original information is typically lost after linking, even with

bolt/test/AArch64/Inputs/array_end.lld_script

+SECTIONS {
+  .fini_array    :
+  {
+    PROVIDE_HIDDEN (__fini_array_start = .);
+    KEEP (*(SORT_BY_INIT_PRIORITY(.fini_array.*) SORT_BY_INIT_PRIORITY(.dtors.*)))
+    PROVIDE_HIDDEN (__fini_array_end = .);
+  }
+
+  . = . + 128;
+
+  .text : { *(.text) }
+}

bolt/test/AArch64/array_end.c

+// Test checks that bolt properly finds end section label.
+// Linker script contains gap after destructor array, so
+// __init_array_end address would not be owned by any section.
+
+// REQUIRES: system-linux
+// RUN: %clang %cflags -no-pie %s -o %t.exe -Wl,-q \
+// RUN:   -Wl,-T %S/Inputs/array_end.lld_script
+// RUN: llvm-bolt %t.exe -o %t.bolt --print-disasm \
+// RUN:  --print-only="callFini" | FileCheck %s
+
+// CHECK: adr [[REG:x[0-28]+]], "__fini_array_end/1"
+
+__attribute__((destructor)) void destr() {}
+
+__attribute__((noinline)) void callFini() {
+  extern void (*__fini_array_start[])();
+  extern void (*__fini_array_end[])();
+  unsigned long Count = __fini_array_end - __fini_array_start;
+  for (unsigned long I = 0; I < Count; ++I)
+    (*__fini_array_start[I])();
+}
+
+void _start() { callFini(); }