[PATCH] D78304: [yaml2obj] - Program headers: and add an additional check for `Offset`

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:760
+    for (const Fragment &F : Fragments)
+      PhdrFileOffset = std::min(PhdrFileOffset, F.Offset);
 
----------------
Should this ignore SHT_NOBITS?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:764
+      if (!Fragments.empty() && *YamlPhdr.Offset > PhdrFileOffset)
+        reportError("the 'Offset' specified for the segment with index " +
+                    Twine(PhdrIdx) +
----------------
I think it's okay to slightly simplify to make the error message slightly less verbose:

"'Offset' for segment with index"


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:766
+                    Twine(PhdrIdx) +
+                    " must be less or equal than the minimal file offset of a "
+                    "section included into it (0x" +
----------------
I think this needs rewording:

" must be less than or equal to the minimum file offset of all included sections (0x123456)"


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:95
       - Section: .text
   # Program header with sections, invalid properties.
   - Type:     0x6abcdef0
----------------
This comment is stale and needs updating.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:111
+## Check that by default the p_offset field of a segment is set to the
+## offset of the section with the minimal section offset.
+# RUN: yaml2obj --docnum=2 %s -o %t2
----------------
minimal section offset -> minimum offset

(I would generally prefer minimum to minimal as minimal can also just mean "really small", so could occasionally be ambiguous)


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:141
+ProgramHeaders:
+  - Type:    PT_LOAD
+    Sections:
----------------
FWIW, I wouldn't indent PT_LOAD here or below. I don't consider the Sections block as important for spacing, becuase it doesn't have a single inline value.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:149
+
+## Check we can set the "Offset" value explicitly when it is less or equal to
+## the offset of a section in the segment.
----------------
I would change "when it is less or equal to" to "to be less than or equal to".

Also, this test case only shows one of "equal to" or "less than". It should probably test both cases.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:178
+
+## Check we report an error when the "Offset" value is larger than the offset of a section in the segment.
+# RUN: not yaml2obj --docnum=3 -DOFFSET=0x79 %s -o /dev/null 2>&1 | \
----------------
Perhaps worth also showing that ShOffset is ignored for these calculations (or not as the case may be)?


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

https://reviews.llvm.org/D78304