[PATCH] D60785: [ELF] Align file offset for .bss if first section in a PT_LOAD

[James Clarke via Phabricator via llvm-commits]

jrtc27 marked 2 inline comments as done.
jrtc27 added inline comments.


================
Comment at: lld/ELF/Writer.cpp:2224
+    // monotonically increasing rather than setting to zero.
+    if (OS->Type == SHT_NOBITS)
+      return Off;
----------------
MaskRay wrote:
> MaskRay wrote:
> > What happens if you delete `if (OS->Type == SHT_NOBITS) return Off;` here?
> I think we probably just need to delete `if (OS->Type == SHT_NOBITS) return Off;` from the original code.
> 
> `.tss` in `tls-offset.s` is affected but that offset is not significant.
Then you potentially unnecessarily over-align a non-loaded nobits section in the file. It's not wrong, it just deviates more from the current output.


================
Comment at: lld/ELF/Writer.cpp:2239
+  // This is a .bss section that isn't first in its PT_LOAD; see above.
+  if (OS->Type == SHT_NOBITS)
+    return Off;
----------------
This might actually be wrong; consider `.bss` being followed (stupidly) by `.data` in the same segment. In that case, `.bss` will be filled with zeroes and thus have a non-zero filesz, but we need to make sure its file offset is still the same as its address modulo the page size (ie by using the standard formula below), otherwise it will appear shifted in the segment. I'll see if this is true in practice with another simple test case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60785/new/

https://reviews.llvm.org/D60785