clayborg added a comment. In D67390#1671128 <https://reviews.llvm.org/D67390#1671128>, @kwk wrote:
> In D67390#1671018 <https://reviews.llvm.org/D67390#1671018>, @labath wrote: > > > In D67390#1667270 <https://reviews.llvm.org/D67390#1667270>, @kwk wrote: > > > > > @labath how shall we go about this? We do have the situation that when > > > you lookup a symbol you might find it twice if it is in `.dynsym` and in > > > `.symtab`. Shall I adjust the test expectation to that or change my > > > implementation? > > > > > > That's a good question (and another reason why I wanted this to be a > > separate patch). Since only two tests broke it does not seem like having > > some symbols twice does much harm. OTOH, having an identical symbol twice > > does seem like asking for trouble down the line. One possibility would be > > to restrict this merging to the gnu_debugdata case only. Another option > > would be to make the merging code smarter and avoid adding the symbol a > > second time if it has the same name and address. That would have the > > advantage of having the symbol just once in the common case, while still > > preserving the full information (in case the symbol tables were munged > > independently of the gnu_debugdata thingy). > > > > Overall, I guess I would prefer the last solution (inserting only different > > symbols) unless that turns out to be difficult. In that case, I think just > > restricting this to gnu_debugdata is fine. > > > The crucial line is the following line in `ObjectFileELF::ParseSymbols()`: > > symtab->AddSymbol(dc_symbol); > > > If I change that to: > > symtab->FindSymbolsByNameAndAddress(dc_symbol.GetName(), > dc_symbol.GetAddress(), indexVector) > if (indexVector.empty()) { > symtab->AddSymbol(dc_symbol); > } > in mach-o plug-in we keep a std::map or something easier. We don't sort or search the current lldb_private::Symbol objects since they aren't sorted, nor are the name indexes created until we are done with adding all symbols. Can we use a side map that is just something like: std::map<lldb::addr_t, ContString> SymbolMapType; > is that the logic you have in mind? `FindSymbolsByNameAndAddress` I would > need to implement. > >> BTW, if you want, I think you can submit the rest of the gnu_debugdata >> changes without waiting for this, if you just adjust the test expectations >> to account for the fact that symtab+dynsym merging does not work (yet). > > Let's wait first, okay? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
