[COFF] Don't treat DWARF sections as GC roots

[rnk]

Summary

DWARF sections are typically live and not COMDAT, so they would be
treated as GC roots. Enabling DWARF would essentially keep all code with
debug info alive, preventing any section GC.

Fixes PR45273

[mstorsjo]

Looks good I think. I'm not very familiar with the GC parts of LLD, but the explanations sound sensible to me, and I believe this solution has been vetted by at least two others already (bug reporter and @rnk).

[MaskRay]

The code change looks good, but I've got some test suggestions. I have some thoughts if COFF/MarkLive.cpp is ever made more sophisticated. In ELF, gc is for sections which are part of the load image (SHF_ALLOC) and non-SHF_ALLOC sections are generally not discarded. I don't know what is similar to SHF_ALLOC in COFF, though.

lld/test/COFF/gc-dwarf.s

6

This can be --input-file=%t.map to avoid a cat.

9

-NEXT:

11

It will be better to prepend # MAP: Symbol to make clear it is the MAP column.

In not-so-rare circumstances the absolute path of %t.obj may contain main.
main{{$}} can prevent this pattern from matching a path component.

23

Optional: in ELF we tend to omit unneeded details like xorl %eax, %eax; retq.
But we will keep retq because an empty section and a non-empty section make a difference for GNU linkers.

[rnk]

In D76935#1946511, @MaskRay wrote:

The code change looks good, but I've got some test suggestions. I have some thoughts if COFF/MarkLive.cpp is ever made more sophisticated. In ELF, gc is for sections which are part of the load image (SHF_ALLOC) and non-SHF_ALLOC sections are generally not discarded. I don't know what is similar to SHF_ALLOC in COFF, though.

I guess the analogous flag would be IMAGE_SCN_MEM_DISCARDABLE. It is phrased in the opposite direction: sections lacking the flag are loaded, sections with the flag are not loaded into memory, and are generally sorted to the end of the image.

Strangely, LLVM MC suppresses the D assembler flag when printing textual assembly, and infers it from the section name:
https://github.com/llvm/llvm-project/blob/a55daa146166353236aa528546397226bee9363b/llvm/lib/MC/MCParser/COFFAsmParser.cpp#L258-L260

I realized, though, that there is one major difference in behavior. isDWARF() returns true for .eh_frame sections, but they are not discardable. They must be loaded into memory for unwinding to work. So, in this situation, GC will not work:

void foo();
int main() { foo(); } 
void unused1() { foo(); } 
void unused2() { foo(); }

Compiled like so:
$ clang -O1 --target=x86_64-windows-gnu -c t.c -fdwarf-exceptions -ffunction-sections

The unwind data is not separated into nice little comdat .pdata/.xdata sections, it is a single monolithic .eh_frame section:

$ llvm-objdump -h t.o

t.o:    file format coff-x86-64

Sections:
Idx Name          Size     VMA              Type
  0 .text         00000000 0000000000000000 TEXT
  1 .data         00000000 0000000000000000 DATA
  2 .bss          00000000 0000000000000000 BSS
  3 .text$main    0000001a 0000000000000000 TEXT
  4 .text$unused1 0000000e 0000000000000000 TEXT
  5 .text$unused2 0000000e 0000000000000000 TEXT
  6 .eh_frame     00000080 0000000000000000 DATA
  7 .llvm_addrsig 00000000 0000000000000000

I checked, and it has relocations against all the functions. :( Blech. I think we have to do the isDWARF check.

lld/test/COFF/gc-dwarf.s

11

Yeah, unfortunately there is no other leading text on the line to match against to try to prevent that, so I left it as was. It's a bit redundant to check which symbols were included anyway. But I can make it tighter.

23

Sure, I based this assembly on normal compiler output. I prefer it when the assembly appears to do something semi-plausible.

[rnk]

• Test .eh_frame refs

[rnk]

In D76935#1946511, @MaskRay wrote:

The code change looks good, but I've got some test suggestions. I have some thoughts if COFF/MarkLive.cpp is ever made more sophisticated.

Also, I hope we don't have to make it sophisticated. Simplicity is a good thing. :)

Anyway, I didn't make any code changes, so I will land this with the updated .eh_frame test. Thanks!

[MaskRay]

LGTM.

lld/COFF/MarkLive.cpp

 void markLive(ArrayRef<Chunk *> chunks) {
   ScopedTimer t(gctimer);
 
   // We build up a worklist of sections which have been marked as live. We only
   // push into the worklist when we discover an unmarked section, and we mark
   // as we push, so sections never appear twice in the list.
   SmallVector<SectionChunk *, 256> worklist;
 
-  // COMDAT section chunks are dead by default. Add non-COMDAT chunks.
+  // COMDAT section chunks are dead by default. Add non-COMDAT chunks. Do not
+  // traverse DWARF sections. They are live, but they should not keep other
+  // sections alive.
   for (Chunk *c : chunks)
     if (auto *sc = dyn_cast<SectionChunk>(c))
-      if (sc->live)
+      if (sc->live && !sc->isDWARF())
         worklist.push_back(sc);
 
   auto enqueue = [&](SectionChunk *c) {
     if (c->live)
       return;
     c->live = true;
     worklist.push_back(c);
   };

lld/test/COFF/gc-dwarf.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-windows-msvc %s -filetype=obj -o %t.obj
+# RUN: lld-link -lldmap:%t.map -out:%t.exe -opt:ref -entry:main %t.obj -verbose 2>&1 | FileCheck %s
+# RUN: FileCheck %s --check-prefix=MAP --input-file=%t.map
+
+# CHECK:      Discarded unused1
+# CHECK-NEXT: Discarded unused2
+# CHECK-NOT: Discarded
+
+# MAP: In Symbol
+# MAP: gc-dwarf.s.tmp.obj:(.text)
+# MAP: {{ main$}}
+# MAP: gc-dwarf.s.tmp.obj:(.text)
+# MAP: {{ used$}}
+
+  .def   @feat.00; .scl 3; .type  0; .endef
+  .globl  @feat.00
+.set @feat.00, 0
+
+  .def   main; .scl 2; .type  32; .endef
+  .section  .text,"xr",one_only,main
+  .globl  main
+main:
+  callq used
+  xorl  %eax, %eax
+  retq
+
+  .def   used; .scl 2; .type  32; .endef
+  .section  .text,"xr",one_only,used
+  .globl  used
+used:
+  retq
+
+
+  .def   unused1; .scl  2; .type  32; .endef
+  .section  .text,"xr",one_only,unused1
+  .globl  unused1
+unused1:
+  retq
+
+  .def   unused2; .scl  2; .type  32; .endef
+  .section  .text,"xr",one_only,unused2
+  .globl  unused2
+unused2:
+  retq
+
+# This isn't valid DWARF, but LLD doesn't care. Make up some data that
+# references the functions above.
+.section .debug_info,"r"
+.long main@IMGREL
+.long unused1@IMGREL
+.long unused2@IMGREL
+
+# Similarly, .eh_frame unwind info should not keep functions alive. Again, this
+# is not valid unwind info, but it doesn't matter for testing purposes.
+.section .eh_frame,"r"
+.long main@IMGREL
+.long unused1@IMGREL
+.long unused2@IMGREL