[PATCH] D78005: [yaml2obj] - Reimplement how tool calculates memory sizes of segments.

[George Rimar via Phabricator via llvm-commits]

grimar marked 3 inline comments as done.
grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:33-34
+## This segment includes sections 1 to 4 + the .filler chunk.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000200 0x000220
+# OFFSET-NEXT:   LOAD 0x000158 0x0000000200000158 0x0000000200000158 0x0000a8 0x0000c8
+## This segment includes the first two sections.
----------------
jhenderson wrote:
> FileSize here looks like it should be 0x220, matching the memory size (which looks to be correct). It looks like the .nobits2 section is not being allocated file space, even though there's something non-nobits appearing after it. Please correct my understanding if I've misunderstood something. Same problem with the OFFSET case (off by 0x20 in file size).
> It looks like the .nobits2 section is not being allocated file space, even though there's something non-nobits appearing after it. 

Yes, yaml2obj does not allocate a file space in such case. In fact the following piece is the all we do for SHT_NOBITS:

```
    } else if (auto S = dyn_cast<ELFYAML::NoBitsSection>(Sec)) {
      // SHT_NOBITS sections do not have any content to write.
      SHeader.sh_entsize = 0;
      SHeader.sh_size = S->Size;
    }
```

I.e. it never allocates a file space for SHT_NOBITS. Seems to be the root of the problem.

So, should we teach yaml2obj to allocate a file space for a SHT_NOBITS section if there is any no-nobits chunk after it?
How does this behavior sound for you?


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:35
+# OFFSET-NEXT:   LOAD 0x000158 0x0000000200000158 0x0000000200000158 0x0000a8 0x0000c8
+## This segment includes the first two sections.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x00015a 0x00015a
----------------
jhenderson wrote:
> I was slightly surprised to not see a case for sections 1 to 4 or 1 to 3, but I assume that's because the 1 to 6 and 1 to 5 are essentially equivalent?
Honestly I do not remember why I did not include these initially. but I guess I tried to avoid having similar cases. Though now I think this test would benefit in readability from having all possibile cases. I am inclined to add them.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:137
+    Size:   0x1D0
+    Offset: 0x220
+  - Name:  .nobits4
----------------
jhenderson wrote:
> Am I right in thinking that this is unnecessary now? The fill chunk should cause this section to be placed after it, and if my counting is correct, that'll result in it appearing at 0x220 naturally.
In addition to my comment above, and to clarify: so 0x220 naturally could happen here if yaml2obj allocated a file space for `.nobits2`.
Then I would not need to use "Offset". 


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

https://reviews.llvm.org/D78005