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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

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