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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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;
----------------
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.)


https://reviews.llvm.org/D54495