[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: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) {
----------------
jhenderson wrote:
> 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.
This and other conditions seems to be based on the GNU readelf code.
They are either similar or very close. See:

https://github.com/bminor/binutils-gdb/blob/master/binutils/readelf.c#L5399
https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L323
https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L309

Perhaps, it worth keeping the `checkTLSSections` because it implements first conditions
of the `ELF_SECTION_IN_SEGMENT_1` (https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L323)
(that are related to TLS) what makes easier to compare the code (since we are trying to match GNU's behavior).

We have `checkTLSSections`, `checkOffsets`, `checkVMA` and `checkPTDynamic` currently.
Seems they just implement the checks mentioned. I.e. we have a split of `ELF_SECTION_IN_SEGMENT_1` here.


================
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)
----------------
jhenderson wrote:
> 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?
Yes, it seems to be true:

```
   /* Any section besides one of type SHT_NOBITS must have file		\
      offsets within the segment.  */					\
   && ((sec_hdr)->sh_type == SHT_NOBITS					\
       || ((bfd_vma) (sec_hdr)->sh_offset >= (segment)->p_offset	\
	   && (!(strict)						\
	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\
		   <= (segment)->p_filesz - 1))				\
	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\
		+ ELF_SECTION_SIZE(sec_hdr, segment))			\
	       <= (segment)->p_filesz)))
```

https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L345


================
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:
> 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.




================
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.
----------------
jhenderson wrote:
> 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.
> I wonder how much of this actually happens in GNU readelf in practice, and how much was incidental for other reasons? 

It is what they implement explicitly:
https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L366

> I might even consider some of the conditions that GNU readelf follows to be non-sensical and just ignore them

Maybe. We should land this and add tests for the current behavior first though I think.


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

https://reviews.llvm.org/D78709