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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:6
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-readelf --segments %t1 | FileCheck %s --check-prefix=SEGMENT-MAPPING
+
----------------
jhenderson wrote:
> Thinking about it, do we really need this part? It's really testing yaml2obj and/or llvm-readelf, not obj2yaml.
I did it for documentation purposes: (1) I find it very convenient for reading and maintaining this test: with it we can check and compare obj2yaml's output with "expected" version easily. (2) This also might allow us to refer and explain (in comments) any differences we might have in outputs.
I'd really would like to keep it because of (1) if there are no strong objections.



================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:157
+    Align: 0x1
+## Show we can dump a standalone empty segment.
+  - Type:  PT_LOAD
----------------
jhenderson wrote:
> Should this be moved to Part II?
I am not sure why. This tests we can have an empty segment somewhere between other ones.
Do you want me to add an additional test to Part II that will have an object with only one empty segment?


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:290
+    Flags: [ PF_W, PF_R ]
+    Sections: []
+    VAddr: 0x1100
----------------
jhenderson wrote:
> It might be clearer to explicitly specify .empty.tls.end here?
> 
> Also, this appears to be missing a PT_TLS at the start of the segment, i.e. covering .empty.tls.start?
Done.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:358
+
+## Test we do not include a non-allocatable section to a segment.
+# RUN: yaml2obj --docnum=4 %s -o %t4
----------------
jhenderson wrote:
> in a segment.
> 
> I disagree with this behaviour. A segment doesn't not have to contain only SHF_ALLOC sections (it is not mandated by the ELF gABI, and my organisation at least has linker scripts like that for segments other than PT_LOAD). If a segment's file offset + file size implies that it covers a non-alloc section, it should include it, no different to an alloc section.
Done.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:174-175
+template <class ELFT>
+Error ELFDumper<ELFT>::dumpProgramHeaders(
+    std::vector<ELFYAML::ProgramHeader> &Phdrs,
+    ArrayRef<std::unique_ptr<ELFYAML::Chunk>> Chunks) {
----------------
jhenderson wrote:
> Could this return an `Expected<std::vector<ELFYAML::PogramHeader>>`? That would be cleaner than passing in something to be filled.
Done. I think I did it to be consistent with `dumpSymbols`, but returning `Expected<std::vector<ELFYAML::ProgramHeader>>` is better indeed,


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:192-193
+    // Here we match sections with segments.
+    // It is not possible to have a non-Section chunk, because
+    // obj2yaml does not create Fill chunks.
+    for (const std::unique_ptr<ELFYAML::Chunk> &C : Chunks) {
----------------
jhenderson wrote:
> Maybe add a TODO here to add support for Fills?
I avoided any additional details as I am not sure it is clear to me how exactly we are going to support them.
(e.g. for example, I guess we might want to have a command line option, as I doubt we always want to dump a data between sections).
Just having  something like "TODO: add a support for Fill chunks" is probably not very useful?




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

https://reviews.llvm.org/D75342