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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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



================
Comment at: test/tools/llvm-readobj/elf-no-dtnull.s:1
+# Check we are able to dump the dynamic section without a DT_NULL entry correctly.
+
----------------
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).


================
Comment at: test/tools/llvm-readobj/elf-no-dtnull.s:4
+# RUN: yaml2obj -docnum=1 %s -o %t.o
+# RUN: llvm-readobj --dynamic-table %t.o | FileCheck %s
+
----------------
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.


================
Comment at: test/tools/llvm-readobj/elf-no-dtnull.s:30
+    Sections:
+      - Section: .dynstr
+      - Section: .dynamic
----------------
.dynstr? What .dynstr?

Ditto below.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1917
 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 an
+  // DT_NULL entry. But sometimes section content might overpass that barrier
----------------
terminated with an -> terminated with a


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1918
+  // A valid .dynamic section contains an array of entries terminated with an
+  // DT_NULL entry. But sometimes section content might overpass that barrier
+  // and so to dump this section correctly we should find the end of entries at
----------------
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."


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

https://reviews.llvm.org/D58716