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

[George Rimar via Phabricator via llvm-commits]

grimar 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.
+
----------------
jhenderson wrote:
> 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?
> By the way, what in this test case forces the dynamic tag to be used and no the section header?

Our logic at first finds a section header and then overrides the data with the data read
with use of dynamic tag.
(https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1735)

If something went wrong with the location found then we would report a warning:
"SHT_DYNSYM section header and DT_SYMTAB disagree about the location of the dynamic symbol table"
That is why I used "--implicit-check-not="warning:"".

But note, that we still need the section header, because we take a size of a dynamic symbol table from there.
There is no dynamic tag that allows us to get size.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:61-62
+        Value: 0
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+DynamicSymbols:
----------------
jhenderson wrote:
> 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.
It works without that, but I would like to be a bit more explicit here, since we refer to `.dynsym` from program headers.
I've added `Address: 0x100` as you suggested, though it is not really important here.
It is only important for program header to have a proper `p_offset` such that Phdr.p_offset + VAddr - Phdr.p_vaddr == offset of `.dynsym`.
(https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L557)


================
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
----------------
jhenderson wrote:
> Do we have testing elsewhere for dynamic symbol dumping of symbols with version strings like this?
No. And here it was not tested properly too. I've added a new test case for that.


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

https://reviews.llvm.org/D71595