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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D121631#3387077 <https://reviews.llvm.org/D121631#3387077>, @clayborg wrote:

> In D121631#3386244 <https://reviews.llvm.org/D121631#3386244>, @labath wrote:
>
>> In D121631#3384124 <https://reviews.llvm.org/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-demand` provides trade-off experience (for example some global expression evaluation may fail because we won't automatically hydrate based on type query).
>>
>> That is a fair point, but the having them separate also has downsides, as we have to figure out what is the meaning of on-demand=true&preload=true combo going to be. I think it would be strange to have on-demand take precedence in this setup, but then if it doesn't, then you need to change *two* settings to switch from "preload everything" (the current default) to the "on demand mode". I am not particularly invested in any solution, but I think we should make a conscious choice one way or the other.
>
> These two settings can co-exist pretty easily. If preload is on, preloading can be done for anything that has debug info enabled, and would be done as soon as debug info was enabled for on demand. It would be like saying "please preload symbols as soon as debug info is enabled". Right now if we enable debug info due to a breakpoint, function, or global lookup that matches a symbol, we can immediately preload the symbols so they are ready regardless of wether a name lookup happens just like we do normally.
>
> If preload is off, then we wait until someone makes a global name lookup call.

I understand your point, and I see how having a ternary setting where (exactly) one of the values creates a completely different debug experience could be confusing. When you describe it like this, I think the behavior makes sense. The part I am worried about is someone getting the relationship backwards (with preload-symbols being "on top"), and then wondering "why the %#@# is aren't my symbols loaded when I said you should preload them". And I do think one could think this is the expected behavior, since the (whether you like it or not) idea behind the preloading was to load everything up-front instead of at a random point in the debug session -- something which is no longer true when the on-demand feature is enabled.

Overall, I am still fairly fond of the ternary idea, but I am not going to push for it if there's interest in it. Having two settings makes sense too, if we can clearly communicate their relationship. One thing that might help is renaming (one or both) something so that both settings aren't using the word "load" (I'll note that you used the phrase "debug info is enabled" in your exposition), though I am not sure what would those names be.

In D121631#3397355 <https://reviews.llvm.org/D121631#3397355>, @yinghuitan wrote:

> Remove ondemand test category/flavor
> Add new testcases:
>
> - source text regex breakpoint
> - regex breakpoint filter by language
> - global variable symbol table match hydration

Awesome. Thanks.



================
Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+
----------------
Is this here because there is no portable way to create shared libraries in shell tests? Would it be possible to run this test on other systems as well if this were written as an API test (where we have a shared lib portability layer)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121631/new/

https://reviews.llvm.org/D121631