[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun abandoned this revision. alvinhochun added a comment. Handled in separate changes. 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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun added a comment. Separated the first part with some new changes here: https://reviews.llvm.org/D134196 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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun added inline comments.
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());
mstorsjo wrote:
> alvinhochun wrote:
> > mstorsjo wrote:
> > > 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?
> > Indeed it is a good idea to match symbols with different export names to
> > synchronize the symbol types. I probably should use a `vector > export_name>>` sorted by address for lookup instead.
> Is it even relevant to keep the name in the vector in that case? As you'd
> only be looking up on address anyway, and based on that you can get the
> symbol name from the pointed-at symbol anyway, right?
Ah, I guess I meant to say the symbol index there...
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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
mstorsjo added inline comments.
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:787
+ Symbol *symbols = symtab.Extend(num_syms);
+ uint32_t i = 0;
+ for (const auto _ref : m_binary->symbols()) {
alvinhochun wrote:
> mstorsjo wrote:
> > 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?
> I made `symtab.Extend` return a pointer to the first item of the appended
> range, so counting from 0 does not overwrite existing symbols.
Ah, I see - ok, good then, otherwise I was wondering how this was working at
all :-)
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());
alvinhochun wrote:
> mstorsjo wrote:
> > 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?
> Indeed it is a good idea to match symbols with different export names to
> synchronize the symbol types. I probably should use a `vector export_name>>` sorted by address for lookup instead.
Is it even relevant to keep the name in the vector in that case? As you'd only
be looking up on address anyway, and based on that you can get the symbol name
from the pointed-at symbol anyway, right?
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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun added a comment.
Thanks for the review. Yes I shall split the changes into smaller pieces to aid
review.
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1558
}
+ strm.IndentLess();
}
mstorsjo wrote:
> Looks like this is a stray change unrelated to the rest (although it does
> seem correct).
Oh right, this is relevant for https://reviews.llvm.org/D131705#inline-1292343.
I'll make sure not to mix in this change for the final patch.
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:787
+ Symbol *symbols = symtab.Extend(num_syms);
+ uint32_t i = 0;
+ for (const auto _ref : m_binary->symbols()) {
mstorsjo wrote:
> 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?
I made `symtab.Extend` return a pointer to the first item of the appended
range, so counting from 0 does not overwrite existing symbols.
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());
mstorsjo wrote:
> 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?
Indeed it is a good idea to match symbols with different export names to
synchronize the symbol types. I probably should use a `vector>` sorted by address for lookup instead.
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);
mstorsjo wrote:
> If it's relevant, we can look up what section the exported address is in, and
> check if the section is executable or not.
Right, this is something we can do too.
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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
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 _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
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun created this revision.
alvinhochun added reviewers: labath, DavidSpickett, mstorsjo.
Herald added a project: All.
alvinhochun requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.
This reimplements `ObjectFilePECOFF::ParseSymtab` with the following
changes:
- When the image has both an export table and a COFF symbol table, the symbols
obtained from the symbol table no longer get erased.
- Remove manual data extraction in favour of using what `COFFObjectFile`
provides.
- Mark symbols from the export table as "External" instead of "Debug".
- Support DLL forwarder exports (marked as re-exported).
- Handle absolute symbols in the symbol table.
- Heuristically set some symbol types. Symbols in the symbol table starting in
`__imp_` (dllimport IAT reference) or `.refptr.` (mingw stub) are marked as
"Data", because their locations store a pointer address.
- When a symbol in the symbol table is a duplicate of an exported symbol, its
info will be synchronized with the exported symbol, then it will be marked as
"Additional" type to avoid unwanted repetition when used in commands like
`disassemble`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D134133
Files:
lldb/include/lldb/Symbol/Symtab.h
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
lldb/source/Symbol/Symtab.cpp
llvm/include/llvm/Object/COFF.h
Index: llvm/include/llvm/Object/COFF.h
===
--- llvm/include/llvm/Object/COFF.h
+++ llvm/include/llvm/Object/COFF.h
@@ -914,6 +914,10 @@
uint32_t getStringTableSize() const { return StringTableSize; }
+ const export_directory_table_entry *getExportTable() const {
+return ExportDirectory;
+ }
+
const coff_load_configuration32 *getLoadConfig32() const {
assert(!is64());
return reinterpret_cast(LoadConfig);
Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -61,6 +61,14 @@
return m_symbols.empty() ? nullptr : _symbols[0];
}
+Symbol *Symtab::Extend(size_t count) {
+ // Clients should grab the mutex from this symbol table and lock it manually
+ // when calling this function to avoid performance issues.
+ auto old_size = m_symbols.size();
+ m_symbols.resize(old_size + count);
+ return m_symbols.empty() ? nullptr : _symbols[old_size];
+}
+
uint32_t Symtab::AddSymbol(const Symbol ) {
// Clients should grab the mutex from this symbol table and lock it manually
// when calling this function to avoid performance issues.
@@ -809,6 +817,22 @@
return nullptr;
}
+bool Symtab::HasSymbolWithType(SymbolType symbol_type, Debug symbol_debug_type,
+ Visibility symbol_visibility) const {
+ std::lock_guard guard(m_mutex);
+
+ const size_t count = m_symbols.size();
+ for (size_t idx = 0; idx < count; ++idx) {
+if (symbol_type == eSymbolTypeAny ||
+m_symbols[idx].GetType() == symbol_type) {
+ if (CheckSymbolAtIndex(idx, symbol_debug_type, symbol_visibility)) {
+return true;
+ }
+}
+ }
+ return false;
+}
+
void
Symtab::FindAllSymbolsWithNameAndType(ConstString name,
SymbolType symbol_type,
Index: lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
===
--- lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
+++ lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
@@ -81,6 +81,11 @@
abilities |= Functions;
}
+ if (symtab->HasSymbolWithType(eSymbolTypeReExported, Symtab::eDebugAny,
+Symtab::eVisibilityAny)) {
+abilities |= Functions;
+ }
+
if (symtab->AppendSymbolIndexesWithType(eSymbolTypeData,
m_data_indexes)) {
symtab->SortSymbolIndexesByValue(m_data_indexes, true);
Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
@@ -225,12 +225,6 @@
data_dirs; // will contain num_data_dir_entries entries
} coff_opt_header_t;
- enum coff_data_dir_type {
-coff_data_dir_export_table = 0,
-coff_data_dir_import_table = 1,
-coff_data_dir_exception_table = 3
- };
-
typedef struct section_header {
char name[8] = {};
uint32_t vmsize = 0; // Virtual Size
@@ -244,29 +238,6 @@
uint32_t flags = 0;
} section_header_t;
- typedef struct coff_symbol {
-char name[8] = {};
