[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. Thanks for the context. Fixed in https://reviews.llvm.org/D124499. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 ___ lldb-commits

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

jasonmolenda added a comment. Thanks! Yeah, I only noticed because lldb-dotest threw an error when I was trying to run a test case earlier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 __

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. @jasonmolenda, thanks, I discovered a bit late that testcases can't have duplicate names; I fixed one case in the patch but missed these two. Will add a follow-up patch. I would be great if we have a linter/script to detect/warn duplications, not sure how hard to add

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

jasonmolenda added a comment. Hi, I noticed that we have two tests with the same name, lldb/test/API/lang/c/shared_lib/TestSharedLib.py lldb/test/API/symbol_ondemand/shared_library/TestSharedLib.py Can you rename your newly added test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. Another patch to fix the missing import: https://reviews.llvm.org/D124479 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 ___ lldb-c

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. @stella.stamenova, thanks for the heads-up. I am fixing this in https://reviews.llvm.org/D124471. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 _

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

stella.stamenova added a comment. Looks like this broke the windows lldb bot: https://lab.llvm.org/buildbot/#/builders/83/builds/18228 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7b81192d462b: Introduce new symbol on-demand for debug info (authored by yinghuitan). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. LGTM. Anyone else have any issues? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 ___ lldb-commits mailing list lldb-commits@lists.ll

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 424808. yinghuitan marked 3 inline comments as done. yinghuitan added a comment. Incorporate feedback and update summary with follow-up todos. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https:

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan marked 8 inline comments as done. yinghuitan added a comment. Thanks for all the feedback. I will address the comments and add @clayborg's follow-up suggestions into summary. Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:47 +lldb::LanguageType SymbolFileOnDe

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. Thanks Pavel for taking the time to review this thoroughly. I am fine with this as I have been working with Jeffrey on this, so anyone else please chime in if you have any comments! A few follow up patches I would love to see after this goes in: - add a command that a

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

DavidSpickett added a comment. Bunch of random nits. Looks like a great addition. I know debug load time is a common annoyance. Comment at: lldb/docs/use/ondemand.rst:1 +On Demand Symbols += If you can find a logical place for it, maybe define

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. Thanks, for your patience. Overall, I'd say this looks pretty clean now -- much cleaner than I originally thought it could be. I don't have any further comments here. Does anyone else have any thoughts? Comment at: lldb/source/Core/Module.cpp:474 +

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 424360. yinghuitan added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/docs/use/ondemand.rst lldb/include/lldb/Core/ModuleList.

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 424318. yinghuitan added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/docs/use/ondemand.rst lldb/include/lldb/Core/ModuleList.

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 423974. yinghuitan added a comment. Rebase on top of https://reviews.llvm.org/D124110 Rename test file name to avoid conflict Added a new totalModuleCountHasDebugInfo statistics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. Yes, I think that's exactly what I had in mind. Basically, the idea is to structure things such that the ondemand class can sit between the actual symbol file class and the outside world, but that it does not (and cannot) interfere with any of the interactions that happe

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. Thanks for clarifying. The friendship declaration in SymbolFile is required not only for `SetCompileUnitAtIndex` but also for other protected virtual methods in `SymbolFile` like `CalculateNumCompileUnits` and `ParseCompileUnitAtIndex` which are pure virtual and requ

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. I am trying to get rid of the friendship declaration and the forwarding of protected methods. I have no issue with making `GetCompileUnitAtIndex` virtual -- that is pretty much a necessity for this feature (*). However, `SetCompileUnitAtIndex` seems like an internal impl

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. > Does this by any chance have something to do with the fact that there are now > two compile unit lists (one in the actual symbol file, and one in the > wrapping ondemand class? Yes, that's the major reason. We also need make SymbolFileOnDemand friend in SymbolFile

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. I'm looking at the SymbolFileOnDemand friendship and the forwarding of protected methods (mostly dealing with compile unit lists). Does this by any chance have something to do with the fact that there are now two compile unit lists (one in the actual symbol file, and one

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. So we added documentation on this feature explaining who will want to enable this feature and also describe exactly when and how debug info will get enabled. Does anyone else have objections or comments to make? If we need to setup a meeting to discuss, please chime in

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 419226. yinghuitan added a comment. Support debug map dwarf mode. Use zero symbol ability to skip SymbolFileOnDemand wrapping. Add new API tests for symbol on-demand which support multi-platform. Remove linux-only shell tests. Repository: rG LLVM Github

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added inline comments. Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4 + +# REQUIRES: system-linux + yinghuitan wrote: > labath wrote: > > yinghuitan wrote: > > > labath wrote: > > > > Is this here because there is no p

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added inline comments. Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4 + +# REQUIRES: system-linux + labath wrote: > yinghuitan wrote: > > labath wrote: > > > Is this here because there is no portable way to create

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added inline comments. Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4 + +# REQUIRES: system-linux + yinghuitan wrote: > labath wrote: > > Is this here because there is no portable way to create shared libraries in > >

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan marked an inline comment as done. yinghuitan added inline comments. Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4 + +# REQUIRES: system-linux + labath wrote: > Is this here because there is no portable way to crea

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. In D121631#3387077 , @clayborg wrote: > In D121631#3386244 , @labath wrote: > >> In D121631#3384124 , @yinghuitan >> wrote: >> unify with `ta

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 418708. yinghuitan added a comment. Fix log channel typo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/docs/use/ondemand.rst lldb/include/lldb/Co

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 418632. yinghuitan added a comment. Add documentation for the feature. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/docs/use/ondemand.rst lldb/i

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 417049. yinghuitan added a comment. Remove last ondemand test flavor testcase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/include/lldb/Core/Modu

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 417048. yinghuitan added a comment. Remove ondemand test category/flavor Add new testcases: - source text regex breakpoint - regex breakpoint filter by language - global variable symbol table match hydration Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. I will remove ondemand test flavor/category and add more new testcases next. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 ___ lld

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 416333. yinghuitan marked 4 inline comments as done. yinghuitan added a comment. Address review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files:

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan marked 19 inline comments as done. yinghuitan added inline comments. Comment at: lldb/source/Symbol/SymbolFileOnDemand.cpp:28 + +uint32_t SymbolFileOnDemand::CalculateAbilities() { + if (!this->m_debug_info_enabled) { clayborg wrote: > We might consid

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan added a comment. Re @labath > How many tests/bugs are we talking about here? Were there more than say ten > distinct bugs caught there? It found 2~3 bugs, like source regex breakpoint (breakpoint set -p "source"), regex breakpoint filter by language (breakpoint set -r -L c++) etc.

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/SymbolFileOnDemand.h:202-203 +private: + bool m_debug_info_enabled{false}; + bool m_preload_symbols{false}; + std::unique_ptr m_sym_file_impl; Comment at: lldb/source/Core

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. In D121631#3386244 , @labath wrote: > In D121631#3384124 , @yinghuitan > wrote: > >>> unify with `target.preload-symbols` feature >> >> I do not have strong opinion on this. One concern

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

JDevlieghere added a comment. In D121631#3386244 , @labath wrote: > In D121631#3384124 , @yinghuitan > wrote: > >>> unify with `target.preload-symbols` feature >> >> I do not have strong opinion on this. One conc

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. In D121631#3384124 , @yinghuitan wrote: >> unify with `target.preload-symbols` feature > > I do not have strong opinion on this. One concern is that > `target.preload-symbols` provides full experience but > `symbols.load-on-dema

Re: [Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

> On Mar 15, 2022, at 5:03 PM, Greg Clayton via Phabricator > wrote: > > clayborg added a comment. > > In D121631#3380824 , @jingham wrote: > >> This sort of change needs to do a lot more work to help the user understand >> what they can and can't s

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. I am personally not a fan of the preloading of symbols settings being on by default as there are large delays in debug session startup on linux due to the lack of any kind of viable acceleration tables. I understand that delays can occur later in the debug session as a

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. In D121631#3380824 , @jingham wrote: > This sort of change needs to do a lot more work to help the user understand > what they can and can't see if they accept the speed for visibility tradeoff. > > They (and we) also need to kn

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan marked 2 inline comments as done. yinghuitan added a comment. Jim, Pavel, thanks for the comment! I am happy to start a strategy discussion for this feature. @clayborg will chrime in later to suggest the approach how we can discuss. I would like to answer some of the questions here t

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

labath added a comment. I agree with everything Jim said. One of the interesting 'strategy' questions would be the interaction between this feature and the `target.preload-symbols` feature. I don't think the interaction between the two is obvious or intuitive, and I'm even sure that supporting

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 415252. yinghuitan added a comment. Remove duplicate file and fix wrapping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/D121631 Files: lldb/include/lldb/Core/ModuleL

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

jingham added a comment. This sort of change needs to do a lot more work to help the user understand what they can and can't see if they accept the speed for visibility tradeoff. They (and we) also need to know what operations might trigger a full "hydration" of a symbol table. It isn't useful

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

clayborg added a comment. I have been working with Jeffrey on this feature and wanted to explain our thoughts a bit. We wanted to solve the issue of having too much debug information in your project and having debugging being slowed down by all of this information. Much of this information is

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan updated this revision to Diff 415183. yinghuitan added a comment. Herald added a subscriber: JDevlieghere. Fix tests line number change after formatting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121631/new/ https://reviews.llvm.org/

[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

yinghuitan created this revision. yinghuitan added reviewers: clayborg, aadsm, jingham. Herald added subscribers: arphaman, mgorny. Herald added a project: All. yinghuitan requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: lldb-commits, sstefan1. Herald