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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

This looks fairly ok to me, but it could use a more explicit test of the symbol uniqueing code. Right, now I believe the two tests you added check that the symbols are _not_ uniqued. (They're also a bit hard to follow due to the whole 
 objcopy business). Could you create a test with a yaml file which will contain various edge cases relevant to this code. I'm thinking of stuff like "a dynsym and a symtab symbol at the same address, but a different name", "a dynsym and symtab symbols with identical names, but different addresses", etc. Then just run "image dump symtab" on that file to check what we have parsed?

I am also surprised that you weren't able to just use a Section* (instead of the name) for the uniqueing. I'd expect that all symbols (even those from the separate object file) should be resolved to the sections in the main object. I see that this isn't the case, but I am surprised that this isn't causing any problems. Anyway, as things seem to be working as they are now, we can leave that for another day.

In D67390#1685313 <https://reviews.llvm.org/D67390#1685313>, @kwk wrote:

> Before I used the bare symbol name with stripped `@VERSION` suffix. Now I've changed the implementation of `NamedELFSymbol` to include the `@VERSION` suffix and the tests pass.


Interesting. I'm pretty sure that the symbol count is irrelevant for that test (it just wants to know if it is there), so we can change that if needed. However, having the uniqueing include the `@VERSION` sounds right to me, so if that makes the test happy too then, great.



================
Comment at: lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c:1
+// This function will be embedded within the .dynsym section of the main binary.
+int functionInDynsym(int num) { return num * 3; }
----------------
It looks like you could inline these test inputs into the test files. You'd just need to change all the comments to `//` and add `.c` to the list of valid suffixes in lit.local.cfg.


================
Comment at: lldb/lit/Modules/ELF/load-from-dynsym-alone.test:16
+# have it put the default .symtab section.
+# RUN: echo "{functionInDynsym;};" > %T/dynmic-symbols.txt
+# RUN: %clang -Wl,--dynamic-list=%T/dynmic-symbols.txt -g -o %t.binary %p/Inputs/load-from-dynsym-alone.c
----------------
s/dynmic/dynamic/


================
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;
----------------
kwk wrote:
> 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.
I'll echo @clayborg here. This business with copying the ELFSymbol and clearing some fields is confusing. Do you even need the ELFSymbol::operator== for anything? If not I'd just delete that, and have the derived version compare all fields.

Also, it would be nice if the operator== and hash function definitions were next to each other. Can you just forward declare the std::hash stuff in the header, and have the implementation be next to this?


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2202-2204
+    needle.st_name_string = ConstString(symbol_ref);
+    if (symbol_section_sp)
+        needle.st_section_name_string = ConstString(symbol_section_sp->GetName());
----------------
Move this into the NamedELFSymbol constructor?


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2205
+        needle.st_section_name_string = ConstString(symbol_section_sp->GetName());
+    if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) {
+      symtab->AddSymbol(dc_symbol);
----------------
something like `if (unique_elf_symbols.insert(needle).second)` would be more efficient, as you don't need to mess with the map twice.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2650
+    // A unique set of ELF symbols added to the symtab
+    UniqueElfSymbolColl unique_elf_symbols({});
     SectionList *section_list = module_sp->GetSectionList();
----------------
what's wrong with the old-fashioned `UniqueElfSymbolColl unique_elf_symbols;` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390