[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables

[Martin Storsjö via Phabricator via lldb-commits]

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 &sym_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<pair<address, 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