[Lldb-commits] [PATCH] D134426: [lldb][COFF] Match symbols from COFF symbol table to export symbols

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.
Herald added a subscriber: JDevlieghere.

This looks mostly reasonable to me, a couple discussion points only.



================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+        if (symbol_type != lldb::eSymbolTypeInvalid)
+          exported->SetType(symbol_type);
+        if (exported->GetMangled() == symbol.GetMangled()) {
----------------
As a curious question - since D134265 we get better quality for the symbol types set from the get go - what are the other cases you foresee where this will make a difference? I don't mind if there isn't any difference though, syncing the types from the symbol table which is more expressible probably is good anyway. Just wondering about the actual utility of it.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:828
+          // either names will work. Only synchronize the symbol type.
+          if (symbol.GetType() == lldb::eSymbolTypeInvalid)
+            symbol.SetType(exported->GetType());
----------------
This condition had me puzzled for a moment, until I realized that you're synchronizing symbol type in the other direction here. Is it worth clarifying this in the comment?

(Also, I presume the test case doesn't really trigger this case?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134426/new/

https://reviews.llvm.org/D134426