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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
