RFC: [LLD] [COFF] Include VirtualSize when sizing sections

[mstorsjo]

Summary

This fixes cases where the Go compiler created bss sections with a zero SizeOfRawData, with the size commnicated via the VirtualSize field.

GNU ld does pick up on and allocate section size based on VirtualSize in addition to SizeOfRawData.

See https://bugs.llvm.org/show_bug.cgi?id=49172 for the full backstory. I'm not entirely sure it's worthwhile doing (if this is the only case that happens to allocate bss sections differently, and it's getting fixed upstream there), and the fix looks overly generic like it could have a number of other unexpected side effects, but it doesn't at least trigger any change in any test case.

[rnk]

Is it possible to create objects with bss sections like this when enabling -fcommon? I vaguely recall running into a few cases where common linkage mattered in chrome.

[rnk]

In D96698#2583410, @rnk wrote:

Is it possible to create objects with bss sections like this when enabling -fcommon? I vaguely recall running into a few cases where common linkage mattered in chrome.

I played around a bit, but I wasn't able to get LLVM to make anything like the object described. I think I would prefer not to put this conditional in the hot path if we agree that tools shouldn't produce objects like this.

[mstorsjo]

In D96698#2583410, @rnk wrote:

Is it possible to create objects with bss sections like this when enabling -fcommon? I vaguely recall running into a few cases where common linkage mattered in chrome.

No, commons are handled entirely differently.

In D96698#2583428, @rnk wrote:

I played around a bit, but I wasn't able to get LLVM to make anything like the object described. I think I would prefer not to put this conditional in the hot path if we agree that tools shouldn't produce objects like this.

Yeah I'd tend to agree at this point. From the outset of the bug report, it seemed like a potentially valid case (as there's two size fields one could use for conveying the size in the case of bss sections where there's no actual data in the object file anyway), but given that the fix is highly generic and essentially could affect handling of all object files' sections, I think we'd rather not do it. The issue has been fixed on the Go side now anyways.

lld/COFF/Chunks.h

public:
   public:
     symbol_iterator() = default;
 
     Symbol *operator*() const { return file->getSymbol(I->SymbolTableIndex); }
   };
 
   SectionChunk(ObjFile *file, const coff_section *header);
   static bool classof(const Chunk *c) { return c->kind() == SectionKind; }
-  size_t getSize() const { return header->SizeOfRawData; }
+  size_t getSize() const {
+    return std::max(header->VirtualSize, header->SizeOfRawData);
+  }
   ArrayRef<uint8_t> getContents() const;
   void writeTo(uint8_t *buf) const;
 
   uint32_t getOutputCharacteristics() const {
     return header->Characteristics & (permMask | typeMask);
   }
   StringRef getSectionName() const {
     return StringRef(sectionNameData, sectionNameSize);

lld/test/COFF/bss-virtualsize.yaml

+# RUN: yaml2obj %s -o %t.obj
+# RUN: lld-link -out:%t.exe -entry:entry -subsystem:console %t.obj
+# RUN: llvm-readobj -s %t.exe | FileCheck %s
+
+# CHECK:      Name: .data
+# CHECK-NEXT: VirtualSize: 0x100
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     C3
+  - Name:            .bss
+    Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    VirtualSize:     256
+    Alignment:       16
+    SectionData:     ''
+    SizeOfRawData:   0
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          1
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        40735498
+      Number:          1
+  - Name:            .bss
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          256
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          2
+  - Name:            '@feat.00'
+    Value:           0
+    SectionNumber:   -1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            entry
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssvar
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...