kwk added a comment.
Not all is answered now but please respect:
https://reviews.llvm.org/D67390#1683705
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373
+ r.st_name = st_name;
+ return elf::ELFSymbol::operator==(r) &&
+ st_name_string == rhs.st_name_string;
----------------
clayborg wrote:
> I would almost just manually compare only the things we care about here.
> Again, what about st_shndx when comparing a symbol from the main symbol table
> and one from the .gnu_debugdata symbol table. Are those section indexes
> guaranteed to match? I would think not.
@clayborg I explicitly only compare what we care about and therefore always set
the name index to be the same.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:289
+struct NamedELFSymbol : ELFSymbol {
+ lldb_private::ConstString st_name_string; ///< Actual name of the ELF symbol
+
----------------
clayborg wrote:
> Do we need a ConstString for the st_shndx as well so we can correctly compare
> a section from one file to a section from another as in the .gnu_debugdata
> case?
That is a good point.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:440
+ tmp.st_name = 0;
+ std::size_t h1 = std::hash<elf::ELFSymbol>()(tmp);
+ std::size_t h2 = std::hash<const char *>()(s.st_name_string.AsCString());
----------------
clayborg wrote:
> Don't we need to hash everything we care about except the st_name? Those
> indexes can differ if they come from a different string table? Shouldn't this
> be:
>
> ```
> std::size_t h1 = std::hash<elf::elf_addr>()(s.st_value);
> std::size_t h2 = std::hash<elf::elf_xword>()(s.st_size);
> // Skip std::size_t h3 = std::hash<elf::elf_word>()(s.st_name);
> std::size_t h4 = std::hash<unsigned char>()(s.st_info);
> std::size_t h5 = std::hash<unsigned char>()(s.st_other);
> std::size_t h6 = std::hash<elf::elf_half>()(s.st_shndx);
> std::size_t h7 = std::hash<const char *>()(s.st_name_string.AsCString());
> return llvm::hash_combine(h1, h2, h4, h5, h6, h7);
> ```
> I left the "h" variables with the same names to show we don't want "h3".
That's why I explicitly set the name to 0 always which effectively ignores it
because it has no effect on the hash then. Please see the specialization hash
for `NamedELFSymbol`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67390/new/
https://reviews.llvm.org/D67390
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
