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

Konrad Wilhelm Kleine via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Oct 1 02:40:14 PDT 2019


kwk marked an inline comment as done.
kwk added a comment.

@labath can you please check this patch one last time (hopefully)?



================
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;
----------------
labath wrote:
> kwk wrote:
> > labath wrote:
> > > 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?
> > > I'll echo @clayborg here. This business with copying the ELFSymbol and clearing some fields is confusing.
> > 
> > I've cleared up the documentation now and it is exactly the way I like it. Every entity deals with it's own business (respects its own fields when comparing). I find it pretty dirty to compare fields from the base struct in a derived one. The way I compare fields from the base struct is minimally invasive.
> > 
> > > Do you even need the ELFSymbol::operator== for anything?
> > 
> > Yes, when you want to compare ELFSymbols. I know that I don't do that explicitly but I the function only deals with fields from the entity itself and they don't spill into any derived structure (with the exception of explicitly ignoring fields).
> > 
> > > If not I'd just delete that, and have the derived version compare all fields.
> > 
> > No because I call it explcitly from the derived NamedELFSymbol.
> > 
> > > 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?
> > 
> > I've found a compromise that is even more appealing to me. The ELFSymbol and NamedELFSymbol structs now have a `hash` function which contains the implementation next to the one of `operator==()`. This `hash` is called in the specialization which remains in the same location as before; the reason being that I didn't find a way do define something in the `std::` namespace when I'm in the `elf::` namespace.
> > 
> > 
> > Yes, when you want to compare ELFSymbols. I know that I don't do that explicitly but I the function only deals with fields from the entity itself and they don't spill into any derived structure (with the exception of explicitly ignoring fields).
> Yes, but to me that exception kind of invalidates the whole idea. In order to know which fields you need to ignore, you need the knowledge of what fields are present in the struct (and as the fields are public, that is not a big deal), at which point you can just avoid the whole copying business, and explicitly compare the fields that you care about. Given that this also saves a nontrivial amount of code, I still think it's a better way to go. (Also, defining equality operators on class hierarchies is usually not a good idea even if they "nest" nicely, since they can still produce strange results due to object slicing.)
> 
> > I've found a compromise that is even more appealing to me. The ELFSymbol and NamedELFSymbol structs now have a hash function which contains the implementation next to the one of operator==().
> 
> That works for me.
@labath okay, I've remove the logic from `ELFSymbol` and coded everything straight away. I guess, that I wanted to be able to extend `ELFSymbol` with n number of fields and add them to the `ELFSymbol::operator==()` without touching the `NamedELFSymbol::operator==()` as long as the added fields shall not be ignored. Makes sense? I guess that you can find arguments for both ways to implement it. Anyway, I've coded it the way you want now, I hope.


================
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);
----------------
labath wrote:
> kwk wrote:
> > labath wrote:
> > > jankratochvil wrote:
> > > > labath wrote:
> > > > > something like `if (unique_elf_symbols.insert(needle).second)` would be more efficient, as you don't need to mess with the map twice.
> > > > I would unconditionally add all the symbols to `std::vector<NamedElfSymbol> unique_elf_symbols` (with `unique_elf_symbols.reserve` in advance for the sym of `.symtab`+`.dynsym` sizes) and then after processing both `.symtab` and `.dynsym` and can `llvm::sort(unique_elf_symbols)` and add to `symtab` only those which are unique. I believe it will be much faster, one could benchmark it.
> > > sounds like a good idea.
> > @jankratochvil @labath yes, this sound like a good idea for *performance* improvement but honestly, I need to get this patch done first in order to make any progress with minidebuginfo. I hope you don't mind when I take this task to another patch, okay?
> I don't think that kind of reasoning really applies here, since it's this patch that is introducing the potential performance problem. However, I don't think that is going to be a big deal, so I think we can leave this out for now.
Okay, thank you for allowing me to leave it out for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390





More information about the lldb-commits mailing list