[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

Mon Sep 16 07:09:30 PDT 2019

clayborg 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.

You should ensure that there aren't multiple symbols for the same address. Mach-o has this problem with the STAB symbols and the standard symbol table symbols. That plug-in will unique them into one using the STAB symbol table index as the lldb::user_id_t.

> 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).

We really want to present the simplest and best view of the module to the the user. So please do only create one symbol, no matter how many sources we have.

> 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.

Code to unique symbols isn't that hard to do during the symbol table parsing. Thought don't keep re-sorting the lldb_private::Symbol objects, just keep a side map that has file address to symbol name. I am guessing it will be easier than mach-o because there the different symbols have different info (STAB vs normal symbol table symbol) where as in ELF they should be the same.

> 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).

Love to see this fixed prior to checkin in possible

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390