kwk added a comment. 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); } 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
