[COFF] Simplify symbol table output section computation

[smeenai]

Summary

Rather than using a loop to compare symbol RVAs to the starting RVAs of
sections to determine which section a symbol belongs to, just get the
output section of a symbol directly via its chunk, and bail if the
symbol doesn't have an output section, which avoids having to hardcode
logic for handling dead symbols, CodeView symbols, etc. This was
suggested by Reid Kleckner; thank you.

This also fixes writing out symbol tables in the presence of RVA table
input sections (e.g. .sxdata and .gfids). Such sections aren't written
to the output file directly, so their RVA is 0, and the loop would thus
fail to find an output section for them, resulting in a segfault. Extend
some existing tests to cover this case.

Fixes PR37584.

[smeenai]

COFF/Writer.cpp

611–614

Does this need to be generalized to all RVA table sections, e.g. the control flow guard sections? We aren't using that right now, so it's not an immediate concern, but just wondering in general.

[rnk]

COFF/Writer.cpp

611–614

It does. I think these are all the cases where we pull out chunks and set them aside so they don't make it to the final output:

if (C->isCodeView())
  DebugChunks.push_back(C);
else if (Config->GuardCF != GuardCFLevel::Off && Name == ".gfids$y")
  GuardFidChunks.push_back(C);
else if (Config->GuardCF != GuardCFLevel::Off && Name == ".gljmp$y")
  GuardLJmpChunks.push_back(C);
else if (Name == ".sxdata")
  SXDataChunks.push_back(C);

They are still marked as "live" so we can figure out which debug info (or RVA tables) to toss and which to keep, but none of them will ever make it into the output.

[smeenai]

COFF/Writer.cpp

611–614

Okay. Sounds like we should be setting some property on SectionChunk to indicate that, to avoid having to duplicate all that logic in Writer? Something along the lines of WrittenToOutputDirectly (since the section's contents do get written to the output, just in a debug info/RVA table form)? Alternatively we could leave CodeView sections as is and mark the other ones as RVATableChunkInput or something along those lines, and then ignore those sections in the Writer (in addition to the existing ignoring of CodeView sections).

[rnk]

COFF/Writer.cpp

611–614

I think we'd actually want to set that in Writer::assignAddresses, since it would subsume the isLive check as well.

Actually, remind me why we can't just use Def->getChunk()->getOutputSection() (with some null checks) here? Why are we searching the output sections by RVA anyway? I think it's only a few cases. If there's no OutputSection, then we shouldn't write it to the symbol table.

[smeenai]

COFF/Writer.cpp

611–614

Good question. I did some archeology, and the search for output sections by RVA actually dates all the way back to https://reviews.llvm.org/D11023, which first introduced symbol tables. Let me try changing that and seeing if tests are happy.

[smeenai]

@rnk's suggestion

[rnk]

lgtm

[pcc]

COFF/Writer.cpp

638

Can we move this switch to the top of the function? Then the only code that needs to care about output sections will be in the default case of the switch.

[smeenai]

COFF/Writer.cpp

638

Good idea; I'll throw up a patch in a bit.

[smeenai]

COFF/Writer.cpp

638

https://reviews.llvm.org/D47497

COFF/Writer.cpp

size_t Writer::addEntryToStringTable(StringRef Str) {
   return OffsetOfEntry;
 }
 
 Optional<coff_symbol16> Writer::createSymbol(Defined *Def) {
   // Relative symbols are unrepresentable in a COFF symbol table.
   if (isa<DefinedSynthetic>(Def))
     return None;
 
-  // Don't write dead symbols or symbols in codeview sections to the symbol
+  // Don't write symbols that won't be written to the output to the symbol
   // table.
-  if (!Def->isLive())
-    return None;
-  if (auto *D = dyn_cast<DefinedRegular>(Def))
-    if (D->getChunk()->isCodeView())
+  OutputSection *OS = nullptr;
+  if (Chunk *C = Def->getChunk()) {
+    OS = C->getOutputSection();
+    if (!OS)
       return None;
+  }
 
   coff_symbol16 Sym;
   StringRef Name = Def->getName();
   if (Name.size() > COFF::NameSize) {
     Sym.Name.Offset.Zeroes = 0;
     Sym.Name.Offset.Offset = addEntryToStringTable(Name);
   } else {
     memset(Sym.Name.ShortName, 0, COFF::NameSize);
     memcpy(Sym.Name.ShortName, Name.data(), Name.size());
   }
 
   if (auto *D = dyn_cast<DefinedCOFF>(Def)) {
     COFFSymbolRef Ref = D->getCOFFSymbol();
     Sym.Type = Ref.getType();
     Sym.StorageClass = Ref.getStorageClass();
   } else {
     Sym.Type = IMAGE_SYM_TYPE_NULL;
     Sym.StorageClass = IMAGE_SYM_CLASS_EXTERNAL;
   }
   Sym.NumberOfAuxSymbols = 0;
 
   switch (Def->kind()) {
   case Symbol::DefinedAbsoluteKind:
     Sym.Value = Def->getRVA();
     Sym.SectionNumber = IMAGE_SYM_ABSOLUTE;
     break;
   default: {
-    uint64_t RVA = Def->getRVA();
-    OutputSection *Sec = nullptr;
-    for (OutputSection *S : OutputSections) {
-      if (S->getRVA() > RVA)
-        break;
-      Sec = S;
-    }
-    Sym.Value = RVA - Sec->getRVA();
-    Sym.SectionNumber = Sec->SectionIndex;
+    assert(OS && "Writing dead symbol to symbol table");
+    Sym.Value = Def->getRVA() - OS->getRVA();
+    Sym.SectionNumber = OS->SectionIndex;
     break;
   }
   }
   return Sym;
 }
 
 void Writer::createSymbolAndStringTable() {
   // PE/COFF images are limited to 8 byte section names. Longer names can be

test/COFF/gfids-gc.s

 # RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
 # RUN: lld-link %t.obj -guard:nolongjmp -out:%t.exe -opt:noref -entry:main
 # RUN: llvm-readobj -file-headers -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
+# RUN: lld-link %t.obj -guard:nolongjmp -out:%t.exe -opt:noref -entry:main -debug:dwarf
+# RUN: llvm-readobj -file-headers -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
 # RUN: lld-link %t.obj -guard:nolongjmp -out:%t.exe -opt:ref -entry:main
 # RUN: llvm-readobj -file-headers -coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-GC
 
 # This assembly is meant to mimic what CL emits for this kind of C code when
 # /Gw (-fdata-sections) is enabled:
 # int f() { return 42; }
 # int g() { return 13; }
 # int (*fp1)() = &f;

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-basereloc -coff-load-config -file-headers %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
+# RUN: lld-link %t.obj -safeseh -out:%t.exe -opt:noref -entry:main -debug:dwarf
+# RUN: llvm-readobj -coff-basereloc -coff-load-config -file-headers %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-basereloc -coff-load-config -file-headers %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-NOT: IMAGE_DLL_CHARACTERISTICS_NO_SEH
 # CHECK-NOGC: BaseReloc [
 # CHECK-NOGC:   Entry {
 # CHECK-NOGC:     Type: HIGHLOW
 # CHECK-NOGC: LoadConfig [
 # CHECK-NOGC:   Size: 0x48
-# CHECK-NOGC:   SEHandlerTable: 0x402048
+# CHECK-NOGC:   SEHandlerTable: 0x
 # CHECK-NOGC:   SEHandlerCount: 1
 # CHECK-NOGC: ]
 # CHECK-NOGC: SEHTable [
 # CHECK-NOGC-NEXT:   0x401006
 # CHECK-NOGC-NEXT: ]
 
 # If we enable GC, the exception handler should be removed, and we should add
 # the DLL characteristic flag that indicates that there are no exception