This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][test] - Improve dyn-symbols.test.
ClosedPublic

Authored by grimar on Dec 17 2019, 2:57 AM.

Details

Summary

This removes the precompiled binary used, simplifies
the first test case, adds comments and llvm-readelf tool
invocations.
It also adds a test case for checking versioning symbols.

Diff Detail

Event Timeline

grimar created this revision.Dec 17 2019, 2:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
jhenderson added inline comments.Dec 18 2019, 1:29 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
1–2

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?

61–62

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.

93

Do we have testing elsewhere for dynamic symbol dumping of symbols with version strings like this?

grimar updated this revision to Diff 234504.Dec 18 2019, 4:15 AM
grimar marked 3 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
1–2

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.

61–62

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)

93

No. And here it was not tested properly too. I've added a new test case for that.

jhenderson accepted this revision.Dec 18 2019, 5:23 AM

LGTM.

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
68

Doesn't this need to be 0x100 too? Possibly not strictly, but otherwise the VAddr viewed via the program header won't match that in the section header. The tag would need updating too.

This revision is now accepted and ready to land.Dec 18 2019, 5:23 AM
grimar marked an inline comment as done.Dec 18 2019, 5:39 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
68

but otherwise the VAddr viewed via the program header won't match that in the section header.

Right, but isn't that good for this test? We are trying to test that "dynamic symbol table found via the DT_SYMTAB dynamic tag".
Ideally we do not want to have a section header at all (but we can't).

jhenderson added inline comments.Dec 18 2019, 5:43 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
68

I'm just thinking that the section mapping produced by something like llvm-readelf is going to show the dynsym as not in the program header, if I'm not mistaken, which could get confusing if anybody debugs this test in the future.

I'm okay with no addresses being set at all. I just think it needs to be consistent.

MaskRay accepted this revision.Dec 18 2019, 1:26 PM
MaskRay added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
76

cmp

This revision was automatically updated to reflect the committed changes.