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

Mon Sep 16 02:27:29 PDT 2019

labath added a comment.

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.

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

================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py:276
             matching=False,
-            patterns=["1 match found"])
+            patterns=["2 matches found"])

----------------
You can't do that because this will have only two matches on elf platforms. Since we don't really care about the number here. I suggest finding a string that can be grepped for, which does not depend on the number of matches (maybe just `a_function`).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67390