Why don't we always print the target index? Wouldn't that be much simpler?

Addressed all review comments.

• Review comments

• Fixed logic error

Running tests locally.

Reopening because I did find the logic error.

• Have ResolveSymbolContext not return anything

In D70774#1761444, @teemperor wrote:

Let's just remove that return value. It's anyway only used in one place or so where we can just call GetSize() manually. Then we also don't need the lambda. Otherwise this LGTM, thanks!

Small changes

Are there no tests for this code?

LGTM with the exception of a local variable name.

@grimar @labath tests did pass locally. Are you fine with this change then?

Still tests are running locally...

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"