[PATCH] D78709: [llvm-readobj] - Simplify conditions used for printing segment mappings. NFCI.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4052
+    return (Sec.sh_offset + 1 <= Phdr.p_offset + Phdr.p_filesz);
+  return Sec.sh_offset +  Sec.sh_size <= Phdr.p_filesz + Phdr.p_offset;
+}
----------------
jhenderson wrote:
> jhenderson wrote:
> > Nit: too many spaces before Sec.sh_size. Did you clang-format?
> This hasn't been addressed. There are still two spaces between `+` and `Sec.sh_size`.
Sorry. Fixed.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4078
 template <class ELFT>
-bool GNUStyle<ELFT>::checkPTDynamic(const Elf_Phdr &Phdr, const Elf_Shdr &Sec) {
-  if (Phdr.p_type != ELF::PT_DYNAMIC || Sec.sh_size != 0 || Phdr.p_memsz == 0)
+static bool checkPTDynamic(const typename ELFT::Phdr &Phdr,
+                           const typename ELFT::Shdr &Sec) {
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Similar to the TLS one, this function seems bonkers. Why does it exist at all? Why is the dynamic segment special compared to other segments?
> > I believe it is based on the GNU readelf logic:
> > https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L366
> > 
> > It says "No zero size sections at start or end of PT_DYNAMIC nor PT_NOTE.".
> > Here only the `PT_DYNAMIC` is handled, seems we want to handle `PT_NOTE` too.
> > 
> > 
> Not dug into it, but I guess the intent here is to avoid having non SHT_DYNAMIC/SHT_NOTE sections appear in PT_DYNAMIC/PT_NOTE segments, which does make some sense. Perhaps worth writing/expanding a comment to explain this? What do you think?
> but I guess the intent here is to avoid having non SHT_DYNAMIC/SHT_NOTE sections appear in PT_DYNAMIC/PT_NOTE segment. Perhaps worth writing/expanding a comment to explain this? What do you think?

The code does not check the section type, i.e. if it is "SHT_DYNAMIC" or not. We might have (in a mailformed object) 2 SHT_DYNAMIC sections:
first is empty, second is not and will never see the first one in the output with the current code.

I guess the best comment could be: "This is just how GNU does, we follow". We can return to it later when will add support for "PT_NOTE" here.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4069
+  // .tbss is special, it only has memory in PT_TLS and has NOBITS properties.
+  uint64_t SectionSize = (IsTbss && Phdr.p_type != ELF::PT_TLS) ? 0 : Sec.sh_size;
+
----------------
jhenderson wrote:
> I wonder if it now makes sense to make this `IsEmpty` instead? In other words:
> 
> ```bool IsEmpty = (IsTbss && Phdr.p_type != ELF::PT_TLS) || Sec.sh_size == 0;
> if (IsEmpty)
>   ...```
> 
> Or even:
> 
> ```bool IsTbssInNonTLS = IsTbss && Phdr.p_type != ELF::PT_TLS;
> if (Sec.sh_size == 0 || IsTbssInNonTLS)
>   ...```
OK.


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

https://reviews.llvm.org/D78709