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

Thu Mar 19 02:40:15 PDT 2020

jhenderson added a comment.

In D75342#1903392 <https://reviews.llvm.org/D75342#1903392>, @MaskRay wrote:

> @jhenderson listed many good test cases. It requires some work. I've left some notes.
>
> > 1. Standalone empty segments
>
> GNU ld can create a p_memsz=p_filesz=0 PT_LOAD. lld doesn't.

Not all segments have to be nested inside a PT_LOAD. We have several OS-specific segments in our downstream port that are not part of the memory image, but must appear in the link output, as they are used by tools other than the loader in our ecosystem, so they are not nested in PT_LOADs in our linker scripts. I imagine there are other cases too.

In D75342#1919197 <https://reviews.llvm.org/D75342#1919197>, @grimar wrote:

> - Reimplemented, added test cases.
>
> In D75342#1903392 <https://reviews.llvm.org/D75342#1903392>, @MaskRay wrote:
>
> > @jhenderson listed many good test cases. It requires some work. I've left some notes.
>
>
> Thank you guys for these suggestions and comments, I used them in the update.
>
> I've added tests for all cases listed except the next two.
>
> >> Segments with gaps in them (file size != size of all sections), where gap is at start/middle/end of segment.
> > 
> > Type: Filler
>
> I think we might want having a separate test for this. Also it needs more code I think.
>
> >> Segments with memsize > file size.
> > 
> > Needs non-empty SHT_NOBITS.
>
> I'd like to support SHT_NOBITS case separatelly too for the same reasons.
>
> How does it sound?

Those both sound reasonable to me.

================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:179

+  // This holds the original section size and used for
+  // dumping program headers.
----------------
jhenderson wrote:
> and is used
holds -> hold

================
Comment at: llvm/test/Object/obj2yaml.test:670
+# ELF-AVR-NEXT:     - Section: .text
+# ELF-AVR-NEXT:     - Section: .data
+# ELF-AVR-NEXT:    Align: 0x0000000000000002
----------------
MaskRay wrote:
> grimar wrote:
> > `.data` section size == 0, I wonder if it is OK we dump it?
> > 
> > Can we ignore empty sections at the end of segment? I.e do what D74755 removes:
> > 
> > ```
> > // If a section is empty it should be treated like it has a size of 1. This is
> > // to clarify the case when an empty section lies on a boundary between two
> > // segments and ensures that the section "belongs" to the second segment and
> > // not the first.
> > uint64_t SecSize = Sec.Size ? Sec.Size : 1;
> > ```
> I vote for removing `uint64_t SecSize = Sec.Size ? Sec.Size : 1;`
I think for obj2yaml, it's fine to do this, since obj2yaml should just recreate the layout, and the sections in the segment just help to configure the program headers.

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:3
+
+## Part I. Base check. All simple cases that looks OK as a part of a single large test live here.
+
----------------
looks -> look

================
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
+
----------------
Thinking about it, do we really need this part? It's really testing yaml2obj and/or llvm-readelf, not obj2yaml.

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:8
+
+## Show the layout of the object before we dumped it using obj2yaml.
+
----------------
dumped -> dump

================
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{{ *$}}
----------------
You need {{$}} at the end of all of these lines, since there could be more segments after the last one in each of them.

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:132
+    Align: 0x1000
+## Show we can create a writeable PT_LOAD segment and put an arbitrary section into.
+## Here we test both regular (SHT_PROGBITS) and a special section (SHT_DYNAMIC).
----------------
into it.

(same throughout)

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:141
+    Align: 0x1000
+## Show we can create a dynamic segment and put a section into.
+  - Type:  PT_DYNAMIC
----------------
Might be worth saying "a nested dynamic segment" to highlight that being nested is also an interesting property.

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:157
+    Align: 0x1
+## Show we can dump a standalone empty segment.
+  - Type:  PT_LOAD
----------------
Should this be moved to Part II?

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:172
+    Align: 0x1
+## This must be the same as previous. Test we and are able
+## to dump duplicated segments.
----------------
This comment needs to explain "why" it must be the same as the **previous** segment. Otherwise, I think the first sentence is unnecessary.

"we and are" -> "we are"

================
Comment at: llvm/test/tools/obj2yaml/program-headers.yaml:290
+    Flags: [ PF_W, PF_R ]
+    Sections: []
+    VAddr: 0x1100
----------------
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?

================
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
----------------
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.

================
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) {
----------------
Could this return an `Expected<std::vector<ELFYAML::PogramHeader>>`? That would be cleaner than passing in something to be filled.

================
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) {
----------------
Maybe add a TODO here to add support for Fills?

================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:196
+      ELFYAML::Section &S = cast<ELFYAML::Section>(*C);
+      if ((S.Flags.getValueOr(ELFYAML::ELF_SHF(0)) & ELF::SHF_ALLOC) &&
+          isInSegment<ELFT>(S, Phdr))
----------------
As noted in the test, this SHF_ALLOC check is incorrect. It doesn't faithfully recreate what was in the object, and doesn't allow for non-loadable segments. Please remove it.

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

https://reviews.llvm.org/D75342