[PATCH] D77652: [obj2yaml] - Fix the issue with dumping empty sections when dumping program headers.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:314-316
+  if (!SHdr.sh_size)
+    return SHdr.sh_addr >= Phdr.p_vaddr &&
+           SHdr.sh_addr <= Phdr.p_vaddr + Phdr.p_memsz;
----------------
jhenderson wrote:
> grimar wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > jhenderson wrote:
> > > > > grimar wrote:
> > > > > > jhenderson wrote:
> > > > > > > jhenderson wrote:
> > > > > > > > I think this will change the behaviour for a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address. I'd be inclined to say that the section should be listed in the segment, although I'm not sure if it actually matters either way.
> > > > > > > I'd find `Shdr.sh_size == 0` to be easier to read.
> > > > > > I think the right way would be to implement the missing functionality first (e.g. I'll post a patch for SHT_NOBITS soon, this diff is the preparation),
> > > > > > and then we can add support for tricky mailformed cases (if we want to support them) when we'll see the whole code.
> > > > > I'm not sure I follow what you mean by "missing functionality" here?
> > > > `isInSegment` is incomplete yet for valid cases: it doesn't work with SHT_NOBITS correctly and these particular lines will probably be reused because we need to check virtual addresses and not file offsets. So the code will change more. My point was that we need to implement everything we know we want and then can switch to supporting broken cases like you've mentioned.
> > > > 
> > > > "a (malformed) input with an empty section in the middle of a segment, where that section has a mismatching address" just doesn't sound like a important real case that we need to handle before the changes I've mentioned above or am I missing something?
> > > My feeling is that we should handle SHT_NOBITS first (straightforward), then think about the non-SHF_NOBITS sh_size=0 case (which may involve several alternative designs).
> > This patch fixes the issue we have (see Object/obj2yaml.test).
> Maybe I'm missing something, but can't we just change the if to something along the lines of `if (shdr.sh_size == 0 && (shdr.sh_type == SHT_NOBITS || shdr.sh_offset == phdr.p_offset || shdr.sh_offset == phdr.p_offset + phdr.p_filesz))`? It's not like it's that complicated and it would avoid changing the behaviour for these sections.
> but can't we just change the if to something along the lines of if (shdr.sh_size == 0 && (shdr.sh_type == SHT_NOBITS || shdr.sh_offset == phdr.p_offset || shdr.sh_offset == phdr.p_offset + phdr.p_filesz))?

This doesn't work for `obj2yaml.test` fixed by this patch I think.
It uses a valid precompiled object (`trivial-object-test.elf-avr`) which has the following layout:

```
Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        00000000 000074 000004 00  AX  0   0  2
  [ 2] .data             PROGBITS        00800060 000078 000000 00  WA  0   0  1
  [ 3] .shstrtab         STRTAB          00000000 000078 000027 00      0   0  1
  [ 4] .symtab           SYMTAB          00000000 0000a0 000110 10      5   5  4
  [ 5] .strtab           STRTAB          00000000 0001b0 0000a6 00      0   0  1

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000074 0x00000000 0x00000000 0x00004 0x00004 R E 0x2
  LOAD           0x000078 0x00800060 0x00000004 0x00000 0x00000 RW  0x1
```

The `.data` should not be included to the first PT_LOAD.

(I think first of all we want to focus on valid cases and seems we have to check VAs for that?).


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

https://reviews.llvm.org/D77652