kwk marked 9 inline comments as done.
kwk added a comment.
I think I've finished the implementation now and should have answered all your
comments and concerns. I run tests now. I would appreciate if you (@clayborg ,
@labath , @jankratochvil ) can take a look at this patch again.
================
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
+
----------------
kwk wrote:
> clayborg wrote:
> > 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?
> That is a good point.
@clayborg in fact I think this could be the reason not to use a set of
`lldb_private::Symbol` objects because there we don't store the section name or
symbol name but only addresses or indexes. I did add the
`st_section_name_string` struct member.
================
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);
----------------
clayborg wrote:
> 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?
I did add the section name to `NamedELFSymbol` and explicitly ignore it when
building the hash for the base `ELFSymbol`.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:42
#include "llvm/Support/MipsABIFlags.h"
+#include "lldb/Utility/StreamString.h"
----------------
jankratochvil wrote:
> Is it really needed?
removed.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2205
+ m_unique_symbol_set.push_back(symbol);
+ }
}
----------------
jankratochvil wrote:
> What if the symbol is ignored, the function will then incorrectly return a
> number of added symbols even when they were not added, wouldn't it?
@jankratochvil we already have places inside this `for`-loop where we
`continue`. I hope it is okay to ask the same question back that you've asked
for those `continue`-places. Why don't we adjust the returned number (`i`) in
case symbols where skipped?
================
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);
----------------
clayborg wrote:
> Do we even need NamedELFSymbol? Can we just make an unordered_set of
> lldb_private::Symbol values?
@clayborg I find it much easier with `NamedELFSymbol` because all we have to do
is derive from `ELFSymbol` and add the strings for the symbol name and the
section name. If we were to use `lldb_private::Symbol` I would have to lookup
the symbols manually each time I calculate a hash which seems bad. I mean, the
symbol and section name already are `ConstString`s and should be stored and
computed very efficiently. Also I wanted to keep things local to ELF and not
mess with everything that uses `lldb_private::Symbol`. Makes sense?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67390/new/
https://reviews.llvm.org/D67390
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
