mstorsjo added a comment.
So previously, LLDB essentially used the COFF symbol table for executables, but
only the list of exported symbols for DLLs, ignoring (or, reading and then
overwriting) the symbol table for any DLL with exports? Then this certainly
does look like a good direction.
Overall, I think this does look nice, but it's certainly hard to rewrite and
wrap one's head around. When you've settled what you want to achieve, can you
try to split it up into a short series of patches, so that the initial rewrite
patch doesn't add a ton of extra functionality, but only covers the rewrite and
appending instead of wiping symbols?
================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1558
}
+ strm.IndentLess();
}
----------------
Looks like this is a stray change unrelated to the rest (although it does seem
correct).
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:787
+ Symbol *symbols = symtab.Extend(num_syms);
+ uint32_t i = 0;
+ for (const auto &sym_ref : m_binary->symbols()) {
----------------
If we mean to append to the symbol table here, shouldn't we start with `i =
symtab.size()` or similar? Otherwise we'd start overwriting the existing
symbols from the start of the table?
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:819
+ if (exported->GetType() != lldb::eSymbolTypeReExported &&
+ exported->GetAddressRef() == symbols[i].GetAddressRef()) {
+ symbols[i].SetID(exported->GetID());
----------------
What about the case when a symbol is exported with a different name than the
local symbol? (This is doable with def files e.g. as `ExportedName = LocalName`
iirc.) Is it worth to have a map of address -> exported symbol, to use instead
of the raw name?
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:917
+ // actually be data, but this is relatively rare and we cannot tell
+ // from just the export table.
symbols[i].SetType(lldb::eSymbolTypeCode);
----------------
If it's relevant, we can look up what section the exported address is in, and
check if the section is executable or not.
================
Comment at: lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp:84
+ if (symtab->HasSymbolWithType(eSymbolTypeReExported, Symtab::eDebugAny,
+ Symtab::eVisibilityAny)) {
----------------
For ease of review (when removing the WIP status), I think it'd be easier to
wrap the head around, if new features like these were deferred to a separate
patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134133/new/
https://reviews.llvm.org/D134133
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
