[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:180
 
+  // Hold the original section index. Used for dumping program headers.
+  unsigned OriginalSecNdx;
----------------
Hold -> Holds (!)

(For reference, third-person singular verbs usually have an 's' on the end, but not third-person plural - previously multiple variables were being referenced, so plural, but now there is only one)


================
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
+
----------------
grimar wrote:
> 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.
> 
No strong objection, but might be worth moving the comment to the llvm-readelf run line, along with a statement saying the check is here to make it clear what the layout should look like.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:23-32
+# SEGMENT-MAPPING-NEXT:   00     .hash .gnu.hash .dynsym .dynstr
+# SEGMENT-MAPPING-NEXT:   01     .foo .zed
+# SEGMENT-MAPPING-NEXT:   02     .foo .baz
+# SEGMENT-MAPPING-NEXT:   03     .dynamic .dynamic.tail
+# SEGMENT-MAPPING-NEXT:   04     .dynamic
+# SEGMENT-MAPPING-NEXT:   05     .dynamic
+# SEGMENT-MAPPING-NEXT:   06{{ *$}}
----------------
jhenderson wrote:
> You need {{$}} at the end of all of these lines, since there could be more segments after the last one in each of them.
On second thoughts, using --match-full-lines might be just as good. I don't mind either way.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:157
+    Align: 0x1
+## Show we can dump a standalone empty segment.
+  - Type:  PT_LOAD
----------------
grimar wrote:
> 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?
I don't think there's a need for that. I wouldn't expect that to be a special case.


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:173
+## Test we are able to dump duplicated segments.
+## We use a segment that is the same as previous for this.
+  - Type:  PT_LOAD
----------------
the same as the previous one for this


================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:373
+
+## Test we include non-allocatable sections to segments.
+# RUN: yaml2obj --docnum=4 %s -o %t4
----------------
to segments -> in segments


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:174-175
+                        const typename ELFT::Phdr &Phdr) {
+  if (Sec.Type == ELF::SHT_NULL)
+    return false;
+  return SHdr.sh_offset >= Phdr.p_offset &&
----------------
Do we need this special case? Do you have a test case that illustrates it?


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:226-231
+  // The contents of these sections are described by other parts of the YAML
+  // file. We only keep allocatable section because their positions and
+  // addresses are important, e.g. for creating program headers. Some sections,
+  // like .symtab or .strtab normally are not allocatable and do not have
+  // virtual addresses. We want to avoid noise in the YAML output and assume
+  // that they are placed at the end.
----------------
This comment refers to allocatable sections, which I don't think is quite right?


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

https://reviews.llvm.org/D75342