[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D97739#2608510 <https://reviews.llvm.org/D97739#2608510>, @friss wrote:

> In D97739#2607961 <https://reviews.llvm.org/D97739#2607961>, @clayborg wrote:
>
>> In D97739#2607869 <https://reviews.llvm.org/D97739#2607869>, @jingham wrote:
>>
>>> This way of doing progress is going to look odd in anything that uses multiple debuggers.  If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity.
>>
>> that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.
>
> Just one tidbit here. Most users are actually routinely running tens of debuggers at the same time, because tests run in parallel and they have a debugger attached by default. Now if you have a long running operation kick in in your unit tests, you might already have a different kind of issue, but I’d like to avoid a world where the IDE displays spurious and wrong information because of this.

But you wouldn't actually hookup any progress callbacks on any of these debuggers right? You don't make a window for each test, IIRC you are only running it under the debugger so you can report issues by using the debugger. Am I remember this correctly? What happens when a test crashes? If you are running 100 tests in parallel and 10 of them crash, do you open a window for each one that does crash? Or do you manually have to debug the test in order to stop at breakpoints or at a crash?

If we truly need debugger specific task progress, that is a lot more work and we have no great solution. One idea is we could end up having progress items take a SymbolContextScope pointer as an optional parameter that would allow us to grab the module pointer from it and then ask the debugger if any targets contain this module prior to reporting progress. This would be a bit expensive code for a lot of quick progress updates as we would need to iterate over each target and each target's module list and we would still have many system libraries reporting progress to all debuggers when targets contain the same system libraries.

The threading overhead and expense of delivering SBEvents also seems like overkill as threading itself and waiting using a mutex + condition will slow down progress delivery especially if we have a progress that does a bunch of updates. And once we receive the event we will need to make a static function call to extract all progress variables (total, completed, progress_id, baton, etc).

In D97739#2608510 <https://reviews.llvm.org/D97739#2608510>, @friss wrote:

> In D97739#2607961 <https://reviews.llvm.org/D97739#2607961>, @clayborg wrote:
>
>> In D97739#2607869 <https://reviews.llvm.org/D97739#2607869>, @jingham wrote:
>>
>>> This way of doing progress is going to look odd in anything that uses multiple debuggers.  If I'm reading the code aright, if I have two debuggers, and a target in Debugger A starts doing something that would cause progress traffic, both debuggers will show activity.
>>
>> that is true, but it is a global module repository that benefits both debuggers. And I very rarely debug two things at the same time, so most of the time for most people this will be beneficial and shouldn't cause too much confusion.
>
> Just one tidbit here. Most users are actually routinely running tens of debuggers at the same time, because tests run in parallel and they have a debugger attached by default. Now if you have a long running operation kick in in your unit tests, you might already have a different kind of issue, but I’d like to avoid a world where the IDE displays spurious and wrong information because of this.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739