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

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:817
+        if (symbol_type != lldb::eSymbolTypeInvalid)
+          exported->SetType(symbol_type);
+        if (exported->GetMangled() == symbol.GetMangled()) {
----------------
mstorsjo wrote:
> 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.
Given what limited info the existing implementations (LLD and GNU ld) write to the symbol table, we probably won't get any better type info from just the symbol table alone. Though I was thinking if we can use a bit of additional heuristics we can assign more specific symbol types.

For example, perhaps if we can guess that a particular code symbol may be an import thunk from the jump instruction it contains, then we can set the symbol as 'eSymbolTypeTrampoline'. It seems the breakpoint command will skip trampoline symbols, so if you, say, set a breakpoint on `memcpy` it will only break on the real function but not the import thunk. Not sure how feasible it is to implement though.


================
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());
----------------
mstorsjo wrote:
> 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?)
Hmm, I can try to improve the comment.

The `aliasInt` symbol should trigger this case. Perhaps it will be clearer if I update the previous patch to include these symbols, so the difference in output can be seen in this patch.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:892
   }
+  std::sort(export_list.begin(), export_list.end(), RVASymbolListCompareRVA);
+  return export_list;
----------------
labath wrote:
> Can you have multiple symbols pointing to the same address? Make this should use `stable_sort` instead?
Yes, it can happen. The ordering shouldn't affect what AppendFromCOFFSymbolTable does but I suppose stable_sort does make it more deterministic to avoid future issues down the line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426