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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
- Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
1–2 |
Our logic at first finds a section header and then overrides the data with the data read If something went wrong with the location found then we would report a warning: But note, that we still need the section header, because we take a size of a dynamic symbol table from there. | |
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. | |
93 | No. And here it was not tested properly too. I've added a new test case for that. |
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. |
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
68 |
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". |
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. |
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test | ||
---|---|---|
76 | cmp |
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?