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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
