[PATCH] D92098: [obj2yaml] - Don't crash when dumping an object with no sections.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/eshnum.yaml:15-20
+# CHECK:      --- !ELF
+# CHECK-NEXT: FileHeader:
+# CHECK-NEXT:   Class: ELFCLASS64
+# CHECK-NEXT:   Data:  ELFDATA2LSB
+# CHECK-NEXT:   Type:  ET_REL
+# CHECK-NEXT: ...
----------------
jhenderson wrote:
> If you feed this output of obj2yaml back into yaml2obj, the final object doesn't really reflect what you had originally - it generates three section headers (.strtab and .shstrtab, plus the null one). Seems like we need to fix something, but not entirely sure what.
Ideally we might want to produce the same output as YAML input I think.
At the same time this is a corner case that I am not sure is worth to support.
I've tried to expain in the comment below.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:238
                                ArrayRef<ELFYAML::ProgramHeader> Phdrs,
                                std::vector<std::unique_ptr<ELFYAML::Chunk>> &V,
                                ArrayRef<typename ELFT::Shdr> S) {
----------------
jhenderson wrote:
> grimar wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > I take it `V` doesn't normally contain a chunk for the null (index 0) section header?
> > > `V` contains all sections that are dumped from an object. The `SHT_NULL` section is
> > > normally present, it dumped as a regular section, and so `V` usually contains it. This is assumed below:
> > > 
> > > ```
> > >   for (const std::unique_ptr<ELFYAML::Chunk> &C :
> > >        makeArrayRef(V).drop_front()) {
> > > ```
> > > 
> > > In the case revealed by this patch we have no section header and no sections are dumped,
> > > that's why `V` is empty.
> > "we have no section header" -> "we have 0 sections"
> You've lost me a little. In the case when e_shnum and sh_size of section table index 0 are both 0, obj2yaml doesn't populate `V` with anything? So there is a section header table, it's just that no sections are read, because we say there are none. Is that right?
> You've lost me a little. In the case when e_shnum and sh_size of section table index 0 are both 0, obj2yaml doesn't populate V with anything?

Yes.

> So there is a section header table, it's just that no sections are read, because we say there are none. Is that right?

Yes.

The responsible code that returns us the sections list is:


```
template <class ELFT>
Expected<typename ELFT::ShdrRange> ELFFile<ELFT>::sections() const {
  const uintX_t SectionTableOffset = getHeader().e_shoff;
  if (SectionTableOffset == 0)
    return ArrayRef<Elf_Shdr>();

....

  const Elf_Shdr *First =
      reinterpret_cast<const Elf_Shdr *>(base() + SectionTableOffset);

  uintX_t NumSections = getHeader().e_shnum;
  if (NumSections == 0)
    NumSections = First->sh_size;

....
  return makeArrayRef(First, NumSections);
}
```

When `e_shoff != 0` and `e_shnum==0`, it reads the number of sections from ` First->sh_size` (which is 0) and returns us an empty array.
This is why no chunks are dumped.

To be able to support the case of this patch properly I think we should check the `e_shnum` manually. When it is 0, and the `e_shoff != 0`,
then we should dump the first section header table entry (manually). After that we should emit the `EShNum = 0` key to preserve `e_shnum`.
With all of these steps done, the output of the test case will be the same as input.

But do we want all this complexity to support this particular corner case? I believe having all this conditions in the real life is unrealistic. I've only tried to fix the crash I've found accidentally.


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

https://reviews.llvm.org/D92098