[coff] use relative instead of absolute __safe_se_handler_base when present

[inglorion]

Summary

__safe_se_handler_base should be either absolute 0 (when no SafeSEH
table is present), or relative to the image base (when the table is
present). An earlier change inadvertedly made the symbol absolute in
both cases, leading to the SafeSEH table not being locatble at run
time. This change fixes that and updates the safeseh test to check for
the presence of the relocation.

[ruiu]

LGTM

[inglorion]

The code change mostly just moves code around, but this helps because we previously fixed the symbol type too late, so that it was still output as an absolute symbol. After moving the code around, we now output __safe_se_handler_base with the correct symbol type.

lld/trunk/COFF/Writer.cpp

private:
   void createMiscChunks();
   void createImportTables();
   void createExportTable();
   void assignAddresses();
   void removeEmptySections();
   void createSymbolAndStringTable();
   void openFile(StringRef OutputPath);
   template <typename PEHeaderTy> void writeHeader();
-  void fixSafeSEHSymbols();
+  void createSEHTable(OutputSection *RData);
   void setSectionPermissions();
   void writeSections();
   void writeBuildId();
   void sortExceptionTable();
 
   llvm::Optional<coff_symbol16> createSymbol(Defined *D);
   size_t addEntryToStringTable(StringRef Str);
 
void Writer::run() {
   // to read the contents of the existing output file.
   PreviousBuildId = loadExistingBuildId(Config->OutputFile);
   openFile(Config->OutputFile);
   if (Config->is64()) {
     writeHeader<pe32plus_header>();
   } else {
     writeHeader<pe32_header>();
   }
-  fixSafeSEHSymbols();
   writeSections();
   sortExceptionTable();
   writeBuildId();
 
   if (!Config->PDBPath.empty() && Config->Debug) {
 
     assert(BuildId);
     createPDB(Symtab, OutputSections, SectionTable, *BuildId->BuildId);
if (Config->Debug) {
     BuildId = CVChunk;
     DebugRecords.push_back(CVChunk);
 
     RData->addChunk(DebugDirectory);
     for (Chunk *C : DebugRecords)
       RData->addChunk(C);
   }
 
-  // Create SEH table. x86-only.
-  if (Config->Machine != I386)
-    return;
-
-  std::set<Defined *> Handlers;
-
-  for (ObjFile *File : ObjFile::Instances) {
-    if (!File->SEHCompat)
-      return;
-    for (Symbol *B : File->SEHandlers) {
-      // Make sure the handler is still live. Assume all handlers are regular
-      // symbols.
-      auto *D = dyn_cast<DefinedRegular>(B);
-      if (D && D->getChunk()->isLive())
-        Handlers.insert(D);
-    }
-  }
-
-  if (!Handlers.empty()) {
-    SEHTable = make<SEHTableChunk>(Handlers);
-    RData->addChunk(SEHTable);
-  }
+  createSEHTable(RData);
 }
 
 // Create .idata section for the DLL-imported symbol table.
 // The format of this section is inherently Windows-specific.
 // IdataContents class abstracted away the details for us,
 // so we just let it create chunks and add them to the section.
 void Writer::createImportTables() {
   if (ImportFile::Instances.empty())

 }
 
 void Writer::openFile(StringRef Path) {
   Buffer = check(
       FileOutputBuffer::create(Path, FileSize, FileOutputBuffer::F_executable),
       "failed to open " + Path);
 }
 
-void Writer::fixSafeSEHSymbols() {
-  if (!SEHTable)
+void Writer::createSEHTable(OutputSection *RData) {
+  // Create SEH table. x86-only.
+  if (Config->Machine != I386)
     return;
+
+  std::set<Defined *> Handlers;
+
+  for (ObjFile *File : ObjFile::Instances) {
+    if (!File->SEHCompat)
+      return;
+    for (Symbol *B : File->SEHandlers) {
+      // Make sure the handler is still live. Assume all handlers are regular
+      // symbols.
+      auto *D = dyn_cast<DefinedRegular>(B);
+      if (D && D->getChunk()->isLive())
+        Handlers.insert(D);
+    }
+  }
+
+  if (Handlers.empty())
+    return;
+
+  SEHTable = make<SEHTableChunk>(Handlers);
+  RData->addChunk(SEHTable);
+
   // Replace the absolute table symbol with a synthetic symbol pointing to the
   // SEHTable chunk so that we can emit base relocations for it and resolve
   // section relative relocations.
   Symbol *T = Symtab->find("___safe_se_handler_table");
   Symbol *C = Symtab->find("___safe_se_handler_count");
   replaceSymbol<DefinedSynthetic>(T, T->getName(), SEHTable);
   cast<DefinedAbsolute>(C)->setVA(SEHTable->getSize() / 4);
 }

lld/trunk/test/COFF/safeseh.s

 # RUN: llvm-mc -triple i686-windows-msvc %s -filetype=obj -o %t.obj
 # RUN: lld-link %t.obj -safeseh -out:%t.exe -opt:noref -entry:main
-# RUN: llvm-readobj -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
+# RUN: llvm-readobj -coff-basereloc -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
 # RUN: lld-link %t.obj -safeseh -out:%t.exe -opt:ref -entry:main
-# RUN: llvm-readobj -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-GC
+# RUN: llvm-readobj -coff-basereloc -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-GC
 
+# __safe_se_handler_table needs to be relocated against ImageBase.
+# check that the relocation is present.
+# CHECK-NOGC: BaseReloc [
+# CHECK-NOGC:   Entry {
+# CHECK-NOGC:     Type: HIGHLOW
 # CHECK-NOGC: LoadConfig [
 # CHECK-NOGC:   Size: 0x48
 # CHECK-NOGC:   SEHandlerTable: 0x401048
 # CHECK-NOGC:   SEHandlerCount: 1
 # CHECK-NOGC: ]
 # CHECK-NOGC: SEHTable [
 # CHECK-NOGC-NEXT:   0x402006
 # CHECK-NOGC-NEXT: ]
 
+# Without the SEH table, the address is absolute, so check that we do
+# not have a relocation for it.
+# CHECK-GC: BaseReloc [
+# CHECK-GC-NEXT: ]
 # CHECK-GC: LoadConfig [
 # CHECK-GC:   Size: 0x48
 # CHECK-GC:   SEHandlerTable: 0x0
 # CHECK-GC:   SEHandlerCount: 0
 # CHECK-GC: ]
 # CHECK-GC-NOT: SEHTable