[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:4022
 template <class ELFT>
-bool GNUStyle<ELFT>::checkTLSSections(const Elf_Phdr &Phdr,
-                                      const Elf_Shdr &Sec) {
-  return (((Sec.sh_flags & ELF::SHF_TLS) &&
-           ((Phdr.p_type == ELF::PT_TLS) || (Phdr.p_type == ELF::PT_LOAD) ||
-            (Phdr.p_type == ELF::PT_GNU_RELRO))) ||
-          (!(Sec.sh_flags & ELF::SHF_TLS) && Phdr.p_type != ELF::PT_TLS));
+static bool checkTLSSections(const typename ELFT::Phdr &Phdr,
+                             const typename ELFT::Shdr &Sec) {
----------------
Unrelated to this commit, but I'm not at all convinced by this function.

Firstly, I wonder if this function really should be specifically abpout TLS segments, or if there's some equivalent behaviour for all segment types + NOBITS sections? For example, what if there were a .bss.rel.ro segment at the end of the PT_GNU_RELRO segment? Should it appear in the parent data segment or not (I'm guessing not)?

Example linker script snippet:
```
.data.rel.ro : { *(.data.rel.ro .data.rel.ro.*) } : ph_relro : ph_data
.bss.rel.ro : { *(.bss.rel.ro .bss.rel.ro.*) } : ph_relro
.data : { *(.data .data.*) } : ph_data
```

Semantically, it doesn't sound much different to me to TLS nobits stuff.

Secondly, why should we omit non TLS sections from the TLS segment printing and why should we omit TLS sections from non-TLS/LOAD/RELRO segments? It seems like the logic is unnecessarily complicated here.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4025
+  if (Sec.sh_flags & ELF::SHF_TLS) {
+    // The .tbss must only be shown in the PT_TLS segment.
+    if (Sec.sh_type == ELF::SHT_NOBITS)
----------------
I'd get rid of "The"


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4042
+                         const typename ELFT::Shdr &Sec) {
+  // Only non-SHT_NOBITS must have its offset inside the segment.
   if (Sec.sh_type == ELF::SHT_NOBITS)
----------------
I'd rephrase this comment to "SHT_NOBITS sections don't need to have an offset inside the segment."

Aside: is that true compared to what GNU readelf does?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4049
+
+  // Only non-zero section can be at the end of a segment.
+  if (Sec.sh_size == 0)
----------------
non-zero section -> non-empty sections


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4051
+  if (Sec.sh_size == 0)
+    return (Sec.sh_offset + 1 <= Phdr.p_offset + Phdr.p_filesz);
+  return Sec.sh_offset +  Sec.sh_size <= Phdr.p_filesz + Phdr.p_offset;
----------------
It might be good for the RHS of this expression to match the order in the below equivalent line.


================
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;
+}
----------------
Nit: too many spaces before Sec.sh_size. Did you clang-format?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4071
+
+  // Only non-zero section can be at the end of a segment.
+  if (SectionSize == 0)
----------------
non-zero section -> non-empty sections


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4074
+    return Sec.sh_addr + 1 <= Phdr.p_vaddr + Phdr.p_memsz;
+  return Sec.sh_addr + SectionSize <= Phdr.p_vaddr + Phdr.p_memsz;
 }
----------------
Might be worth using Sec.sh_size instead of SectionSize here?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4076
     return true;
-  // Is section within the phdr both based on offset and VMA ?
   return ((Sec.sh_type == ELF::SHT_NOBITS) ||
----------------
I think this comment made sense. Please reinstate it.


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


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4083
+
+  // No section with zero size must be at the start or at the end of PT_DYNAMIC.
   return ((Sec.sh_type == ELF::SHT_NOBITS) ||
----------------
This comment belongs with the above if, I think.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4159-4160
+
       // readelf additionally makes sure it does not print zero sized sections
       // at end of segments and for PT_DYNAMIC both start and end of section
       // .tbss must only be shown in PT_TLS section.
----------------
Related to my above comments re. TLS/Dynamic segments, I wonder how much of this actually happens in GNU readelf in practice, and how much was incidental for other reasons? Could you check if you are considering looking more at my other comments?

I might even consider some of the conditions that GNU readelf follows to be non-sensical and just ignore them, though that might be a bit controversial.


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

https://reviews.llvm.org/D78709