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