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

[George Rimar via Phabricator via llvm-commits]

grimar 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) ||
----------------
MaskRay wrote:
> 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.
Not sure I understood this point. A `SHT_NOBITS` section is often followed by a **non-allocatable** `non-SHT_NOBITS` (e.g. `.comment` might follow `.bss`), but the previous version returned `getChunksAfter` for a given program header. We usually include only allocatable sections to a `PT_LOAD`s (which might contain a `SHT_NOBITS` section normally), so in 99% cases the returned vector was just empty I believe.

So. I believe that shorter code was the only real (but important one) benefit here.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:106
+# RUN: yaml2obj --docnum=2 %s -o %t4
+# RUN: llvm-readelf --sections -l %t4 | FileCheck %s --check-prefix=MIDDLE
+
----------------
jhenderson wrote:
> Here and above, it's slightly offputting seeing a mixture of short and long options. I'd either go one or the other (i.e. `-S -l` or `--sections --segments`).
I've used the long version.


================
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
+
----------------
MaskRay wrote:
> Is `-NEXT:` applicable?
Yes. Done.


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

https://reviews.llvm.org/D80629