[PATCH] D77652: [obj2yaml] - Fix the issue with dumping empty sections when dumping program headers.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:428
+## Notes: 1) '.empty.foo', '.empty.bar1' and '.bar' have the same file offset, but '.empty.foo'
+##           has VA that is outside of the segment, hence we should not include it into it.
+##        2) '.bar1' ends at 0x79, which is the starting file offset of both '.empty.bar2'
----------------
has a VA
include it in it


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:431
+##           and '.empty.zed'. We should only include '.empty.bar2', because the VA of the
+##           '.empty.zed' section is outside of segment's virtual space.
+# RUN: llvm-readelf -sections %t5 | FileCheck %s --check-prefix=ZERO-SIZE-MAPPING
----------------
outside of segment's -> outside the segment's


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:311
+
+  // An empty section can share the file offset with another section, so we need
+  // to do an additional check to confirm that such section is not outside of
----------------
I'm not sure this first part of the comment is relevant: it's not about whether two sections share the same offset, it's about what to do with empty sections on the edges of a program header. There could be no other sections present at the same offset, but we should still include/not include the section in that case, based on its address.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:312
+  // An empty section can share the file offset with another section, so we need
+  // to do an additional check to confirm that such section is not outside of
+  // the virtual address space of the segment.
----------------
such section -> such a section
outside of -> outside


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:314
+  // the virtual address space of the segment.
+  if (!SHdr.sh_size)
+    return SHdr.sh_addr >= Phdr.p_vaddr &&
----------------
jhenderson wrote:
> I think this will change the behaviour for a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address. I'd be inclined to say that the section should be listed in the segment, although I'm not sure if it actually matters either way.
I'd find `Shdr.sh_size == 0` to be easier to read.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:314-316
+  if (!SHdr.sh_size)
+    return SHdr.sh_addr >= Phdr.p_vaddr &&
+           SHdr.sh_addr <= Phdr.p_vaddr + Phdr.p_memsz;
----------------
I think this will change the behaviour for a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address. I'd be inclined to say that the section should be listed in the segment, although I'm not sure if it actually matters either way.


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

https://reviews.llvm.org/D77652