Mitigate relocation overflow [part 2 of 2]

[shenhan]

Summary

This CL is to mitigate R_X86_64_PC32 relocation overflow problems for huge binaries that has near 2G allocated sections.

By examining those binaries, I found these 2 issues contributes to the problem:
1). huge ".dynsym" and ".dynstr" stands in the way between .rodata and .text
2). _init_array_start/end are placed at 0 if no ".init_array" presents, this causes .text relocation against them become more prone to overflow.

This CL is to address 2).
( 1) is addressed in D45788

[shenhan]

Comments?

[shenhan]

Hi George, mind take a look? (This is a follow up of D45788.)

[grimar]

It looks a bit hacky. But I think it should work. Rui, what do you think?

ELF/Writer.cpp

1710

We do not write like that I think: !(DefaultOutSec = findSection(".text").
Please move the assignment to the DefaultOutSec outside of the if.

I think ".text" can be a default for both Pic and !Pic probably.

[shenhan]

ELF/Writer.cpp

1710

Revised. Thanks.

[ruiu]

ELF/Writer.cpp

1704–1721

I'd think that this comment could be improved by making it clear what is the default behavior, what your problem is, and how to solve it. The important thing is that future readers will be able to understand this comment without too much context. I'd write something like this.

If a section does not exist, there's ambiguity as to how we define _start and _end symbols for an init/fini section. Since the loader assume that the symbols are always defined, we need to always define them. But what value? The loader iterates over all pointers between _start and _end to run global ctors/dtors, so if the section is empty, their symbol values don't actually matter as long as _start and _end point to the same location.

That said, we don't want to set the symbols to 0 (which is probably the simplest value) because that could cause some program to fail to link due to relocation overflow, if their program text is above 2 GiB. We use the address of the .text section instead to prevent that failure.

1709

Since this function is very small, we probably should use shorter names. I'd name this just Text.

[grimar]

ELF/Writer.cpp

1709

But Text does not look correct for Out::ElfHeader case. Maybe OutSec?

[grimar]

ELF/Writer.cpp

1709

Oops. OutSec is confusing since we have OS. DefaultSec?

[ruiu]

I think Text is fine, but if you don't like it, I'd name it Default.

[grimar]

In D46200#1103407, @ruiu wrote:

I think Text is fine, but if you don't like it, I'd name it Default.

Default sounds/fits better IMO. Thanks!

[shenhan]

Thanks. (Comments revised and DefaoutOutSec -> Default)

[ruiu]

LGTM

ELF/Writer.cpp

1711

Add an empty comment line //.

[grimar]

LGTM.

[grimar]

Was committed as r332688

Hint: If you will put "Differential revision: https://reviews.llvm.org/DXXXXX" to the end of your commit message, reviews will be closed automatically.

ELF/Writer.cpp

   // of finalizing other sections.
   for (OutputSection *Sec : OutputSections)
     Sec->finalize<ELFT>();
 }
 
 // The linker is expected to define SECNAME_start and SECNAME_end
 // symbols for a few sections. This function defines them.
 template <class ELFT> void Writer<ELFT>::addStartEndSymbols() {
-  auto Define = [&](StringRef Start, StringRef End, OutputSection *OS) {
-    // These symbols resolve to the image base if the section does not exist.
-    // A special value -1 indicates end of the section.
+  // If a section does not exist, there's ambiguity as to how we
+  // define _start and _end symbols for an init/fini section. Since
+  // the loader assume that the symbols are always defined, we need to
+  // always define them. But what value? The loader iterates over all
+  // pointers between _start and _end to run global ctors/dtors, so if
+  // the section is empty, their symbol values don't actually matter
+  // as long as _start and _end point to the same location.
+
+  // That said, we don't want to set the symbols to 0 (which is
+  // probably the simplest value) because that could cause some
+  // program to fail to link due to relocation overflow, if their
+  // program text is above 2 GiB. We use the address of the .text
+  // section instead to prevent that failure.
+  OutputSection *Default = findSection(".text");
+  if (!Default)
+    Default = Out::ElfHeader;
+  auto Define = [=](StringRef Start, StringRef End, OutputSection *OS) {
     if (OS) {
       addOptionalRegular(Start, OS, 0);
       addOptionalRegular(End, OS, -1);
     } else {
-      if (Config->Pic)
-        OS = Out::ElfHeader;
-      addOptionalRegular(Start, OS, 0);
-      addOptionalRegular(End, OS, 0);
+      addOptionalRegular(Start, Default, 0);
+      addOptionalRegular(End, Default, 0);
     }
   };
 
   Define("__preinit_array_start", "__preinit_array_end", Out::PreinitArray);
   Define("__init_array_start", "__init_array_end", Out::InitArray);
   Define("__fini_array_start", "__fini_array_end", Out::FiniArray);
 
   if (OutputSection *Sec = findSection(".ARM.exidx"))

test/ELF/pre_init_fini_array_missing.s

 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 // RUN: ld.lld %t -o %t2
 // RUN: llvm-objdump -d %t2 | FileCheck %s
 // RUN: ld.lld -pie %t -o %t3
 // RUN: llvm-objdump -d %t3 | FileCheck --check-prefix=PIE %s
 // REQUIRES: x86
 
 .globl _start
 _start:
   call __preinit_array_start
   call __preinit_array_end
   call __init_array_start
   call __init_array_end
   call __fini_array_start
   call __fini_array_end
 
-// With no .init_array section the symbols resolve to 0
-// 0 - (0x201000 + 5) = -2101253
-// 0 - (0x201005 + 5) = -2101258
-// 0 - (0x20100a + 5) = -2101263
-// 0 - (0x20100f + 5) = -2101268
-// 0 - (0x201014 + 5) = -2101273
-// 0 - (0x201019 + 5) = -2101278
+// With no .init_array section the symbols resolve to .text.
+// 0x201000 - (0x201000 + 5) = -5
+// 0x201000 - (0x201005 + 5) = -10
+// ...
 
 // CHECK: Disassembly of section .text:
 // CHECK-NEXT:  _start:
-// CHECK-NEXT:   201000:    e8 fb ef df ff     callq    -2101253
-// CHECK-NEXT:   201005:    e8 f6 ef df ff     callq    -2101258
-// CHECK-NEXT:   20100a:    e8 f1 ef df ff     callq    -2101263
-// CHECK-NEXT:   20100f:    e8 ec ef df ff     callq    -2101268
-// CHECK-NEXT:   201014:    e8 e7 ef df ff     callq    -2101273
-// CHECK-NEXT:   201019:    e8 e2 ef df ff     callq    -2101278
+// CHECK-NEXT:   201000:    e8 fb ff ff ff     callq    -5
+// CHECK-NEXT:   201005:    e8 f6 ff ff ff     callq    -10
+// CHECK-NEXT:   20100a:    e8 f1 ff ff ff     callq    -15
+// CHECK-NEXT:   20100f:    e8 ec ff ff ff     callq    -20
+// CHECK-NEXT:   201014:    e8 e7 ff ff ff     callq    -25
+// CHECK-NEXT:   201019:    e8 e2 ff ff ff     callq    -30
 
-// In position-independent binaries, they resolve to the image base.
+// In position-independent binaries, they resolve to .text too.
 
 // PIE:      Disassembly of section .text:
 // PIE-NEXT: _start:
-// PIE-NEXT:     1000:  e8 fb ef ff ff  callq -4101
-// PIE-NEXT:     1005:  e8 f6 ef ff ff  callq -4106
-// PIE-NEXT:     100a:  e8 f1 ef ff ff  callq -4111
-// PIE-NEXT:     100f:  e8 ec ef ff ff  callq -4116
-// PIE-NEXT:     1014:  e8 e7 ef ff ff  callq -4121
-// PIE-NEXT:     1019:  e8 e2 ef ff ff  callq -4126
+// PIE-NEXT:     1000:  e8 fb ff ff ff  callq   -5
+// PIE-NEXT:     1005:  e8 f6 ff ff ff  callq   -10
+// PIE-NEXT:     100a:  e8 f1 ff ff ff  callq   -15
+// PIE-NEXT:     100f:  e8 ec ff ff ff  callq   -20
+// PIE-NEXT:     1014:  e8 e7 ff ff ff  callq   -25
+// PIE-NEXT:     1019:  e8 e2 ff ff ff  callq   -30