[PATCH] D95591: [yaml2obj/obj2yaml] - Implement program header table as a special Chunk.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:757
+            dyn_cast<ELFYAML::ProgramHeaderTable>(D.get())) {
+      PHT->Offset = alignToOffset(CBA, /*Align=*/1, PHT->Offset);
+      uint64_t Size = sizeof(Elf_Phdr) * PHT->Segments.size();
----------------
jhenderson wrote:
> My reading of the ELF gABI is that this will need to be aligned on a 4-byte/8-byte boundary dependent on ELF32/ELF64. We should do that for an implicit offset (but allow it if the offset is explicit):
> 
> > All data structures that the object file format defines follow the ``natural'' size and alignment guidelines for the relevant class. If necessary, data structures contain explicit padding to ensure 8-byte alignment for 8-byte objects, 4-byte alignment for 4-byte objects, to force structure sizes to a multiple of 4 or 8, and so forth. Data also have suitable alignment from the beginning of the file. Thus, for example, a structure containing an Elf32_Addr member will be aligned on a 4-byte boundary within the file. 
> 
> Speaking of which, this code looks quite different to the section header table code, which has two separate calls to `alignToOffset` dependent on whether the Offset has been specified, if I read it correctly. Why is that?
> Why is that?

Just because the code did not do the auto alignment. I wasn't sure if we want it as usually the program header
table is placed at the begining, where it is naturally aligned, though your reference to gABI makes sense to me. Also, the code is indeed better to be
consistent with the code for the section header table. So, I've fixed it.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:203-204
+
+## Check that we can place the program header table at an
+## arbitrary offset before other sections.
+
----------------
jhenderson wrote:
> What about an offset that isn't already aligned to 4/8?
Done.


================
Comment at: llvm/test/tools/yaml2obj/ELF/program-header.yaml:218-219
+
+## Check we can place the program header table somewhere in
+## the middle of the sections list.
+
----------------
jhenderson wrote:
> Should we have one for after all other chunks so that the total file size is based on the end of the last chunk? I don't know if that's really needed or not.
I've improved testing and added such a test case.


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

https://reviews.llvm.org/D95591