[PATCH] D71595: [llvm-readobj][test] - Improve dyn-symbols.test.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:1-4
+## Here we test how dynamic symbols are dumped.
+
+## In this case we find a dynamic symbol table via the DT_SYMTAB dynamic tag.
+
----------------
As mentioned elsewhere, I'd rephrase the top-level comment "In this test we test that dynamic symbols are dumped as expected."

Also, you could simplify the case-level comments to "Case 1: Dynamic symbol table found via the DT_SYMTAB dynamic tag."

And remove the blank line after the case-level comment.

By the way, what in this test case forces the dynamic tag to be used and no the section header?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:61-62
+        Value: 0
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+DynamicSymbols:
----------------
Is this bit necessary?

Also, at a glance, the addresses look like they could get messed up (two sections have address 0). I'd suggest using a non-zero address for .dynsym.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:93
-# CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: __cxa_finalize at GLIBC_2.2.5{{ }}
-# CHECK-NEXT:     Value: 0x0
----------------
Do we have testing elsewhere for dynamic symbol dumping of symbols with version strings like this?


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

https://reviews.llvm.org/D71595