[LLD] [COFF] Remove empty sections before calculating the size of section headers

[mstorsjo]

Summary

The number of sections is used in assignAddresses (in finalizeAddresses) and the space for all sections is permanent from that point on, even if we later decide we won't write some of them.

The VirtualSize field also gets calculated in assignAddresses, so we need to manually check whether the section is empty here instead.

[mstorsjo]

I don't have any good test for this, do you think one is needed? It avoids a 512 byte hole in the produced files.

[pcc]

I don't understand this change, won't there always need to be an alignment gap of up to 512 bytes between sections? So it won't matter whether we align once or several times.

[mstorsjo]

In D54495#1297650, @pcc wrote:

I don't understand this change, won't there always need to be an alignment gap of up to 512 bytes between sections? So it won't matter whether we align once or several times.

Yes, but there is an alignment gap of 512 bytes more than necessary due to this.

In one test of mine, SizeOfHeaders ends up as 1536 after alignment, based on an initial OutputSections.size() = 24. After removing the empty sections, I end up with OutputSections.size() = 15. If I have this value already when doing assignAddresses, the aligned SizeOfHeaders ends up as 1024. So it doesn't get rid of the gap altogether, but it makes sure it's not unnecessarily big.

[pcc]

In D54495#1297657, @mstorsjo wrote:

In D54495#1297650, @pcc wrote:

I don't understand this change, won't there always need to be an alignment gap of up to 512 bytes between sections? So it won't matter whether we align once or several times.

Yes, but there is an alignment gap of 512 bytes more than necessary due to this.

In one test of mine, SizeOfHeaders ends up as 1536 after alignment, based on an initial OutputSections.size() = 24. After removing the empty sections, I end up with OutputSections.size() = 15. If I have this value already when doing assignAddresses, the aligned SizeOfHeaders ends up as 1024. So it doesn't get rid of the gap altogether, but it makes sure it's not unnecessarily big.

Got it, it's due to the alignment gap between the section headers and the section contents, not the alignment between sections.

I think it should be possible to test this by providing an input file with a large number of empty sections and one non-empty section and checking the PointerToRawData value for the non-empty section.

[ruiu]

COFF/Writer.cpp

905–906

It feels like S->getVirtualSize() should return 0 if all sections in it are empty. Can you fix the code computing output section size instead of computing a bogus value as a size and then remove such section here?

[mstorsjo]

COFF/Writer.cpp

905–906

S->getVirtualSize() doesn't do any calculation at all, it only returns Header.VirtualSize (where Header is a llvm::object::coff_section which will be written to disk as is). If we'd make getVirtualSize() actually do the calculation, it'd redo it every time we query it later.

Or should we move this into a isEmpty() helper method in OutputSection instead?

[ruiu]

COFF/Writer.cpp

905–906

Yeah I know it is an accessor, but I wonder why we compute a non-zero value as a size even if all its members are of size zero.

[mstorsjo]

COFF/Writer.cpp

905–906

We don't compute a non-zero value. If I switch execution order so that we run removeEmptySections() before finalizeAddresses(), we haven't actually calculated that value yet, so all sections have VirtualSize set to 0 still.

[mstorsjo]

Added a testcase based on @pcc's suggestion.

[ruiu]

COFF/Writer.cpp

905–906

Ah, understood. I think assginAddresses is a misleading function name if it computes not only section addresses but also size of sections. Do you think you can separate assignAddresses into two functions, one to compute size and the other to assign offsets, and call the former before removeEmptySections? Or, maybe we can compute section size in this removeEmptySections (if we do that, we probably should rename this function computeSectionSize or something though.)

[mstorsjo]

COFF/Writer.cpp

905–906

I could split it, but I feel it would be less meaningful than this form.

Even though computing the size of a section is a different thing than assigning addresses, they are very closely linked. Part of the relevant loop (simplified) is this:

uint64_t VirtualSize = 0;
Sec->Header.VirtualAddress = RVA;
for (Chunk *C : Sec->Chunks) {
  VirtualSize = alignTo(VirtualSize, C->Alignment);
  C->setRVA(RVA + VirtualSize);
  C->OutputSectionOff = VirtualSize;
  VirtualSize += C->getSize();
}
Sec->Header.VirtualSize = VirtualSize;

For cases with thunks, we need to run this multiple times as part of finalizeAddresses/assignAddresses. So yes, we could split it into a different method, but we would still need to redo all the work in order to assign RVAs. At the point of removeEmptySections we don't really need to know the exact size of sections, we just need to know if they are non-empty.

[ruiu]

Makes sense. LGTM.

Thank you for doing this.

[mstorsjo]

In D54495#1298076, @ruiu wrote:

Makes sense. LGTM.

Thank you for doing this.

Actually, it turns out this doesn't work like this, sorry I hadn't run the full testsuite on this change before now.

It turns out that this change removes the base reloc section. That section is empty up until assignAddresses where it is populated, and afaik it does have to be re-updated every time we adjust addresses (for thunks).

So basically, we should remove all empty sections except for the reloc section, and reserve space for it. After finalizeAddresses, if it turns out the reloc section really is empty, we could remove that one as well.

This turned into something much more complicated than I had originally intended...

[mstorsjo]

Updated to not remove RelocSec prematurely, and to not remove sections that contain MergeChunks, as these get their size allocated later (in assignAddresses).

At this point, this ended more messy than I intended first, so I'm not sure if this is worth it any more.

[mstorsjo]

Ping @ruiu @pcc, do you think this still is worthwhile doing even when the patch looks like this?

[mstorsjo]

Once again after holidays - ping @ruiu @pcc, do you think this still is worthwhile doing even when the patch looks like this?

[ruiu]

LGTM

COFF/Writer.cpp

private:
   void createMiscChunks();
   void createImportTables();
   void appendImportThunks();
   void locateImportTables(
       std::map<std::pair<StringRef, uint32_t>, std::vector<Chunk *>> &Map);
   void createExportTable();
   void mergeSections();
   void readRelocTargets();
+  void removeUnusedSections();
   void assignAddresses();
   void finalizeAddresses();
   void removeEmptySections();
   void createSymbolAndStringTable();
   void openFile(StringRef OutputPath);
   template <typename PEHeaderTy> void writeHeader();
   void createSEHTable();
   void createRuntimePseudoRelocs();
void Writer::run() {
 
   createImportTables();
   createSections();
   createMiscChunks();
   appendImportThunks();
   createExportTable();
   mergeSections();
   readRelocTargets();
+  removeUnusedSections();
   finalizeAddresses();
   removeEmptySections();
   setSectionPermissions();
   createSymbolAndStringTable();
 
   if (FileSize > UINT32_MAX)
     fatal("image size (" + Twine(FileSize) + ") " +
         "exceeds maximum allowable size (" + Twine(UINT32_MAX) + ")");

 
 void Writer::createExportTable() {
   if (Config->Exports.empty())
     return;
   for (Chunk *C : Edata.Chunks)
     EdataSec->addChunk(C);
 }
 
+void Writer::removeUnusedSections() {
+  // Remove sections that we can be sure won't get content, to avoid
+  // allocating space for their section headers.
+  auto IsUnused = [this](OutputSection *S) {
+    if (S == RelocSec)
+      return false; // This section is populated later.
+    // MergeChunks have zero size at this point, as their size is finalized
+    // later. Only remove sections that have no Chunks at all.
+    return S->Chunks.empty();
+  };
+  OutputSections.erase(
+      std::remove_if(OutputSections.begin(), OutputSections.end(), IsUnused),
+      OutputSections.end());
+}
+
 // The Windows loader doesn't seem to like empty sections,
 // so we remove them if any.
 void Writer::removeEmptySections() {
   auto IsEmpty = [](OutputSection *S) { return S->getVirtualSize() == 0; };
   OutputSections.erase(
       std::remove_if(OutputSections.begin(), OutputSections.end(), IsEmpty),
       OutputSections.end());
   uint32_t Idx = 1;
   for (OutputSection *Sec : OutputSections)
     Sec->SectionIndex = Idx++;
 }
 
 size_t Writer::addEntryToStringTable(StringRef Str) {

test/COFF/header-size.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %s -o %t.obj
+// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe
+// RUN: llvm-readobj -sections %t.exe | FileCheck %s
+    .globl main
+main:
+    ret
+
+// Check that the first section data comes at 512 bytes in the file.
+// If the size allocated for headers would include size for section
+// headers which aren't written, PointerToRawData would be 0x400 instead.
+// CHECK: PointerToRawData: 0x200