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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Writer.cpp:888-892
+  auto IsEmpty = [](OutputSection *S) {
+    for (Chunk *C : S->Chunks)
+      if (C->getSize() > 0)
+        return false;
+    return true;
----------------
ruiu wrote:
> mstorsjo wrote:
> > ruiu wrote:
> > > mstorsjo wrote:
> > > > ruiu wrote:
> > > > > 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?
> > > > `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?
> > > 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.
> > 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.
> 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.)
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.


https://reviews.llvm.org/D54495