[PATCH] D144009: [obj2yaml] Save offset for segments and size for PHDR

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Could you clarify why you only emit the sizes for the PHDR type? It's perfectly legitimate for other segment types to have a size that covers other areas too that aren't parts of sections (and therefore the size can't be derived purely from the section size), but IIRC, yaml2obj will assume the sizes match unless told otherwise. I suppose it might be necessary to emit Fill "sections" in the YAML to cover the gap in some cases to ensure section layout works correctly.



================
Comment at: llvm/test/tools/obj2yaml/ELF/program-headers.yaml:52
 # YAML-NEXT:    Align:    0x1000
+# YAML-NEXT:    Offset: 0x281
 # YAML-NEXT:  - Type:     PT_LOAD
----------------
Nit: here and below, all the other field values within a segment are aligned vertically with each other, but in most cases, the new Offset field isn't. Please fix.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:488-493
+    PH.Offset = Phdr.p_offset;
+
+    if (Phdr.p_type == ELF::PT_PHDR) {
+      PH.MemSize = Phdr.p_memsz;
+      PH.FileSize = Phdr.p_filesz;
+    }
----------------
I think we should be emitting these values in the output only if they can't be derived automatically from the existing layout, otherwise the YAML output will end up unnecessarily verbose. It's been a while since I've looked at the obj2yaml code though, so it's possible this happens elsewhere.


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

https://reviews.llvm.org/D144009