In D69041#1712493, @MaskRay wrote:

In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

@kwk Sorry if you feel your work is commandeered. On D68943, it was me who said

Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).

I skimmed through your yaml2obj changes in D68943 and I believed a complete fix needs more work than your change. check-llvm-tools check-llvm-object should expose more tests that you missed.

Abandoning for D69041

In D69041#1712432, @grimar wrote:

In D69041#1711707, @kwk wrote:

@grimar I don't understand why you created this patch, when I'm working on it already.

I only wanted to help, because supposed that the correct direction is not clear for you (since your version did not fix any of existent test cases that were failing,
obj2yaml tool did not compile and there was at least one obliously wrong place in 'initSymtabSectionHeader' that broke the normal logic).
And it was also not fully clear to me what is exactly needed until I run check-llvm with the first more or less stable version of the patch at least.

Now when the whole picture is revealed, feel free to commandeer this revision or use it as a reference for your patch if you would like to continue it.
Or let me continue. Please let me know about your decision, I'll put this on hold until that.

@grimar I don't understand why you created this patch, when I'm working on it already. That's a waste of my time.

I noticed that the order of sections does matter to some tests. Before the .symtab section was always the first one and now it is the last of the implicit sections being created. Some Link: X checks break because X points to a different section. In order to compensate for this I will make .symtab the first of the implicit sections again. In my local testing this already helps a bit. In cases where we refer to a section by number and expect .symtab to be the section being referenced, I have two options: 1st) create a symbol to force .symtab to be implicitly created or 2nd) change the name of the expected section. Just tell me what you want.

• Remove unnecessary section
• Have consitent ordering of run, check and YAML lines
• use ## for comments in YAML
• Remove not needed Entry and Sections
• Only have Name for a symbol
• Moved test files from llvm/test/ObjectYAML/ELF/ to llvm/test/tools/yaml2obj/
• Fixup for test files

@grimar I've tackled the most obvious of your comments. I will now see what fails in llvm/test/tools/yaml2obj.

In D68943#1708068, @MaskRay wrote:

This is not correct. A lot of yaml2obj tests fail with:

yaml2obj: error: unknown section referenced: '.symtab' by YAML section '.rela.text'

Moved yaml2obj tests under llvm subtree.

• moved test files over to llvm subtree

Move tests over to llvm/llvm/test/ObjectYAML/ELF/.

• restore formatting

• Silence FileCheck in test

Fixup

I think I'm guilty for adding llvm-strip to LLDB_TEST_DEPS I wasn't aware that it might cause problems.

Closed by 2c082b48274fcba62bf9b3acb63075aedcc7a976.

rebased

@labath as much as I would love to merge this patch I get errors from these tests that I didn't see before. They only appear after rebasing onto master:

Rebased onto current master

@labath ah, I've just seen that you did accept the revision already. Will merge it then after running tests once more.

@labath I've addressed all you comments. Thank you for answering @jankratochvil's question as well. If this patch is good to go now, please give a thumbs up.

• Fix typo: s/a. /a ./
• Fix comment header
• Remove redundant input buffer check
• Remove another redundant input buffer check
• Change variable initialization to assign
• added comment about minidebuginfo

Tests did pass so this change is ready for review @labath @jankratochvil .

• Change logic and comment for when .dynsym is parsed
• Use different pattern for error checking
• use less auto
• Remove required system-linux test feature from LZMA tests

Here's the relevant transcript from #lldb@otfc for why this change is abandoned.

LLVM reasoning for why to go with std::vector: http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc.

• Check that no additional symbols follow after the expected ones
• Use compiler-generated copy-ctor
• Cleanup from experiment
• Simplify NamedELFSymbol::hash()
• Cleanup

• Simplify NamedELFSymbol::hash()

• Fix comment

• Correct comment of test

@labath can you please check this patch one last time (hopefully)?

• Remove verbose output in test
• Fix typo: smymtab -> symtab
• Move compare and hash logic out of base class into derived class as requested

• Fix comment

• include test code in .c test file

• Added YAML test to merge symbols

@labath I did prepare some YAML file but apparently yaml2obj isn't meant to deal with this properly. Instead I get an Error like this: yaml2obj: error: repeated symbol name: 'main'. It looks like symbols from the Symbols: part of the YAML file are just added by name to a map. Changing yaml2obj for this seems a bit too heavy right now. If you're okay I'll go with a few more c programs if I can pull them off.

• typo: dynmic -> dynamic
• Applied changes requested in review

In D67390#1685484, @labath wrote:

This looks fairly ok to me, but it could use a more explicit test of the symbol uniqueing code. Right, now I believe the two tests you added check that the symbols are _not_ uniqued. (They're also a bit hard to follow due to the whole

objcopy business). Could you create a test with a yaml file which will contain various edge cases relevant to this code. I'm thinking of stuff like "a dynsym and a symtab symbol at the same address, but a different name", "a dynsym and symtab symbols with identical names, but different addresses", etc. Then just run "image dump symtab" on that file to check what we have parsed?

Interesting. It looks like we indeed have a test (the only one failing atm.) that wants a symbol to be added twice:

• Use symbol name including @VERSION suffix.

I think I've finished the implementation now and should have answered all your comments and concerns. I run tests now. I would appreciate if you (@clayborg , @labath , @jankratochvil ) can take a look at this patch again.

• make the section name part of NamedELFSymbol

Not all is answered now but please respect: https://reviews.llvm.org/D67390#1683705

• Change order of compare members to match order of member definitions.

• Cleanup

• Cleanup

• Adjust other occurrence of AddSymbol where ELF plays a role
• Working tests
• Use unordered_set for storing unique elf symbols

Please wait before reviewing this patch again. I will let you know when things do work.

• Revert "Change test expectation to find 2 instead of 1 symbol"

• [LLDB][ELF] Fixup for comments in D67390
• Change test expectation to find 2 instead of 1 symbol
• symbol uniqueness by using elf::ELFSymbol

@clayborg @labath I'm still trying to only add symbols when they are unique.

@jankratochvil the file only has two revisions: https://github.com/llvm/llvm-project/commits/b9c1b51e45b845debb76d8658edabca70ca56079/lldb/include/lldb/Core/LoadedModuleInfoList.h

@clayborg are you on IRC?

@clayborg thank you for this explanation. My patch for minidebuginfo is already done in D66791 . But this one here I was asked to pull out as a separate patch. For good reasons as we see. I wonder how this extra parameter SymbolMapType of yours does help. In the end I have to identify duplicates. But if no symbol with the same name should be added then why do I care about where the symbol is coming from?

@clayborg what address is it exactly to store here std::map<lldb::addr_t, ContString> SymbolMapType;? As an example dc_symbol.GetAddress().GetFileAddress() is something that would work but as soon as we have minidebuginfo, then we might end having the same symbol coming from two different object files and so their address would still be different. Also do you want me to keep this map in ObjectFileELF?

In D67390#1671018, @labath wrote:

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

• [LLDB][ELF] Fixup for comments in D67390
• Change test expectation to find 2 instead of 1 symbol

@stella.stamenova I've reverted this patch and thanks for letting me know.

• Addressed review comments

Rebased on master to include changes from D67390

@labath I've addressed your comment rewrites in a fixup commit that I've commited without a review (llvm-svn: 371600): https://reviews.llvm.org/rLLDB371600