[Lldb-commits] [PATCH] D131328: [lldb] Support fetching symbols with dsymForUUID in the background

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131328#3706565 <https://reviews.llvm.org/D131328#3706565>, @JDevlieghere wrote:

> In D131328#3706202 <https://reviews.llvm.org/D131328#3706202>, @labath wrote:
>
>> I see two problems with this patch.
>>
>> - it makes shutting down lldb safely impossible (because you never know if there are any unfinished tasks running). We already have problems with shutting down (grep for "intentionally leaked"), but this is guaranteed to make the problem worse. Using the global thread pool would make things better, as then one could flush the pool in SBDebugger::Terminate, or something similar
>
> It *is* using the global thread pool, but it doesn't look like that's being flushed. I can rework that in a separate patch to make it fit the Initialize/Terminate pattern.

Ok, I'm sorry. I got confused by the `static ThreadPoolTaskGroup` thingy. In either case, I think we should flush that pool during termination. The reason we didn't need that before is because this is the first time that we have these sorts of fully async tasks (the other task pool tasks are "flushing" themselves).

>> - there seems to be a race between new debuggers/targets being added, and their existing instances being notified. I'm not 100% sure what will happen in this case, but it seems very easy to miss some debuggers (or targets) that were added after we took the "snapshot" of the current state
>
> As I (tried to) explain in the comment, since we set the symbol file in the module before taking the "snapshot", I don't think this is a problem. If a target was created after, it should already know about the symbol file. Did I miss something with that approach?

Right. I missed that part. I think this should be mostly fine. I mean, I suppose this can still race in the other "direction", where a target picks up the new symbol file, but we still end up calling SymbolsDidLoad manually. But hopefully that is ok (?)

>> (There's also the (obvious) problem of any breakpoints that are set on code within those modules becoming effective at an unpredictable future data, but I am assuming you are aware of that.)
>
> Yeah, but I don't see any way around that. The unpredictability is unfortunate, but from UX perspective it's not much worse than modules getting loaded at runtime (although hopefully that's more deterministic).

Yeah, if that's the thing you want, then that's the thing you want. One could play some UX games, and add a mechanism to show the downloads that are in progress and/or force the completion of loading of some symbols of whatever, but there's no way to get rid of that completely.


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

https://reviews.llvm.org/D131328