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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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.
+
----------------
jhenderson wrote:
> 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).
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)


================
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
+
----------------
jhenderson wrote:
> 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.
I have no problems with doing that. Done.


================
Comment at: test/tools/llvm-readobj/elf-no-dtnull.s:30
+    Sections:
+      - Section: .dynstr
+      - Section: .dynamic
----------------
jhenderson wrote:
> .dynstr? What .dynstr?
> 
> Ditto below.
Fixed.


================
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
----------------
jhenderson wrote:
> 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."
Done, thanks!


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

https://reviews.llvm.org/D58716