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

[Martin Storsjö via Phabricator via lldb-commits]

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