DWARF breaks garbage collection in PE/COFF

[vit9696]

Bugzilla Link	45273
Resolution	FIXED
Resolved on	Apr 13, 2020 13:55
Version	unspecified
OS	All
Blocks	#44654
CC	@dwblaikie,@MaskRay,@mstorsjo,@rnk,@rui314,@smeenai,@tstellar
Fixed by commit(s)	c579a5b edbe962

Extended Description

Generating DWARF debug information breaks dead code removal in PE/COFF images. Consider test.c file with the following content:

void unused() {}
void entry() {}

After compiling and linking it with LLD I expect unused function to be stripped from the resulting binary, but for some reason it does not happen. These are the commands I execute:

$ clang -g -fno-builtin -ffunction-sections -fdata-sections -fno-common -fno-stack-protector -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc -m64 -mno-red-zone -mcmodel=small -O0 -target x86_64-unknown-windows-gnu -gdwarf -funwind-tables -c -o test.obj test.c

$ lld-link /OUT:test.dll /NOLOGO /NODEFAULTLIB /IGNORE:4001 /OPT:REF /OPT:ICF=10 /ALIGN:32 /FILEALIGN:32 /Machine:X64 /DLL /ENTRY:entry /SUBSYSTEM:EFI_BOOT_SERVICE_DRIVER /SAFESEH:NO /BASE:0 /DEBUG:DWARF /lldmap test.obj

$ llvm-readobj -t test.dll

File: test.dll
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
Symbols [
Symbol {
Name: .text
Value: 0
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .text$unused
Value: 16
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: unused
Value: 16
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Function (0x2)
StorageClass: External (0x2)
AuxSymbolCount: 0
}
Symbol {
Name: .text$entry
Value: 0
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: entry
Value: 0
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Function (0x2)
StorageClass: External (0x2)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_str
Value: 0
Section: .debug_str (8)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_abbrev
Value: 0
Section: .debug_abbrev (3)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_info
Value: 0
Section: .debug_info (4)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_ranges
Value: 0
Section: .debug_ranges (7)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_macinfo
Value: 0
Section: .debug_macinfo (6)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
Symbol {
Name: .debug_line
Value: 0
Section: .debug_line (5)
BaseType: Null (0x0)
ComplexType: Null (0x0)
StorageClass: Static (0x3)
AuxSymbolCount: 0
}
]

Not even (well, expectedly) it changes after stripping:

$ llvm-objcopy --strip-unneeded test.dll
$ llvm-readobj -t test.dll

File: test.dll
Format: COFF-x86-64
Arch: x86_64
AddressSize: 64bit
Symbols [
Symbol {
Name: unused
Value: 16
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Function (0x2)
StorageClass: External (0x2)
AuxSymbolCount: 0
}
Symbol {
Name: entry
Value: 0
Section: .text (1)
BaseType: Null (0x0)
ComplexType: Function (0x2)
StorageClass: External (0x2)
AuxSymbolCount: 0
}
]

From what I can tell the issue is in MarkLive implementation of LLD, which assumes DWARF symbols are live, and then links everything in the file:

llvm-project/lld/COFF/MarkLive.cpp

Lines 31 to 35 in 332fc71

	// COMDAT section chunks are dead by default. Add non-COMDAT chunks.
	for (Chunk *c : chunks)
	if (auto *sc = dyn_cast<SectionChunk>(c))
	if (sc->live)
	worklist.push_back(sc);

These .debug_xxx symbols have 0x42100040 section characteristics (IMAGE_SCN_MEM_READ|IMAGE_SCN_MEM_DISCARDABLE|IMAGE_SCN_ALIGN_1BYTES|IMAGE_SCN_CNT_INITIALIZED_DATA), and can probably be skipped by IMAGE_SCN_MEM_DISCARDABLE flag, but I do not know if it is safe. For now I changed the code locally to simply skip symbols with DWARF section names in them.

for (Chunk *c : chunks) {
if (auto *sc = dyn_cast(c)) {
if (!sc->live || sc->isDWARF())
continue;
worklist.push_back(sc);
}
}

[rnk]

CodeView chunks don't appear in the main chunks list because we separate them out into debug chunks here:

llvm-project/lld/COFF/InputFiles.cpp

Lines 213 to 216 in 332fc71

	// CodeView sections are stored to a different vector because they are not
	// linked in the regular manner.
	if (c->isCodeView())
	debugChunks.push_back(c);

The same treatment isn't appropriate for DWARF because DWARF sections are included in the PE output file.

I think your fix of checking for isDWARF when enqueuing roots is reasonable, but won't that end up discarding comdat DWARF sections? Are there any of those? Type units maybe (-ftype-units)?

[dwblaikie]

CodeView chunks don't appear in the main chunks list because we separate
them out into debug chunks here:
https://github.com/llvm/llvm-project/blob/
332fc71/lld/COFF/InputFiles.cpp#L213-L216

The same treatment isn't appropriate for DWARF because DWARF sections are
included in the PE output file.

I think your fix of checking for isDWARF when enqueuing roots is reasonable,
but won't that end up discarding comdat DWARF sections? Are there any of
those? Type units maybe (-ftype-units)?

Yep, type units (-fdebug-types-section) uses comdats at least on ELF - not sure of much of how DWARF-on-COFF works, but ideally it'd be using that sort of thing there too.

[vit9696]

Reid, I do not have a good knowledge on how PE/COFF works, however:

1. Type units are not generated by default (I think even for ELFs).

2. Type units are currently not supported for PE, and produce an error: unsupported option '-fdebug-types-section' for target 'x86_64-unknown-windows-gnu'.

3. I can debug structure types just fine with lldb. Probably because they appear in .debug_info:

void unused() {

}

typedef struct MYSTR_ {
int a;
} MYSTR;

MYSTR glob;

void entry() {
glob.a += 1;
}

llvm-dwarfdump -a test.dll

...
0x0000002a: DW_TAG_variable
DW_AT_name ("glob")
DW_AT_type (0x0000003f "MYSTR")
DW_AT_external (true)
DW_AT_decl_file ("/Users/user/Desktop/example/test.c")
DW_AT_decl_line (9)
DW_AT_location (DW_OP_addr 0x380)

0x0000003f: DW_TAG_typedef
DW_AT_type (0x0000004a "MYSTR_")
DW_AT_name ("MYSTR")
DW_AT_decl_file ("/Users/user/Desktop/example/test.c")
DW_AT_decl_line (7)

0x0000004a: DW_TAG_structure_type
DW_AT_name ("MYSTR_")
DW_AT_byte_size (0x04)
DW_AT_decl_file ("/Users/user/Desktop/example/test.c")
DW_AT_decl_line (5)
...

4. llvm-readdwarf -a output is identical with and without the patch, except some offsets, for both the example in the bugreport and the example with a custom structure in this message.

To be honest, after some playing with it I think it should be more or less safe to apply my change, and my only worries are mostly about performance. I do not know whether there is a better way though.

[rnk]

On reflection, I think there would be no behavior change, even if we did support COFF type units. As of today, we treat non-comdat sections as roots. Comdat dwarf sections (type units) are not roots, and are not marked live, and are not included in the output if section GC is enabled. This was actually a bug in LLD/ELF's section GC, and we have the same "bug" (really, more like unimplemented feature).

So, I don't have any concerns with your fix.

[vit9696]

Thanks! I do not have LLVM commit access, in case there are no objections, could anybody push the change to master? I am a bit reluctant to go phabricator route for a one-line change.

[rnk]

Sure, I found a minute and put together a test for it in this patch here:
https://reviews.llvm.org/D76935
Should be reviewed and land shortly.

[MaskRay]

This patch looks good, but I don't know enough about COFF.

ELF --gc-sections has many subtle rules. I have a summary on
https://sourceware.org/legacy-ml/binutils/2020-02/msg00035.html

error: unsupported option '-fdebug-types-section' for target 'x86_64-unknown-windows-gnu

When we start to support it, we may think about the aforementioned rules.

[rnk]

Pushed as c579a5b

[vit9696]

Thank you very much for this. Do you think it is going to be possible to schedule this for 10.0.1 release?

[rnk]

Marked as blocked on the 10.0.1 release bug to ask the question.

There is some risk that this change will discard more sections than intended, but that is almost an inherent risk with section GC / dead stripping / OPT:REF. There is always the trivial workaround for users of not using the feature, so I think we should merge it.

[tstellar]

Merged: edbe962

[tstellar]

mentioned in issue #44654