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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:795
+        const Fragment *Next =
+            (I + 1 < Fragments.size()) ? &Fragments[I + 1] : nullptr;
+
----------------
`Fragments.size()` -> `E`?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:798-799
+        // The distance between the current and the next section can
+        // be larger than the size of the current section when the
+        // alignment gap present.
+        // Another possible case is when we have 2 or more trailing SHT_NOBITS
----------------
when the alignment gap present -> when there is a gap due to alignment


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:800-802
+        // Another possible case is when we have 2 or more trailing SHT_NOBITS
+        // sections with the same offset. In this case we should take all of
+        // their sizes into account.
----------------
I'd change this whole paragraph into a continuation of the previous sentence:

", or when we have 2 or more trailing SHT_NOBITS sections with the same offset. In the latter case, we should take all trailing SHT_NOBITS section sizes into account."

It reads better to me, but also helps fix one or two ambiguities.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:1
-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj -l %t | FileCheck %s
+## Here we test that yaml2obj is able to handle SHT_NOBITS sections properly.
 
----------------
sections -> sections in segments


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:16
+# COMMON-NEXT:   [ 1] .nobits1   NOBITS   0000000200000000 000158 000010
+# COMMON-NEXT:   [ 2] .progbits1 PROGBITS 0000000200000010 000158 000001
+# COMMON-NEXT:   [ 3] .progbits2 PROGBITS 0000000200000020 000160 000010
----------------
I was initially concerned that .progbits1 and .nobits1 share the same file offset. In such a case, the loader would not allocate the bytes for .nobits1 in the right place for any segment where it wasn't the only section in it. However, this is yaml2obj, not the linker, so it's probably fine to leave as-is, if there's some way of adding padding between the two sections without changing the address of .progbits1.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:18-20
+# COMMON-NEXT:   [ 4] .nobits2   NOBITS   0000000200000030 000170 000020
+## The .filler chunk (size = 0x90) is placed here.
+# COMMON-NEXT:   [ 5] .nobits3   NOBITS   00000002000000e0 000200 0001d0
----------------
Same issue here. I'd naturally expect .nobits3 to have an offset of 0x220, since the filler bytes (which might need to be set to some non-zero value), need to appear after the bytes for .nobits2, rather than sharing the same location.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:26-27
+## This segment includes sections 1 to 6.
+# NOOFFSET-NEXT: LOAD 0x000000 0x0000000200000000 0x0000000200000000 0x000200 0x000500
+# OFFSET-NEXT:   LOAD 0x000158 0x0000000200000000 0x0000000200000000 0x0000a8 0x0003a8
+## This segment includes sections 1 to 5.
----------------
Something's not right here. Why is the memory size 0x500/0x3a8?

The highest end address of any section in the segment is 0x2000003b0, according to the section dump above, which would mean a size of 0x508/0x3b0 depending on which offset is used as the start point.

This might be because you've specified section addresses which conflict with the program header address, causing confusion - the program header with Offset 0 can't have the same VAddr as its first section, since the first section doesn't start at offset 0.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:51
+  - Type:  PT_LOAD
+    Flags: [ PF_W, PF_R ]
     Sections:
----------------
Unnecessary noise. Flags aren't needed for this test.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:96
+    Offset: [[OFFSET]]
+## A segment has only a SHT_NOBITS section.
+  - Type:  PT_LOAD
----------------
Either "This segment has only" or "A segment with only"


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-nobits.yaml:132
+    Size:    0x90
+## We have two SHT_NOBITS section at the end.
+## It is important to have at least two of them because we had a bug
----------------
section -> sections


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

https://reviews.llvm.org/D78005