[PATCH] D93209: [libObject, llvm-readobj] - Reimplement `ELFFile<ELFT>::getEntry`.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 15 00:26:17 PST 2020
jhenderson added a comment.
Does this change impact reading a dynamic symbol table for an object with no section header table? In such a case, the size is derived from the hash table, so I imagine it'll work the same, but ideally it should be tested.
================
Comment at: llvm/test/tools/llvm-nm/invalid-symbol-table-size.test:6
# RUN: yaml2obj -DBITS=32 -DSIZE=33 -DDYNSIZE=32 %s -o %t.32-bit.o
-# RUN: not llvm-nm %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
+# RUN: not --crash llvm-nm %t.32-bit.o 2>&1 | FileCheck -DSIZE=33 -DSYMSIZE=16 -DINDEX=2 %s
# RUN: yaml2obj -DBITS=64 -DSIZE=49 -DDYNSIZE=48 %s -o %t.64-bit.o
----------------
Seems to me like this change is a regression on the old behaviour? Same with llvm-objdump and llvm-size. Please don't land it with this.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-symbol-with-versioning.test:2-3
# RUN: yaml2obj %s -o %t.o
-# RUN: llvm-readobj --demangle -r %t.o | FileCheck %s --check-prefix LLVM
-# RUN: llvm-readelf --demangle -r %t.o | FileCheck %s --check-prefix GNU
+# RUN: llvm-readobj --demangle -r %t.o 2>&1 | FileCheck %s --check-prefix LLVM
+# RUN: llvm-readelf --demangle -r %t.o 2>&1 | FileCheck %s --check-prefix GNU
----------------
Why did this change?
================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:503
+## Check that we report a warning when a relocation refers to a symbol
+## that is not exist in the symbol table.
+
----------------
Although actually I think the comment would be better as "Check that we report a warning when a relocation has a symbol index past the end of the symbol table", to avoid confusion with an attempt to look up a symbol by name.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocations.test:535-538
+ - Type: R_X86_64_NONE
+ Symbol: 0x0
+ - Type: R_X86_64_NONE
+ Symbol: 0x1
----------------
I'm not sure you really need these other relocations - just the one with the invalid symbol index would be enough.
================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:353-354
+ Expected<const typename ELF64LE::Shdr *> SecOrErr = Elf.getSection(1);
+ if (!SecOrErr)
+ GTEST_FAIL() << toString(SecOrErr.takeError());
+
----------------
This could be `ASSERT_THAT_EXPECTED(SecOrErr, Succeeded());` right?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93209/new/
https://reviews.llvm.org/D93209
More information about the llvm-commits
mailing list