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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to