[LLD][ELF] Allow empty (.init|.preinit|.fini)_array to be RELRO

[psmith]

Summary

The default GNU linker script uses the following idiom for the array sections. I'll use .init_array here, but this also applies to .preinit_array and .fini_array sections.

.init_array    :
{
  PROVIDE_HIDDEN (__init_array_start = .);
  KEEP (*(.init_array))
  PROVIDE_HIDDEN (__init_array_end = .);
}

The C-library will take references to the _start and _end symbols to process the array. This will make LLD keep the OutputSection even if there are no .init_array sections. As the current check for RELRO uses the section type for .init_array the above example with no .init_array InputSections fails the checks as there are no .init_array sections to give the OutputSection a type of SHT_INIT_ARRAY. This often leads to a non-contiguous RELRO error message, which I've seen in at least PR https://bugs.llvm.org/show_bug.cgi?id=44698 when suggesting a linker script as a work-around.

The simple fix is to a textual section match as well as a section type match. Potential alternatives involve setting the type of the name of an OutputSection called .init_array to SHT_INIT_ARRAY somewhere else. Or potentially trying to work out if an OutputSection has no content and can be considered RELRO regardless of its name.

[MaskRay]

lld/test/ELF/relro-init-fini-script.s

19

It is not very clear what the 3 CHECK lines are about...

See pre_init_fini_array.s (D76852)

Suggest:

// RUN: llvm-readelf -S %t.so | FileCheck %s

// CHECK:      Name           Type     Address
// CHECK:      .preinit_array PROGBITS {{0+}}[[# %x,ADDR:]]
// CHECK-NEXT: .init_array    PROGBITS {{0+}}[[# ADDR]]
// CHECK-NEXT: .fini_array    PROGBITS {{0+}}[[# ADDR]]
// CHECK-NEXT: .data.rel.ro   PROGBITS {{0+}}[[# ADDR]]

23

Nit: the indentation is not needed.

[psmith]

Thanks for the comments, I've added some tests to check that the _array sections are zero size, also to show the extent of the RELRO includes the section before and after.

[MaskRay]

Looks great!

[grimar]

lld/test/ELF/relro-init-fini-script.s

15

I`d suggest using llvm-readelf -S -segments then:
there is no need to have both llvm-readelf and llvm-readobj calls I think.

(Then perhaps you will be able to use {{0+}}[[# ADDR]] check to test PT_GNU_RELRO`s VA).

33

Missing an empty line after before ".section .data.rel.ro ...."

[grimar]

LGTM with 2 nits.

lld/test/ELF/relro-init-fini-script.s

15

(Then perhaps you will be able to use {{0+}}# ADDR check to test PT_GNU_RELRO`s VA).

Ah, though it is not needed, with llvm-readelf -segments you can just test the same what is now tested with llvm-readobj: VA and size of the PT_GNU_RELRO.

lld/ELF/Writer.cpp

static bool isRelroSection(const OutputSection *sec) {
 
   // Sections with some special names are put into RELRO. This is a
   // bit unfortunate because section names shouldn't be significant in
   // ELF in spirit. But in reality many linker features depend on
   // magic section names.
   StringRef s = sec->name;
   return s == ".data.rel.ro" || s == ".bss.rel.ro" || s == ".ctors" ||
          s == ".dtors" || s == ".jcr" || s == ".eh_frame" ||
-         s == ".openbsd.randomdata";
+         s == ".fini_array" || s == ".init_array" ||
+         s == ".openbsd.randomdata" || s == ".preinit_array";
 }
 
 // We compute a rank for each section. The rank indicates where the
 // section should be placed in the file.  Instead of using simple
 // numbers (0,1,2...), we use a series of flags. One for each decision
 // point when placing the section.
 // Using flags has two key properties:
 // * It is easy to check if a give branch was taken.

lld/test/ELF/relro-init-fini-script.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-none-elf %s -o %t.o
+// RUN: echo "SECTIONS { \
+// RUN:  .dynamic 0x10000 : { *(.dynamic) } \
+// RUN:  .preinit_array : { PROVIDE_HIDDEN (__preinit_array_start = .); \
+// RUN:                  KEEP (*(.preinit_array)) } \
+// RUN:  .init_array : { PROVIDE_HIDDEN (__init_array_start = .); \
+// RUN:                  KEEP (*(.init_array)) } \
+// RUN:  .fini_array : { PROVIDE_HIDDEN (__fini_array_start = .); \
+// RUN:                  KEEP (*(.fini_array)) } \
+// RUN:  .data.rel.ro : { *(.data.rel.ro) } \
+// RUN:  .data : { *(.data) } } " > %t.script
+// RUN: ld.lld %t.o -o %t.so --shared --script=%t.script
+// RUN: llvm-readobj --segments %t.so | FileCheck %s
+
+/// Check that an empty .init_array, .fini_array or .preinit_array that is
+/// kept due to symbol references, is still counted as RELRO.
+
+// CHECK:      Type: PT_GNU_RELRO
+// CHECK-NEXT: Offset: 0x2000
+// CHECK-NEXT: VirtualAddress: 0x10000
+
+        .section .data.rel.ro, "aw", %progbits
+        .global foo
+        .quad foo
+
+        .data
+        .quad __init_array_start
+        .quad __fini_array_start
+        .quad __preinit_array_start