[PATCH] D75342: [obj2yaml] - Teach tool to dump program headers.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:179
 
+  // This holds the original section size and used for
+  // dumping program headers.
----------------
and is used


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:4
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readelf -a %t1 | FileCheck %s --check-prefix=SEGMENT-MAPPING
+# RUN: obj2yaml %t1 | FileCheck %s --check-prefix=YAML
----------------
--program-headers/--segments instead of -a


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:77
+
+--- !ELF
+FileHeader:
----------------
Some additional interesting test cases:

1) Standalone empty segments
2) Nested empty segments (at start/end/middle)
3) Empty sections in segments (at start/end/middle)
4) Misaligned segments?
5) Out-of-order PT_LOAD segments?
6) Segments with gaps in them (file size != size of all sections), where gap is at start/middle/end of segment.
7) Segments with memsize > file size.
8) Non-alloc sections within segments.

(Sorry if any of these are already covered)


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:169
+    ArrayRef<std::unique_ptr<ELFYAML::Chunk>> Chunks) {
+  auto PhdrsOrErr = Obj.program_headers();
+  if (!PhdrsOrErr)
----------------
Maybe this shouldn't be `auto`?


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:209
+      ELFYAML::Section* Sec = *It++;
+      uint64_t VA = Sec->Address.getValueOr(llvm::yaml::Hex64(0));
+      if (VA < PH.VAddr || (VA + Sec->OriginalSize > PH.VAddr + Phdr.p_memsz))
----------------
MaskRay wrote:
> I haven't carefully read through the code. Just raise an opinion.
> 
> For non-SHT_NOBITS sections, sh_offset/p_offset works better than sh_addr/p_vaddr.
> For SHT_NOBITS sections, sh_offset can be ignored. See D58426.
+1 to using sh_offset. That is what llvm-objcopy uses, IIRC, and I believe yaml2obj places things based on the file offset too.


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

https://reviews.llvm.org/D75342