[LLD] [COFF] Sort .relocs before all other discardable sections

[mstorsjo]

Summary

If a binary is stripped, which can remove discardable sections (except for the .relocs section, which also is marked as discardable as it isn't loaded at runtime, only read by the loader), the .relocs section should be first of them, in order not to create gaps in the image.

Previously, binaries with relocations were broken if they were stripped by GNU binutils strip. Trying to execute such binaries produces an error about "xx is not a valid win32 application".

This fixes GNU binutils bug 23348.

Prior to SVN r329370 (which didn't intend to have functional changes), the code for moving discardable sections to the end didn't clearly express how other discardable sections should be ordered compared to .relocs, even though the change probably retained the same end result as before.

After SVN r329370, the code (and comments) more clearly indicate that it tries to make the .relocs section the absolutely last one; this patch changes that.

This matches how GNU binutils ld sorts .relocs compared to dwarf debug info sections.

[pcc]

COFF/Writer.cpp

479 ↗

(On Diff #155595)

I think you can make this

if (S->Header.Characteristics & IMAGE_SCN_MEM_DISCARDABLE)
  return 2;

and remove the part about RelocSec. The .reloc section will be sorted at the beginning of the discardable sections because we create it explicitly above (this is the same way that we ensure that the sections appear in the same order as link.exe); you can move your comment there.

[mstorsjo]

Applied @pcc's suggestion

[mstorsjo]

Ping @pcc

[pcc]

LGTM

lld/trunk/COFF/Writer.cpp

for (auto Pair : Map) {
     OutputSection *Sec = CreateSection(Name, OutChars);
     std::vector<Chunk *> &Chunks = Pair.second;
     for (Chunk *C : Chunks)
       Sec->addChunk(C);
   }
 
   // Finally, move some output sections to the end.
   auto SectionOrder = [&](OutputSection *S) {
-    // .reloc should come last of all since it refers to RVAs of data in the
-    // previous sections.
-    if (S == RelocSec)
-      return 3;
     // Move DISCARDABLE (or non-memory-mapped) sections to the end of file because
-    // the loader cannot handle holes.
+    // the loader cannot handle holes. Stripping can remove other discardable ones
+    // than .reloc, which is first of them (created early).
     if (S->Header.Characteristics & IMAGE_SCN_MEM_DISCARDABLE)
       return 2;
     // .rsrc should come at the end of the non-discardable sections because its
     // size may change by the Win32 UpdateResources() function, causing
     // subsequent sections to move (see https://crbug.com/827082).
     if (S == RsrcSec)
       return 1;
     return 0;

lld/trunk/test/COFF/sort-debug.test

 # RUN: yaml2obj < %s > %t.obj
 # RUN: lld-link /debug /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck %s
 # RUN: lld-link /debug:dwarf /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck %s
 # RUN: lld-link /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck -check-prefix=NODEBUG %s
 # RUN: lld-link /debug:symtab /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck -check-prefix=NODEBUG %s
 
 # CHECK: Name: .text
+# CHECK: Name: .reloc
 # CHECK: Name: .debug_abbrev
 # CHECK: Name: .debug_info
 # CHECK: Name: .debug_line
 # CHECK: Name: .debug_pubnames
 # CHECK: Name: .debug_pubtypes
-# CHECK: Name: .reloc
 
 # NODEBUG: Name: .text
+# NODEBUG: Name: .reloc
 # NODEBUG-NOT: Name: .debug_abbrev
 # NODEBUG-NOT: Name: .debug_info
 # NODEBUG-NOT: Name: .debug_line
 # NODEBUG-NOT: Name: .debug_pubnames
 # NODEBUG-NOT: Name: .debug_pubtypes
-# NODEBUG: Name: .reloc
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_I386
   Characteristics: [  ]
 sections:
   - Name:            .text
     Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]