[COFF] Fix SECTION and SECREL relocation handling for absolute symbols

[rnk]

Summary

For SECTION relocations against absolute symbols, MSVC emits the largest
output section index plus one. I've implemented that by threading a
global variable through DefinedAbsolute that is filled in by the Writer.
A more library-oriented approach would be to thread the Writer through
Chunk::writeTo and SectionChunk::applyRel*, but Rui seems to prefer
doing it this way.

MSVC rejects SECREL relocations against absolute symbols, but only when
the relocation is in a real output section. When the relocation is in a
CodeView debug info section destined for the PDB, it seems that this
relocation error is suppressed, and absolute symbols become zeros in the
object file. This is easily implemented by checking the input section
from which we're applying relocations.

This should fix errors about safe_se_handler_table and
guard_fids_table when linking the CRT and generating a PDB.

[ruiu]

LGTM

lld/COFF/Chunks.cpp

61 ↗

(On Diff #103660)

nit: I would reverse the condition so that it becomes one line shorter.

lld/COFF/Symbols.cpp

64 ↗

(On Diff #103660)

You want to use uint32_t.

[rnk]

lld/COFF/Symbols.cpp

64 ↗

(On Diff #103660)

True. uint16_t would probably be best.

lld/trunk/COFF/Chunks.cpp

SectionChunk::SectionChunk(ObjectFile *F, const coff_section *H)
   Live = !Config->DoGC || !isCOMDAT();
 }
 
 static void add16(uint8_t *P, int16_t V) { write16le(P, read16le(P) + V); }
 static void add32(uint8_t *P, int32_t V) { write32le(P, read32le(P) + V); }
 static void add64(uint8_t *P, int64_t V) { write64le(P, read64le(P) + V); }
 static void or16(uint8_t *P, uint16_t V) { write16le(P, read16le(P) | V); }
 
+static void applySecRel(const SectionChunk *Sec, uint8_t *Off, Defined *Sym) {
+  // Don't apply section relative relocations to absolute symbols in codeview
+  // debug info sections. MSVC does not treat such relocations as fatal errors,
+  // and they can be found in the standard library for linker-provided symbols
+  // like __guard_fids_table and __safe_se_handler_table.
+  if (!(isa<DefinedAbsolute>(Sym) && Sec->isCodeView()))
+    add32(Off, Sym->getSecrel());
+}
+
 void SectionChunk::applyRelX64(uint8_t *Off, uint16_t Type, Defined *Sym,
                                uint64_t P) const {
   uint64_t S = Sym->getRVA();
   switch (Type) {
   case IMAGE_REL_AMD64_ADDR32:   add32(Off, S + Config->ImageBase); break;
   case IMAGE_REL_AMD64_ADDR64:   add64(Off, S + Config->ImageBase); break;
   case IMAGE_REL_AMD64_ADDR32NB: add32(Off, S); break;
   case IMAGE_REL_AMD64_REL32:    add32(Off, S - P - 4); break;
   case IMAGE_REL_AMD64_REL32_1:  add32(Off, S - P - 5); break;
   case IMAGE_REL_AMD64_REL32_2:  add32(Off, S - P - 6); break;
   case IMAGE_REL_AMD64_REL32_3:  add32(Off, S - P - 7); break;
   case IMAGE_REL_AMD64_REL32_4:  add32(Off, S - P - 8); break;
   case IMAGE_REL_AMD64_REL32_5:  add32(Off, S - P - 9); break;
   case IMAGE_REL_AMD64_SECTION:  add16(Off, Sym->getSectionIndex()); break;
-  case IMAGE_REL_AMD64_SECREL:   add32(Off, Sym->getSecrel()); break;
+  case IMAGE_REL_AMD64_SECREL:   applySecRel(this, Off, Sym); break;
   default:
     fatal("unsupported relocation type 0x" + Twine::utohexstr(Type));
   }
 }
 
 void SectionChunk::applyRelX86(uint8_t *Off, uint16_t Type, Defined *Sym,
                                uint64_t P) const {
   uint64_t S = Sym->getRVA();
   switch (Type) {
   case IMAGE_REL_I386_ABSOLUTE: break;
   case IMAGE_REL_I386_DIR32:    add32(Off, S + Config->ImageBase); break;
   case IMAGE_REL_I386_DIR32NB:  add32(Off, S); break;
   case IMAGE_REL_I386_REL32:    add32(Off, S - P - 4); break;
   case IMAGE_REL_I386_SECTION:  add16(Off, Sym->getSectionIndex()); break;
-  case IMAGE_REL_I386_SECREL:   add32(Off, Sym->getSecrel()); break;
+  case IMAGE_REL_I386_SECREL:   applySecRel(this, Off, Sym); break;
   default:
     fatal("unsupported relocation type 0x" + Twine::utohexstr(Type));
   }
 }
 
 static void applyMOV(uint8_t *Off, uint16_t V) {
   write16le(Off, (read16le(Off) & 0xfbf0) | ((V & 0x800) >> 1) | ((V >> 12) & 0xf));
   write16le(Off + 2, (read16le(Off + 2) & 0x8f00) | ((V & 0x700) << 4) | (V & 0xff));
if (Sym->isExecutable())
     S |= 1;
   switch (Type) {
   case IMAGE_REL_ARM_ADDR32:    add32(Off, S + Config->ImageBase); break;
   case IMAGE_REL_ARM_ADDR32NB:  add32(Off, S); break;
   case IMAGE_REL_ARM_MOV32T:    applyMOV32T(Off, S + Config->ImageBase); break;
   case IMAGE_REL_ARM_BRANCH20T: applyBranch20T(Off, S - P - 4); break;
   case IMAGE_REL_ARM_BRANCH24T: applyBranch24T(Off, S - P - 4); break;
   case IMAGE_REL_ARM_BLX23T:    applyBranch24T(Off, S - P - 4); break;
-  case IMAGE_REL_ARM_SECREL:    add32(Off, Sym->getSecrel()); break;
+  case IMAGE_REL_ARM_SECREL:    applySecRel(this, Off, Sym); break;
   default:
     fatal("unsupported relocation type 0x" + Twine::utohexstr(Type));
   }
 }
 
 void SectionChunk::writeTo(uint8_t *Buf) const {
   if (!hasData())
     return;

lld/trunk/COFF/Symbols.h

public:
 
   static bool classof(const SymbolBody *S) {
     return S->kind() == DefinedAbsoluteKind;
   }
 
   uint64_t getRVA() { return VA - Config->ImageBase; }
   void setVA(uint64_t V) { VA = V; }
 
+  // The sentinel absolute symbol section index. Section index relocations
+  // against absolute symbols resolve to this 16 bit number, and it is the
+  // largest valid section index plus one. This is written by the Writer.
+  static uint16_t OutputSectionIndex;
+
 private:
   uint64_t VA;
 };
 
 // This is a kind of absolute symbol but relative to the image base.
 // Unlike absolute symbols, relocations referring this kind of symbols
 // are subject of the base relocation. This type is used rarely --
 // mainly for __ImageBase.

lld/trunk/COFF/Symbols.cpp

COFFSymbolRef DefinedCOFF::getCOFFSymbol() {
   size_t SymSize =
       cast<ObjectFile>(File)->getCOFFObj()->getSymbolTableEntrySize();
   if (SymSize == sizeof(coff_symbol16))
     return COFFSymbolRef(reinterpret_cast<const coff_symbol16 *>(Sym));
   assert(SymSize == sizeof(coff_symbol32));
   return COFFSymbolRef(reinterpret_cast<const coff_symbol32 *>(Sym));
 }
 
+uint16_t DefinedAbsolute::OutputSectionIndex = 0;
+
 static Chunk *makeImportThunk(DefinedImportData *S, uint16_t Machine) {
   if (Machine == AMD64)
     return make<ImportThunkChunkX64>(S);
   if (Machine == I386)
     return make<ImportThunkChunkX86>(S);
   assert(Machine == ARMNT);
   return make<ImportThunkChunkARM>(S);
 }

lld/trunk/COFF/Writer.cpp

uint64_t Defined::getSecrel() {
   if (auto *D = dyn_cast<DefinedRegular>(this))
     return getRVA() - D->getChunk()->getOutputSection()->getRVA();
   fatal("SECREL relocation points to a non-regular symbol: " + toString(*this));
 }
 
 uint64_t Defined::getSectionIndex() {
   if (auto *D = dyn_cast<DefinedRegular>(this))
     return D->getChunk()->getOutputSection()->SectionIndex;
+  if (auto *D = dyn_cast<DefinedAbsolute>(this))
+    return DefinedAbsolute::OutputSectionIndex;
   fatal("SECTION relocation points to a non-regular symbol: " +
         toString(*this));
 }
 
 bool Defined::isExecutable() {
   const auto X = IMAGE_SCN_MEM_EXECUTE;
   if (auto *D = dyn_cast<DefinedRegular>(this))
     return D->getChunk()->getOutputSection()->getPermissions() & X;
for (auto &P : Config->Section) {
     uint32_t Perm = P.second;
     if (auto *Sec = findSection(Name))
       Sec->setPermissions(Perm);
   }
 }
 
 // Write section contents to a mmap'ed file.
 void Writer::writeSections() {
+  // Record the section index that should be used when resolving a section
+  // relocation against an absolute symbol.
+  DefinedAbsolute::OutputSectionIndex = OutputSections.size() + 1;
+
   uint8_t *Buf = Buffer->getBufferStart();
   for (OutputSection *Sec : OutputSections) {
     uint8_t *SecBuf = Buf + Sec->getFileOff();
     // Fill gaps between functions in .text with INT3 instructions
     // instead of leaving as NUL bytes (which can be interpreted as
     // ADD instructions).
     if (Sec->getPermissions() & IMAGE_SCN_CNT_CODE)
       memset(SecBuf, 0xCC, Sec->getRawSize());

lld/trunk/test/COFF/pdb-secrel-absolute.yaml

+# RUN: yaml2obj %s -o %t.obj
+# RUN: lld-link -debug -entry:main -out:%t.exe -pdb:%t.pdb %t.obj
+# RUN: llvm-pdbutil raw -symbols %t.pdb | FileCheck %s
+
+# There is an S_GDATA32 symbol record with .secrel32 and .secidx relocations in
+# it in this debug info. This is similar to the relocations in the loadcfg.obj
+# file in the MSVC CRT. We need to make sure that our relocation logic matches
+# MSVC's for these absolute, linker-provided symbols.
+
+# CHECK: Mod 0000 |
+# CHECK-NEXT: - S_GDATA32 [size = 36] `__guard_fids_table`
+# CHECK-NEXT:     type = 0x0022 (unsigned long), addr = 0003:0000
+# CHECK-NEXT: Mod 0001 | `* Linker *`:
+
+--- !COFF
+header:          
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:        
+  - Name:            '.debug$S'
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+    Alignment:       1
+    Subsections:     
+      - !Symbols
+        Records:         
+          - Kind:            S_GDATA32
+            DataSym:         
+              Type:            34
+              DisplayName:     __guard_fids_table
+      - !StringTable
+        Strings:         
+    Relocations:     
+      - VirtualAddress:  20
+        SymbolName:      __guard_fids_table
+        Type:            IMAGE_REL_AMD64_SECREL
+      - VirtualAddress:  24
+        SymbolName:      __guard_fids_table
+        Type:            IMAGE_REL_AMD64_SECTION
+  - Name:            '.text$mn'
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     488D0500000000C3
+    Relocations:     
+      - VirtualAddress:  3
+        SymbolName:      __guard_fids_table
+        Type:            IMAGE_REL_AMD64_REL32
+symbols:         
+  - Name:            '.debug$S'
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          372
+      NumberOfRelocations: 6
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            '.text$mn'
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition: 
+      Length:          8
+      NumberOfRelocations: 1
+      NumberOfLinenumbers: 0
+      CheckSum:        1092178131
+      Number:          0
+  - Name:            main
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            __guard_fids_table
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

lld/trunk/test/COFF/secidx-absolute.s

+# RUN: llvm-mc %s -filetype=obj -triple=x86_64-windows-msvc -o %t.obj
+# RUN: lld-link -entry:main -nodefaultlib %t.obj -out:%t.exe
+# RUN: llvm-readobj %t.exe -sections -section-data | FileCheck %s
+
+# Section relocations against absolute symbols resolve to the last real ouput
+# section index plus one.
+
+.text
+.global main
+main:
+ret
+
+.section .rdata,"dr"
+.secidx __guard_fids_table
+
+# CHECK: Sections [
+# CHECK:   Section {
+# CHECK:     Number: 1
+# CHECK:     Name: .rdata (2E 72 64 61 74 61 00 00)
+# CHECK:     SectionData (
+# CHECK:       0000: 0300                                 |..|
+# CHECK:     )
+# CHECK:   }
+# CHECK:   Section {
+# CHECK:     Number: 2
+# CHECK:     Name: .text (2E 74 65 78 74 00 00 00)
+# CHECK:     VirtualSize: 0x1
+# CHECK:     SectionData (
+# CHECK:       0000: C3                                   |.|
+# CHECK:     )
+# CHECK:   }
+# CHECK-NOT: Section
+# CHECK: ]

lld/trunk/test/COFF/secrel-absolute.s

+# RUN: llvm-mc %s -filetype=obj -triple=x86_64-windows-msvc -o %t.obj
+# RUN: not lld-link -entry:main -nodefaultlib %t.obj -out:%t.exe 2>&1 | FileCheck %s
+
+# secrel relocations against absolute symbols are errors.
+
+# CHECK: SECREL relocation points to a non-regular symbol: __guard_fids_table
+
+.text
+.global main
+main:
+ret
+
+.section .rdata,"dr"
+.secrel32 __guard_fids_table