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

kwk abandoned this revision. kwk added a comment. Here's the relevant transcript from #lldb@otfc for why this change is abandoned. [10/02/19 15:22:25] labath: Is it acceptable for you? "BTW given how this unique-ness of symbols turns out to be non-trivial is it really needed? Because for

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

kwk planned changes to this revision. kwk added a comment. LLVM reasoning for why to go with `std::vector`: http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

kwk updated this revision to Diff 222634. kwk marked 10 inline comments as done. kwk added a comment. - Check that no additional symbols follow after the expected ones - Use compiler-generated copy-ctor - Cleanup from experiment - Simplify NamedELFSymbol::hash() - Cleanup Repository: rG LLVM

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

kwk added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:383 + std::hash()(st_name_string.AsCString()), + std::hash()(st_section_name_string.AsCString())); +} jankratochvil wrote: > llvm::hash_combine already calls

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

jankratochvil added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:383 + std::hash()(st_name_string.AsCString()), + std::hash()(st_section_name_string.AsCString())); +} llvm::hash_combine already calls std::hash for each

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

labath accepted this revision. labath added a comment. Ok, let's give this one more shot. Thanks for your patience. I do have a couple of additional comments inline, but I don't think we need another round of review for those. Comment at:

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

kwk updated this revision to Diff 222584. kwk added a comment. - Simplify NamedELFSymbol::hash() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/load-from-dynsym-alone.c

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

kwk updated this revision to Diff 222577. kwk added a comment. - Fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/load-from-dynsym-alone.c

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

kwk updated this revision to Diff 222576. kwk added a comment. - Correct comment of test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/load-from-dynsym-alone.c

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

kwk marked an inline comment as done. kwk added a comment. @labath can you please check this patch one last time (hopefully)? Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373 + r.st_name = st_name; + return elf::ELFSymbol::operator==(r) && +

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

kwk updated this revision to Diff 222569. kwk marked 8 inline comments as done. kwk added a comment. - Remove verbose output in test - Fix typo: smymtab -> symtab - Move compare and hash logic out of base class into derived class as requested Repository: rG LLVM Github Monorepo CHANGES SINCE

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

labath added inline comments. Comment at: lldb/lit/Modules/ELF/merge-symbols.yaml:51 +# Symbol all_same2 will be renamed to all_same1 and should therefore +# disappear from the dumped smymtab because all fields are equal. + - Name:all_same2

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

kwk updated this revision to Diff 222479. kwk added a comment. - Fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/load-from-dynsym-alone.c

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

kwk updated this revision to Diff 222441. kwk added a comment. - include test code in .c test file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/load-from-dynsym-alone.c

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

kwk updated this revision to Diff 222427. kwk added a comment. - Added YAML test to merge symbols Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

kwk added a comment. @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

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

kwk updated this revision to Diff 222387. kwk marked an inline comment as done. kwk added a comment. - typo: dynmic -> dynamic - Applied changes requested in review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/

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

kwk marked 8 inline comments as done. kwk added a comment. 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

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

labath added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2205 +needle.st_section_name_string = ConstString(symbol_section_sp->GetName()); +if (unique_elf_symbols.find(needle) == unique_elf_symbols.end()) { +

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

jankratochvil added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:446 +std::size_t h2 = std::hash()(s.st_name_string.AsCString()); +std::size_t h3 = std::hash()(s.st_section_name_string.AsCString()); +return llvm::hash_combine(h1, h2,

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

labath added a comment. 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

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

kwk added a comment. Interesting. It looks like we indeed have a test (the only one failing atm.) that wants a symbol to be added twice: [ RUN ] MangledTest.NameIndexes_FindFunctionSymbols /home/kkleine/llvm/lldb/unittests/Core/MangledTest.cpp:186: Failure Expected: 1 To be

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

kwk updated this revision to Diff 222080. kwk marked an inline comment as done. kwk added a comment. - Use symbol name including @VERSION suffix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

kwk marked 9 inline comments as done. kwk added a comment. 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.

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

kwk updated this revision to Diff 222076. kwk added a comment. - make the section name part of NamedELFSymbol Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

kwk added a comment. Not all is answered now but please respect: https://reviews.llvm.org/D67390#1683705 Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373 + r.st_name = st_name; + return elf::ELFSymbol::operator==(r) && + st_name_string ==

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

kwk updated this revision to Diff 222075. kwk marked 10 inline comments as done. kwk added a comment. - Change order of compare members to match order of member definitions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/

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

kwk updated this revision to Diff 222073. kwk added a comment. - Cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c

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

kwk updated this revision to Diff 222029. kwk added a comment. - Cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c

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

clayborg added a comment. So overall approach is good. See inline comments for issue and questions. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373 + r.st_name = st_name; + return elf::ELFSymbol::operator==(r) && + st_name_string ==

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

kwk updated this revision to Diff 221968. kwk added a comment. - Adjust other occurrence of AddSymbol where ELF plays a role - Working tests - Use unordered_set for storing unique elf symbols Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/

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

kwk added a comment. Please wait before reviewing this patch again. I will let you know when things do work. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390

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

jankratochvil added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h:276 + st_size == rhs.st_size && st_other == rhs.st_other && + st_shndx == rhs.st_shndx && st_value == rhs.st_value; + } It could be in the same

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

kwk updated this revision to Diff 221690. kwk added a comment. - Revert "Change test expectation to find 2 instead of 1 symbol" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

kwk updated this revision to Diff 221689. kwk added a comment. - [LLDB][ELF] Fixup for comments in D67390 - Change test expectation to find 2 instead of 1 symbol - symbol uniqueness by using elf::ELFSymbol Repository: rG LLVM Github Monorepo CHANGES SINCE

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

jankratochvil added a comment. In D67390#1681034 , @kwk wrote: > I wonder how to define uniqueness for them. As you can see, the only > difference is the symbol section which wasn't part of your definition of > uniqueness (yet). These symbols should

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

kwk added a comment. @clayborg @labath I'm still trying to only add symbols when they are unique. Take this already existing test: ./bin/llvm-lit -avv ~/llvm-project/lldb/lit/SymbolFile/DWARF/debug-types-line-tables.s The symbols that are being added at the end of

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

kwk added a comment. @clayborg are you on IRC? 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

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

clayborg added a comment. In D67390#1672254 , @labath wrote: > In D67390#1672210 , @kwk wrote: > > > So my point of this whole question is: What makes a symbol unique in the > > sense that it shouldn't be added to

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

clayborg added a comment. In D67390#1672210 , @kwk wrote: > @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

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

labath added a comment. In D67390#1672210 , @kwk wrote: > So my point of this whole question is: What makes a symbol unique in the > sense that it shouldn't be added to the symtab if it is already there? A symbol name is not unique because you can have

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

kwk added a comment. @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`

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

clayborg added a comment. In D67390#1671463 , @kwk wrote: > @clayborg what address is it exactly to store here `std::map ContString> SymbolMapType;`? As an example > `dc_symbol.GetAddress().GetFileAddress()` is something that would work but as > soon as

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

clayborg added a comment. In D67390#1671463 , @kwk wrote: > @clayborg what address is it exactly to store here `std::map ContString> SymbolMapType;`? As an example > `dc_symbol.GetAddress().GetFileAddress()` is something that would work but as > soon as

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

kwk added a comment. @clayborg what address is it exactly to store here `std::map 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

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

clayborg added a comment. In D67390#1671128 , @kwk wrote: > In D67390#1671018 , @labath wrote: > > > In D67390#1667270 , @kwk wrote: > > > > > @labath how shall we go about

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

clayborg added a comment. 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

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

kwk added a comment. 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

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

labath added a comment. 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

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

kwk updated this revision to Diff 220273. kwk added a comment. - [LLDB][ELF] Fixup for comments in D67390 - Change test expectation to find 2 instead of 1 symbol Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

kwk added a comment. @stella.stamenova I've reverted this patch and thanks for letting me know. @labath I didn't notice that tests failed, I'm really sorry. @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`

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

labath added inline comments. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2668-2671 + if (!m_symtab_up) { +auto sec = symtab ? symtab : dynsym; +m_symtab_up.reset(new Symtab(sec->GetObjectFile())); + } kwk wrote: >

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

labath added a comment. It broke on linux too. You did run the tests before committing, did you? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 ___ lldb-commits mailing list

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

stella.stamenova added a comment. This broke the windows bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/8741 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390

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

kwk marked 3 inline comments as done. kwk added a comment. @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 Comment at:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL371599: [LLDB][ELF] Load both, .symtab and .dynsym sections (authored by kwk, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

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

kwk updated this revision to Diff 219677. kwk added a comment. Updated commit message and squashed commits into one rebased onto master. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

labath accepted this revision. labath added a comment. lgtm. The reason I requested this to be put separately, is because it is implemented in a way that kicks in even without minidebuginfo. I think this is fine, because entries can be removed from the symtab even without the whole

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

kwk updated this revision to Diff 219605. kwk added a comment. - Rephrase comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files: lldb/lit/Modules/ELF/Inputs/load-from-dynsym-alone.c

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

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks! This LGTM with the comment on line 2660 rephrased and the motivation as part of the summary/commit message. Comment at:

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

kwk added a comment. Fixed the comment as per request. 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] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

kwk updated this revision to Diff 219594. kwk marked an inline comment as done. kwk added a comment. - update comment for .symtab section with minidebuginfo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390

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

kwk marked an inline comment as done. kwk added a comment. @JDevlieghere I change the support tool. It was @labath who requested (D66791#inline-601050 ) to outsource this patch. In that change you can find the whole reason which is somewhat more

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

kwk updated this revision to Diff 219587. kwk added a comment. - Added llvm-strip to the list of support tools Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67390/new/ https://reviews.llvm.org/D67390 Files:

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

JDevlieghere added inline comments. Comment at: lldb/lit/Modules/ELF/load-from-dynsym-alone.test:24 +# Remove functionInDynsym symbol from .symtab (will leave symbol in .dynsym intact) +# RUN: llvm-strip --strip-symbol=functionInDynsym %t.binary + Please also

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

kwk created this revision. kwk added a reviewer: labath. Herald added subscribers: lldb-commits, MaskRay, arichardson, emaste. Herald added a reviewer: espindola. Herald added a reviewer: alexshap. Herald added a project: LLDB. This change ensures that the .dynsym section will be parsed even when