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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
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;
----------------
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).


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

https://reviews.llvm.org/D77652