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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with nit.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:760
+    for (const Fragment &F : Fragments)
+      PhdrFileOffset = std::min(PhdrFileOffset, F.Offset);
 
----------------
grimar wrote:
> 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).
> 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).

Not sure where I was coming form with that comment before, but it's worth noting that SHT_NOBITS offsets in theory should be meaningless. I don't see a strong reason to ignore them though.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml:184
+# RUN: not yaml2obj --docnum=3 -DOFFSET=0x79 %s -o /dev/null 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=INVALID-OFFSET
+
----------------
Nit: please add an extra space before `FileCheck`.


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

https://reviews.llvm.org/D78304