[llvm-readobj] - Fix the invalid dumping of the dynamic sections without terminating DT_NULL entry.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=40861,

Previously llvm-readobj would print the DT_NULL sometimes for the dynamic section that
has no terminator entry. The logic of printDynamicTable was a bit overcomplicated
fo my eye, I rewrote it slightly and commented.

[jhenderson]

Thanks, I completely agree with you that the logic is unnecessarily complicated.

test/tools/llvm-readobj/elf-no-dtnull.s

1 ↗

(On Diff #188530)

I'd rename this table to make it clear we're testing the dynamic table (i.e. something like elf-dynamic-table-no-dtnull.s).

4 ↗

(On Diff #188530)

I don't feel strongly about this, so if you don't want to that's fine, but as there is a chance of the GNU dumper diverging from the LLVM dumper here in the future (since the output is not all that GNU-like), I think you should test both styles.

30 ↗

(On Diff #188530)

.dynstr? What .dynstr?

Ditto below.

tools/llvm-readobj/ELFDumper.cpp

1917 ↗

(On Diff #188530)

terminated with an -> terminated with a

1918 ↗

(On Diff #188530)

I'd rephrase this and the last sentence somewhat, as it's not clear at first what you mean:

"However, sometimes the section content may continue past the DT_NULL entry, so to dump the section correctly, we first find the end of the entries by iterating over them."

[grimar]

• Addressed review comments.

test/tools/llvm-readobj/elf-no-dtnull.s

1 ↗

(On Diff #188530)

I renamed to elf-dynamic-table-dtnull.s (without -no- suffix, because I missed initially that the second sub-test
also actually checks the case when we have the DT_NULL)

4 ↗

(On Diff #188530)

I have no problems with doing that. Done.

30 ↗

(On Diff #188530)

Fixed.

tools/llvm-readobj/ELFDumper.cpp

1918 ↗

(On Diff #188530)

Done, thanks!

[grimar]

test/tools/llvm-readobj/elf-no-dtnull.s

4 ↗

(On Diff #188530)

The output is the same atm so I did not introduce LLVM and GNU checks testing the same. I guess that is fine for that situation?

[jhenderson]

LGTM.

test/tools/llvm-readobj/elf-no-dtnull.s

4 ↗

(On Diff #188530)

Yes, that's fine.

llvm/trunk/test/tools/llvm-readobj/elf-dynamic-table-dtnull.s

+# Check we are able to dump the dynamic section without a DT_NULL entry correctly.
+
+# RUN: yaml2obj -docnum=1 %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o | FileCheck %s --check-prefix=NONULL
+# RUN: llvm-readelf --dynamic-table %t.o | FileCheck %s --check-prefix=NONULL
+
+# NONULL:      DynamicSection [ (1 entries)
+# NONULL-NEXT:   Tag                Type   Name/Value
+# NONULL-NEXT:   0x0000000000000015 DEBUG  0x0
+# NONULL-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .dynamic
+    Type:            SHT_DYNAMIC
+    Address:         0x0000000000001010
+    AddressAlign:    0x0000000000000010
+    EntSize:         0x0000000000000010
+    Entries:
+      - Tag:             DT_DEBUG
+        Value:           0x0000000000000000
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynamic
+  - Type: PT_DYNAMIC
+    VAddr: 0x1010
+    Sections:
+      - Section: .dynamic
+
+# Sometimes .dynamic section content length can be greater than the
+# length of its entries. In this case, we should not try to dump anything
+# past the DT_NULL entry, which works as a terminator.
+
+# RUN: yaml2obj -docnum=2 %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o | FileCheck %s --check-prefix=LONG
+# RUN: llvm-readelf --dynamic-table %t.o | FileCheck %s --check-prefix=LONG
+
+# LONG:      DynamicSection [ (2 entries)
+# LONG-NEXT:   Tag                Type                 Name/Value
+# LONG-NEXT:   0x0000000000000015 DEBUG                0x0
+# LONG-NEXT:   0x0000000000000000 NULL                 0x0
+# LONG-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .dynamic
+    Type:            SHT_DYNAMIC
+    Address:         0x0000000000001010
+    AddressAlign:    0x0000000000000010
+    EntSize:         0x0000000000000010
+    Entries:
+      - Tag:             DT_DEBUG
+        Value:           0x0000000000000000
+      - Tag:             DT_NULL
+        Value:           0x0000000000000000
+      - Tag:             DT_NULL
+        Value:           0x0000000000000000
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynamic
+  - Type: PT_DYNAMIC
+    VAddr: 0x1010
+    Sections:
+      - Section: .dynamic

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

template <> void ELFDumper<ELF32LE>::printUnwindInfo() {
   }
   W.startLine() << "UnwindInfo not implemented.\n";
 }
 
 } // end anonymous namespace
 
 template<class ELFT>
 void ELFDumper<ELFT>::printDynamicTable() {
-  auto I = dynamic_table().begin();
-  auto E = dynamic_table().end();
-
-  if (I == E)
-    return;
-
-  --E;
-  while (I != E && E->getTag() == ELF::DT_NULL)
-    --E;
-  if (E->getTag() != ELF::DT_NULL)
-    ++E;
-  ++E;
+  // A valid .dynamic section contains an array of entries terminated with
+  // a DT_NULL entry. However, sometimes the section content may continue
+  // past the DT_NULL entry, so to dump the section correctly, we first find
+  // the end of the entries by iterating over them.
+  size_t Size = 0;
+  Elf_Dyn_Range DynTableEntries = dynamic_table();
+  for (; Size < DynTableEntries.size();)
+    if (DynTableEntries[Size++].getTag() == DT_NULL)
+      break;
 
-  ptrdiff_t Total = std::distance(I, E);
-  if (Total == 0)
+  if (!Size)
     return;
 
   raw_ostream &OS = W.getOStream();
-  W.startLine() << "DynamicSection [ (" << Total << " entries)\n";
+  W.startLine() << "DynamicSection [ (" << Size << " entries)\n";
 
   bool Is64 = ELFT::Is64Bits;
-
   W.startLine()
      << "  Tag" << (Is64 ? "                " : "        ") << "Type"
      << "                 " << "Name/Value\n";
-  while (I != E) {
-    const Elf_Dyn &Entry = *I;
+  for (size_t I = 0; I < Size; ++I) {
+    const Elf_Dyn &Entry = DynTableEntries[I];
     uintX_t Tag = Entry.getTag();
-    ++I;
     W.startLine() << "  " << format_hex(Tag, Is64 ? 18 : 10, opts::Output != opts::GNU) << " "
                   << format("%-21s", getTypeString(ObjF->getELFFile()->getHeader()->e_machine, Tag));
     printValue(Tag, Entry.getVal());
     OS << "\n";
   }
 
   W.startLine() << "]\n";
 }