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

[Alvin Wong via Phabricator via lldb-commits]

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