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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

So overall approach is good. See inline comments for issue and questions.



================
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;
----------------
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. 


================
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
+
----------------
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?


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:427
+    std::size_t h2 = std::hash<elf::elf_xword>()(s.st_size);
+    std::size_t h3 = std::hash<elf::elf_word>()(s.st_name);
+    std::size_t h4 = std::hash<unsigned char>()(s.st_info);
----------------
An ELF symbol from one symbol table can have the same name as another yet with a different st_name string table offset. Do we even want to be able to hash an ELFSymbol on its own? Maybe remove this enture function and only hash NamedELFSymbol?


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:430
+    std::size_t h5 = std::hash<unsigned char>()(s.st_other);
+    std::size_t h6 = std::hash<elf::elf_half>()(s.st_shndx);
+    return llvm::hash_combine(h1, h2, h3, h4, h5, h6);
----------------
I know the section index will match between for symbol tables in the same ELF file, but what about a symbol table in an external file like .gnu_debugdata?


================
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());
----------------
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".


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2202-2206
+    NamedELFSymbol needle(symbol);
+    needle.st_name_string = ConstString(symbol_bare);
+    if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+      symtab->AddSymbol(dc_symbol);
+      unique_elf_symbols.insert(needle);
----------------
Do we even need NamedELFSymbol? Can we just make an unordered_set of lldb_private::Symbol values?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390