[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:383
+      std::hash<const char *>()(st_name_string.AsCString()),
+      std::hash<const char *>()(st_section_name_string.AsCString()));
+}
----------------
llvm::hash_combine already calls std::hash<T> for each of its parameters.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:446
+    std::size_t h2 = std::hash<const char *>()(s.st_name_string.AsCString());
+    std::size_t h3 = std::hash<const char *>()(s.st_section_name_string.AsCString());
+    return llvm::hash_combine(h1, h2, h3);
----------------
kwk wrote:
> jankratochvil wrote:
> > I find better to rather define `std::hash<ConstString>` (or provide `ConstString::Hasher` which I do in my DWZ patchset).
> Once your DWZ patchset arrives, please let me know and we can change it.
https://people.redhat.com/jkratoch/ConstStringHasher.patch - Although then the whole UniqueElfSymbolColl should be replaced by `std::sort`+`std::unique` of `std::vector` you plan in a future patch so the hashing does not matter much.



================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1925
+            unique_elf_symbols_sp->bucket_count(), num_symbols,
+            unique_elf_symbols_sp->bucket_count() + num_symbols);
+    std::unique_ptr<UniqueElfSymbolColl> newColl(new UniqueElfSymbolColl(
----------------
also delete


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2681
+    // A unique set of ELF symbols added to the symtab
+    UniqueElfSymbolCollSP unique_elf_symbols_sp;
+    
----------------
For the planned rework of the unification of symbols it could be put (I think) to `Symtab::InitAddressIndexes` which already sorts the Symtab anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67390/new/

https://reviews.llvm.org/D67390