[PATCH] D73269: [llvm-readobj] - Add a few warnings for --gnu-hash-table.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 24 08:23:05 PST 2020
jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnuhash.test:259
+
+## Linker might produce an empty no-op SHT_GNU_HASH section when
+## there are no dynamic symbols or when all dynamic symbols are undefined.
----------------
Linker -> either "Linkers" or "A linker"
================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnuhash.test:264-267
+## The index of the first symbol in the dynamic symbol table
+## included in the hash table can be set to the number of dynamic symbols,
+## what is one larger that the index of the last dynamic symbol and we
+## normally should report it, but not in this case.
----------------
I'm not sure I follow why we shouldn't report this in this specific case? Surely the linker should just set the index to 0, not 1?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2506
+ if (!DynSymRegion.Addr) {
+ reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
+ "section: found no dynamic symbol section"),
----------------
I was going to suggest that this should be `reportUniqueWarning`, but I guess that it can only ever be hit once per file, right?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2507
+ reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
+ "section: found no dynamic symbol section"),
+ ObjF->getFileName());
----------------
I'd move "found" to after "section". I might also change section -> table here, since the data does not technically need a section header, but follow whatever we say elsewhere.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2512
+
+ unsigned NumSyms = dynamic_symbols().size();
+ if (!NumSyms) {
----------------
`unsigned` -> `size_t`?
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2521-2524
+ // A normal empty GNU hash table section produced by linker might have
+ // symndx set to the number of dynamic symbols + 1 (for the zero symbol)
+ // and have the dummy null values in the bloom filter and in the buckets
+ // vector. We should not report a warning in this case.
----------------
Similar comment to the test. Why is it okay for linkers to do this? At least explain this.
================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2530-2531
+ if (!IsEmptyHashTable) {
+ // Use -1 to print the number of dynamic symbols because we do not want to
+ // count the null symbol at the begining of a dynamic symbol table.
+ reportWarning(
----------------
I'm not sure I agree with this statement. The first symbol is the symbol with index 0. Saying we have 0 dynamic symbols is incorrect when there is just that symbol. After all the size of dynamic_symbols() will be 1.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73269/new/
https://reviews.llvm.org/D73269
More information about the llvm-commits
mailing list