[PATCH] D80629: [yaml2obj] - Allocate the file space for SHT_NOBITS sections in some cases.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:872
+  for (const ELFYAML::ProgramHeader &PH : Phdrs)
+    for (ELFYAML::Chunk *C : getChunksAfter(PH, S.Name))
+      if (isa<ELFYAML::Fill>(C) ||
----------------
grimar wrote:
> grimar wrote:
> > MaskRay wrote:
> > > Inlining getChunksAfter can avoid the cost constructing a vector.
> > I think the cost was very low: there is no much output sections usually, and the case when there are non-nobits sections after a nobit section is also probably rare. But it made the final code to be much shorter.
> Perhaps my comment is not very clear: **inlining ** made the code shorter.
Inlining does seem shorter:)

Another thing about the previous approach: the worst case time complexity did not change but allocating vector worsened the expected time complexity (SHT_NOBITS is usually followed by a non-SHT_NOBITS) I know that in some places we have quadratic complexity but introducing a new one when it can be easily avoided is not very great.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:980
+  // When a nobits section is followed by a non-nobits section or fill in the
+  // same segment, we allocate the file space for it.
+  if (shouldAllocateFileSpace(Doc.ProgramHeaders, S))
----------------
This comment should mention that this behavior matches linkers.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:115
+# MIDDLE: Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz
+# MIDDLE: LOAD 0x000078 0x0000000000000000 0x0000000000000000 0x000011 0x000111
+
----------------
Is `-NEXT:` applicable?


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

https://reviews.llvm.org/D80629