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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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`.


================
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) {
----------------
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?


================
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;
+
----------------
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)
  ...```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4086-4090
   return ((Sec.sh_type == ELF::SHT_NOBITS) ||
           (Sec.sh_offset > Phdr.p_offset &&
            Sec.sh_offset < Phdr.p_offset + Phdr.p_filesz)) &&
          (!(Sec.sh_flags & ELF::SHF_ALLOC) ||
           (Sec.sh_addr > Phdr.p_vaddr && Sec.sh_addr < Phdr.p_memsz));
----------------
Perhaps worth breaking this into a few separate booleans, as it gets quite hard to follow the logic.


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

https://reviews.llvm.org/D78709