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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:760
+    for (const Fragment &F : Fragments)
+      PhdrFileOffset = std::min(PhdrFileOffset, F.Offset);
 
----------------
jhenderson wrote:
> Should this ignore SHT_NOBITS?
I am not sure why it should..
Anyways this patch should not change the logic of computations. It's intended to add an Offset check only.

(aside: this piece was changed after rebasing, now when we know that frarments are sorted, the code is simpler).


================
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
----------------
jhenderson wrote:
> 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)
Ok, thanks.


================
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 | \
----------------
jhenderson wrote:
> Perhaps worth also showing that ShOffset is ignored for these calculations (or not as the case may be)?
Done.


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

https://reviews.llvm.org/D78304