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

[grimar]

Summary

This patch is a NFC refactoring.

Currently the logic is overcomplicated, contains dead conditions and is very hard to read.
This patch performs a very straightforward simplification. Probably it can be
simplified and improved more, but we need to land test cases documenting/testing
all the current functionality first.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

4154

This now lives in checkTLSSections

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

4022–4036

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.

4025

I'd get rid of "The"

4042

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?

4049

non-zero section -> non-empty sections

4051

It might be good for the RHS of this expression to match the order in the below equivalent line.

4052

Nit: too many spaces before Sec.sh_size. Did you clang-format?

4071

non-zero section -> non-empty sections

4074

Might be worth using Sec.sh_size instead of SectionSize here?

4076

I think this comment made sense. Please reinstate it.

4077–4079

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?

4081

This comment belongs with the above if, I think.

4161–4162

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.

[grimar]

I've read all the comments and I have no answers for many of them atm.
(I am at the begining of investigating/understanding the logic we have. It looks messy.
This patch is the first step that reveals the conditions we have to document/test them).

Currently I am working on a patch to document the existent behavior.
I am going to investigate the conditions, GNU readelf behavior and probably it's code.
Hopefully it will help to understand and answer the questions that were asked.
It will take some time, I'll return here after posting it.

After landing this and the patch mentioned we will be able to clean-up/improve the logic.
(I am in fear of making any non-trivial changes/cleanups while we have no solid test coverage).

llvm/tools/llvm-readobj/ELFDumper.cpp

4081

I think no. It seems to be about

"(Sec.sh_offset > Phdr.p_offset && Sec.sh_offset < Phdr.p_offset + Phdr.p_filesz)"

One of conditions to get here is to have "Sec.sh_size == 0".

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

4081

This comment is to do with sections with zero size and PT_DYNAMIC, so the comment belongs with the zero size/PT_DYNAMIC check surely?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

4081

See:

if (Phdr.p_type != ELF::PT_DYNAMIC || Phdr.p_memsz == 0 || Sec.sh_size != 0) above means that
we can get here only when a segment is PT_DYNAMIC and section has zero size.

The comment says "No section with zero size must be at the start or at the end of PT_DYNAMIC"

It is related to the following condition:

(Sec.sh_offset > Phdr.p_offset && Sec.sh_offset < Phdr.p_offset + Phdr.p_filesz)

Note that > and < are used here instead of >= and <=. I.e. it checks that the empty section is not at the
start or at the end. Just like comment says.

Right?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

4081

I think I'll be able to improve comments in this method to avoid confusion.

[grimar]

• Addressed review comments.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

4022–4036

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.

4042

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

4077–4079

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.

4161–4162

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.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

4052

This hasn't been addressed. There are still two spaces between + and Sec.sh_size.

4069

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)
  ...

4077–4079

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?

4080–4090

Perhaps worth breaking this into a few separate booleans, as it gets quite hard to follow the logic.

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

4052

Sorry. Fixed.

4069

OK.

4077–4079

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.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

4077–4079

I actually am coming around to GNU's process for this case here. Yes, it doesn't check the section type, but the chances of an empty dynamic section are minimal anyway. Maybe we could special case SHT_DYNAMIC though at a future point. If we did that, I'd want the SHT_DYNAMIC exception for PT_DYNAMIC only and SHT_NOTE for PT_NOTE in a similar manner.

4085

Shouldn't this be an || to match the old behaviour?

[grimar]

• Addressed review comment.

llvm/tools/llvm-readobj/ELFDumper.cpp

4085

Right.

[jhenderson]

LGTM.

llvm/tools/llvm-readobj/ELFDumper.cpp

4085

I'm nervous that (presumably) no test failed because of this mistake, but I guess that the situation isn't any worse than before.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

4085

Me too. And this is what I've mentioned in the description: "Probably it can be
simplified and improved more, but we need to land test cases documenting/testing
all the current functionality first."

[grimar]

Was committed as e4ba3ff35942da35c65315d1a590c2709dc99f07, phab did not close it for some reason.

[MaskRay]

https://discord.gg/xS7Z362 #infrastructure

In D78709#2015201, @grimar wrote:

Was committed as e4ba3ff35942da35c65315d1a590c2709dc99f07, phab did not close it for some reason.

This was discussed in https://discord.gg/xS7Z362 #infrastructure. I noticed a "disk is full" issue for reviews.llvm.org about 17 hours ago. It seems not everything is restored.

[grimar]

In D78709#2015662, @MaskRay wrote:

https://discord.gg/xS7Z362 #infrastructure

In D78709#2015201, @grimar wrote:

Was committed as e4ba3ff35942da35c65315d1a590c2709dc99f07, phab did not close it for some reason.

This was discussed in https://discord.gg/xS7Z362 #infrastructure. I noticed a "disk is full" issue for reviews.llvm.org about 17 hours ago. It seems not everything is restored.

I also had no mails from mailing lists (llvm-comits, llvm-dev) for about ~2.5 days atm. Only have mails for reviews where I am a subscriber/author/reviewer.
Do you know anything about it? I just wonder if it is our local mail filter issue again or something more global.

[MaskRay]

In D78709#2016171, @grimar wrote:

In D78709#2015662, @MaskRay wrote:

https://discord.gg/xS7Z362 #infrastructure

In D78709#2015201, @grimar wrote:

Was committed as e4ba3ff35942da35c65315d1a590c2709dc99f07, phab did not close it for some reason.

This was discussed in https://discord.gg/xS7Z362 #infrastructure. I noticed a "disk is full" issue for reviews.llvm.org about 17 hours ago. It seems not everything is restored.

I also had no mails from mailing lists (llvm-comits, llvm-dev) for about ~2.5 days atm. Only have mails for reviews where I am a subscriber/author/reviewer.
Do you know anything about it? I just wonder if it is our local mail filter issue again or something more global.

This was a server side issue. The server had been down for some time http://lists.llvm.org/pipermail/llvm-dev/2020-May/141343.html
After it came back, I've observed for at least 8 hours some daemon was not running: (1) Differential Revisions were not closed automatically (2) Harbomaster (pre-merge testing) was not working (3) "Recent Activity" did not update. I did not subscribe llvm-commits. llvm-dev is fine (so people could discuss that reviews.llvm.org was down...)

[grimar]

! In D78709#2016336, @MaskRay wrote:
This was a server side issue. The server had been down for some time http://lists.llvm.org/pipermail/llvm-dev/2020-May/141343.html
After it came back, I've observed for at least 8 hours some daemon was not running: (1) Differential Revisions were not closed automatically (2) Harbomaster (pre-merge testing) was not working (3) "Recent Activity" did not update. I did not subscribe llvm-commits. llvm-dev is fine (so people could discuss that reviews.llvm.org was down...)

Thanks. That means my issue is something different, it started ~3 days earlier than that discussion happened.

llvm/tools/llvm-readobj/ELFDumper.cpp

private:
   void printRelocation(const ELFO *Obj, const Elf_Sym *Sym,
                        StringRef SymbolName, const Elf_Rela &R, bool IsRela);
   void printSymbol(const ELFO *Obj, const Elf_Sym *Symbol, const Elf_Sym *First,
                    StringRef StrTable, bool IsDynamic,
                    bool NonVisibilityBitsUsed) override;
   std::string getSymbolSectionNdx(const ELFO *Obj, const Elf_Sym *Symbol,
                                   const Elf_Sym *FirstSym);
   void printDynamicRelocation(const ELFO *Obj, Elf_Rela R, bool IsRela);
-  bool checkTLSSections(const Elf_Phdr &Phdr, const Elf_Shdr &Sec);
-  bool checkoffsets(const Elf_Phdr &Phdr, const Elf_Shdr &Sec);
-  bool checkVMA(const Elf_Phdr &Phdr, const Elf_Shdr &Sec);
-  bool checkPTDynamic(const Elf_Phdr &Phdr, const Elf_Shdr &Sec);
   void printProgramHeaders(const ELFO *Obj);
   void printSectionMapping(const ELFO *Obj);
   void printGNUVersionSectionProlog(const ELFFile<ELFT> *Obj,
                                     const typename ELFT::Shdr *Sec,
                                     const Twine &Label, unsigned EntriesNum);
 };
 
 template <class ELFT>

 static inline std::string printPhdrFlags(unsigned Flag) {
   std::string Str;
   Str = (Flag & PF_R) ? "R" : " ";
   Str += (Flag & PF_W) ? "W" : " ";
   Str += (Flag & PF_X) ? "E" : " ";
   return Str;
 }
 
-// SHF_TLS sections are only in PT_TLS, PT_LOAD or PT_GNU_RELRO
-// PT_TLS must only have SHF_TLS sections
 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) {
+  if (Sec.sh_flags & ELF::SHF_TLS) {
+    // .tbss must only be shown in the PT_TLS segment.
+    if (Sec.sh_type == ELF::SHT_NOBITS)
+      return Phdr.p_type == ELF::PT_TLS;
+
+    // SHF_TLS sections are only shown in PT_TLS, PT_LOAD or PT_GNU_RELRO
+    // segments.
+    return (Phdr.p_type == ELF::PT_TLS) || (Phdr.p_type == ELF::PT_LOAD) ||
+           (Phdr.p_type == ELF::PT_GNU_RELRO);
+  }
+
+  // PT_TLS must only have SHF_TLS sections.
+  return Phdr.p_type != ELF::PT_TLS;
 }
 
-// Non-SHT_NOBITS must have its offset inside the segment
-// Only non-zero section can be at end of segment
 template <class ELFT>
-bool GNUStyle<ELFT>::checkoffsets(const Elf_Phdr &Phdr, const Elf_Shdr &Sec) {
+static bool checkOffsets(const typename ELFT::Phdr &Phdr,
+                         const typename ELFT::Shdr &Sec) {
+  // SHT_NOBITS sections don't need to have an offset inside the segment.
   if (Sec.sh_type == ELF::SHT_NOBITS)
     return true;
-  bool IsSpecial =
-      (Sec.sh_type == ELF::SHT_NOBITS) && ((Sec.sh_flags & ELF::SHF_TLS) != 0);
-  // .tbss is special, it only has memory in PT_TLS and has NOBITS properties
-  auto SectionSize =
-      (IsSpecial && Phdr.p_type != ELF::PT_TLS) ? 0 : Sec.sh_size;
-  if (Sec.sh_offset >= Phdr.p_offset)
-    return ((Sec.sh_offset + SectionSize <= Phdr.p_filesz + Phdr.p_offset)
-            /*only non-zero sized sections at end*/
-            && (Sec.sh_offset + 1 <= Phdr.p_offset + Phdr.p_filesz));
+
+  if (Sec.sh_offset < Phdr.p_offset)
     return false;
+
+  // Only non-empty sections can be at the end of a segment.
+  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_offset + Phdr.p_filesz;
 }
 
-// SHF_ALLOC must have VMA inside segment
-// Only non-zero section can be at end of segment
+// Check that an allocatable section belongs to a virtual address
+// space of a segment.
 template <class ELFT>
-bool GNUStyle<ELFT>::checkVMA(const Elf_Phdr &Phdr, const Elf_Shdr &Sec) {
+static bool checkVMA(const typename ELFT::Phdr &Phdr,
+                     const typename ELFT::Shdr &Sec) {
   if (!(Sec.sh_flags & ELF::SHF_ALLOC))
     return true;
-  bool IsSpecial =
-      (Sec.sh_type == ELF::SHT_NOBITS) && ((Sec.sh_flags & ELF::SHF_TLS) != 0);
-  // .tbss is special, it only has memory in PT_TLS and has NOBITS properties
-  auto SectionSize =
-      (IsSpecial && Phdr.p_type != ELF::PT_TLS) ? 0 : Sec.sh_size;
-  if (Sec.sh_addr >= Phdr.p_vaddr)
-    return ((Sec.sh_addr + SectionSize <= Phdr.p_vaddr + Phdr.p_memsz) &&
-            (Sec.sh_addr + 1 <= Phdr.p_vaddr + Phdr.p_memsz));
+
+  if (Sec.sh_addr < Phdr.p_vaddr)
     return false;
+
+  bool IsTbss =
+      (Sec.sh_type == ELF::SHT_NOBITS) && ((Sec.sh_flags & ELF::SHF_TLS) != 0);
+  // .tbss is special, it only has memory in PT_TLS and has NOBITS properties.
+  bool IsTbssInNonTLS = IsTbss && Phdr.p_type != ELF::PT_TLS;
+  // Only non-empty sections can be at the end of a segment.
+  if (Sec.sh_size == 0 || IsTbssInNonTLS)
+    return Sec.sh_addr + 1 <= Phdr.p_vaddr + Phdr.p_memsz;
+  return Sec.sh_addr + Sec.sh_size <= Phdr.p_vaddr + Phdr.p_memsz;
 }
 
-// No section with zero size must be at start or end of PT_DYNAMIC
 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) {
+  if (Phdr.p_type != ELF::PT_DYNAMIC || Phdr.p_memsz == 0 || Sec.sh_size != 0)
     return true;
-  // Is section within the phdr both based on offset and VMA ?
-  return ((Sec.sh_type == ELF::SHT_NOBITS) ||
+
+  // We get here when we have an empty section. Only non-empty sections can be
+  // at the start or at the end of PT_DYNAMIC.
+  // Is section within the phdr both based on offset and VMA?
+  bool CheckOffset = (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));
+                      Sec.sh_offset < Phdr.p_offset + Phdr.p_filesz);
+  bool CheckVA = !(Sec.sh_flags & ELF::SHF_ALLOC) ||
+                 (Sec.sh_addr > Phdr.p_vaddr && Sec.sh_addr < Phdr.p_memsz);
+  return CheckOffset && CheckVA;
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printProgramHeaders(
     const ELFO *Obj, bool PrintProgramHeaders,
     cl::boolOrDefault PrintSectionMapping) {
   if (PrintProgramHeaders)
     printProgramHeaders(Obj);

 void GNUStyle<ELFT>::printSectionMapping(const ELFO *Obj) {
   OS << "\n Section to Segment mapping:\n  Segment Sections...\n";
   DenseSet<const Elf_Shdr *> BelongsToSegment;
   int Phnum = 0;
   for (const Elf_Phdr &Phdr :
        unwrapOrError(this->FileName, Obj->program_headers())) {
     std::string Sections;
     OS << format("   %2.2d     ", Phnum++);
-    for (const Elf_Shdr &Sec : unwrapOrError(this->FileName, Obj->sections())) {
-      // Check if each section is in a segment and then print mapping.
+    for (const Elf_Shdr &Sec : unwrapOrError(this->FileName, Obj->sections())) {
+      if (Sec.sh_type == ELF::SHT_NULL)
+        continue;
+
       // 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.
-      bool TbssInNonTLS = (Sec.sh_type == ELF::SHT_NOBITS) &&
-                          ((Sec.sh_flags & ELF::SHF_TLS) != 0) &&
-                          Phdr.p_type != ELF::PT_TLS;
-      if (!TbssInNonTLS && checkTLSSections(Phdr, Sec) &&
-          checkoffsets(Phdr, Sec) && checkVMA(Phdr, Sec) &&
-          checkPTDynamic(Phdr, Sec) && (Sec.sh_type != ELF::SHT_NULL)) {
+      if (checkTLSSections<ELFT>(Phdr, Sec) && checkOffsets<ELFT>(Phdr, Sec) &&
+          checkVMA<ELFT>(Phdr, Sec) && checkPTDynamic<ELFT>(Phdr, Sec)) {
         Sections +=
             unwrapOrError(this->FileName, Obj->getSectionName(&Sec)).str() +
             " ";
         BelongsToSegment.insert(&Sec);
       }
     }
     OS << Sections << "\n";
     OS.flush();