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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk 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()));
+}
----------------
jankratochvil wrote:
> llvm::hash_combine already calls std::hash<T> for each of its parameters.
Good to know. Thank you.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2651
 Symtab *ObjectFileELF::GetSymtab() {
+  fprintf(stderr, "GetSymtab()\n");
   ModuleSP module_sp(GetModule());
----------------
labath wrote:
> delete
Sorry, I noticed this myself as well. Some of the performance experiment spilled over in this patch. 


================
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;
+    
----------------
jankratochvil wrote:
> For the planned rework of the unification of symbols it could be put (I think) to `Symtab::InitAddressIndexes` which already sorts the Symtab anyway.
Honestly, for the rework I think about not to use an `std::vector` as you proposed but instead create the `std::unordered_set` using a bucket count that is equal to the number of symbols in `.symtab` and in `.dynsym`. Then inserts to that set will be constant as they are for the vector. But let's see how it goes in a followup patch. I have the feeling that if I use the approach you suggested, I need to keep a vector *and* a set around. The vector for collecting all symbols and the set for doing the unification, no? Anyway, let's postpone this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390