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

Fri Sep 23 12:13:14 PDT 2022

mstorsjo added inline comments.

================
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());
----------------
alvinhochun wrote:
> mstorsjo wrote:
> > alvinhochun wrote:
> > > 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.
> > Oh, ok, I had missed that `MapSymbolType` returns code or invalid - I had expected it to return code or data. I guess this is fine then. (Changing that function, or changing the logic here to look at section types is out of scope here anyway, and I don’t know if the difference between data and invalid matters?)
> Data vs Invalid does seem to have an effect on whether a symbol can be printed as data or have its address taken. I can't print or dump any static/globals when the type is Invalid. I think the DWARF debug info should have at least some information about these symbols, but I have no idea how that works.
> 
> I wonder if I should change `MapSymbolType` to return Data for anything that is not code?
I think that might be a good idea - but that's clearly a separate change from this one too. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134426