This is an archive of the discontinued LLVM Phabricator instance.

Add a progress class that can track and report long running operations that happen in LLDB.
ClosedPublic

Authored by clayborg on Mar 1 2021, 3:32 PM.

Details

Summary

LLDB can often appear deadlocked to users that use IDEs when it is indexing DWARF, or parsing symbol tables. These long running operations can make a debug session appear to be doing nothing even though a lot of work is going on inside LLDB. This patch adds a public API to install a progress callback that will allow IDEs and other tools to create an activity window that can show users what is going on and keep them informed of expensive operations that are going on inside LLDB.

Diff Detail

Event Timeline

clayborg created this revision.Mar 1 2021, 3:32 PM
clayborg requested review of this revision.Mar 1 2021, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 3:32 PM

So this is a first pass on adding progress notifications to the LLDB public API, and to see how it would be used internally. If we can agree on the implementation, then I will add full tests and documentation to this diff. We used a similar patch to this in an older internal version of lldb-vscode before lldb-vscode was fully open sourced, so we know it works at a base level.

Feel free to comment with suggestions or other issues you see and I will iterate on the design until everyone is happy.

clayborg retitled this revision from Add a progress class that can track long running operations in LLDB. to Add a progress class that can track and report long running operations that happen in LLDB..Mar 1 2021, 3:37 PM

The current Progress class allows users to specify how many items are to be done when they construct a lldb_private::Progress object, but this part is not used right now for anything yet because it is often hard to know how many items of work there are. For symbol tables in ELF, we have two of them, but we might also create new symbols from EH frame or other sources when there are no symbols, so it was hard to pre-determine a full number of symbols for a given executable. Same goes for mach-o files with the normal symbol table, then new symbols are created from the LC_FUNCTION_STARTS load command that has start addresses for all functions, many of which have symbols. For DWARF, we have 3 ways to index the DWARF: Apple tables, .debug_names, and manually. I will comment inline where we could specify a valid number of work items for DWARF to improve the feedback. The other reason the progress objects don't report immediate feedback is the frequency of the progress callbacks could slow things down if they are called too often with too little work. I am happy to talk about any of these issues if anyone has any questions.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
471–473

Instead of specifying UINT64_MAX as the number of work items here, we can figure out how many compile units exist in the DWARF and use that number, and then a reference to this progress object can be passed into the various indexing calls below and can be updated as the DWARF is parsed.

shafik added a subscriber: shafik.Mar 1 2021, 3:45 PM
shafik added inline comments.
lldb/include/lldb/Core/Progress.h
34

Maybe I am misunderstanding but if we are going to have a total are we also going to variable to keep track of the expected total number of steps we have in the complete progress?

clayborg added inline comments.Mar 1 2021, 3:49 PM
lldb/include/lldb/Core/Progress.h
34

Users currently have two options when creating a Progress object:

  • accurately specify the "uint64_t total" in the constructor, and then call Progress::Increment() as needed.
  • just specify any number (UINT64_MAX is what all current code example do) and don't bother calling Progress::Increment()

Either way the Progress destructor will always report the progress with the value of "m_total" when the destructor is called. This is what all of the current code examples rely on. See my other comments as to why it is tricky to get an accurate total count for the various long running tasks.

clayborg added a comment.EditedMar 1 2021, 3:56 PM

Another thing to clarify: The current design of the Progress objects, when destructed, will report progress with the stored "m_total" value to indicate that the progress is complete. Even if exceptions are thrown, the destructor will still be called. So all current uses of the "Progress" objects, will currently report progress when constructed with a "completed" value of zero:

ProgressCallback(message = "Indexing DWARF for /tmp/a.out", completed = 0, total = UINT64_MAX, baton = <baton>);

Then when the Progress object is destroyed, we will report a completed value that is equal to the "Progress::m_total" to indicate things are done:

ProgressCallback(message = "Indexing DWARF for /tmp/a.out", completed = UINT64_MAX, total = UINT64_MAX, baton = <baton>);

The idea is when the code that installs the progress callback gets its callback called, and if the "completed == total" then the UI can remove the progress indicator for "<message>". The message string is a const string and will be unique and the same pointer value for the message will always be used, so the progress is keyed off of the message pointer value.

Very cool, this is something I have wanted for a long time. Another really good use case here would be dSYMForUUID which can take such a long time that people think the debugger hangs. I think this will be really powerful on the command line too if we can hook this up with a little progress bar that appears and disappears during long running tasks. I left a few comments inline.

lldb/include/lldb/Core/Progress.h
2

I think this belongs more in Utility than Core.

20

I'm not a fan of how we'rere-implementing printf across different utility classes. Can we have this take std::string (that we move into the member) and have the caller use formatv or something to construct the string?

25

Rather than making this a static property of the Progress class, maybe this should be a member of the debugger? And then you could have a factory in the debugger CreateProgress that hands out a new Progress instance. The reason I'd like to have this tied to the debugger is that I imagine using this to show a little progress bar on the command line. When we do that we can have the debugger wrap its own callback around the one provided by the user instead of having to hijack it. It would also align with where the function is exposed in the SB API.

31

What do you imagine this baton to be used for?

33

Why not uint64_t?

clayborg added inline comments.Mar 1 2021, 5:34 PM
lldb/include/lldb/Core/Progress.h
2

I can move this. It used to rely on stuff in Debugger.h/Debugger.cpp as I was iterating on it, but I removed that requirement, so yes, utility would work fine.

20

Were are not implementing printf, just forwarding to a var args. If we switch over to anything we should switch to ConstString since we report the same ConstString on each callback.

25

I too wanted this, but Module and ObjectFile stuff isn't tied to a specific debugger since all modules within one LLDB process shared the same global module list. So at all of the current call sites where we report progress, there is no access to a debugger since they aren't unique to one.

So given that we have no access to a debugger at any of the current progress call sites, we can't use a CreateProgress factory, and it removes the need for this to be part of the debugger IMHO. I did start off with this being in Debugger.h and Debugger.cpp, but since it ends up just calling a callback, it isn't needed if we can't actually have progress dialogs that are tied to a specific debugger. So we expose it as part of the debugger at the SB API level, but it isn't tied to it internally.

One idea is that the progress dialogs _could_ include a "debugger_id" field that is optionally set, but since the main pain points: DWARF indexing, symbol table parsing, and symbol fetching isn't tied to a debugger, it seems like overkill.

Let me know what your thoughts are.

31

It is common for any callback to also have a baton in case you have a class that was instantiated for the progress reporting, you can specify this the baton as being the class pointer so you can re-use it. We have this type of thing in all other callbacks.

33

Yes, will fix!

clayborg added inline comments.Mar 1 2021, 5:35 PM
lldb/include/lldb/Core/Progress.h
20

If we do switch to "ConstString message" can you add inline code suggestions for using formatv for one of the Progress constructors to see how it would look?

I haven't thought about the details of this implementation closely yet. How will this handled nested progress bars? For instance, if I'm Indexing (maybe even on multiple threads) and then one of the threads results in a debug symbols fetch request (e.g. dsymForUUID). Is the consumer just supposed to check the string that comes into the progress callback to match the now two simultaneous progress elements?

I haven't thought about the details of this implementation closely yet. How will this handled nested progress bars? For instance, if I'm Indexing (maybe even on multiple threads) and then one of the threads results in a debug symbols fetch request (e.g. dsymForUUID). Is the consumer just supposed to check the string that comes into the progress callback to match the now two simultaneous progress elements?

Nesting doesn't matter. I am in the process of adding a "uint64_t progress_id;" to each progress class that will be included in the callback. This will allow as many progress activities to be created from as many threads as needed. All activities will show up as top level activities. So the user will use the "uint64_t progress_id" as the unique indicator. This allows the same message to be used for multiple different progress indicators to avoid potential conflict if the user uses too simple of a string. I am about to post an update. Check the Progress.h header doc once I update this patch, hopefully this will answer any questions you might have.

clayborg updated this revision to Diff 327339.Mar 1 2021, 6:34 PM

Added a "uint64_t progress_id;" as a member variable of the lldb_private::Progress class to help users track individual progress dialogs without conflict. Prior to this the message was assumed to be unique.

Added code to ensure that we only report one update there "completed == total". This allows clients that install the callbacks to ensure they will only receive one completion event.

Added code the ensure we don't increment past the total amount and also can't run into unsigned overflow conditions that could have resulted in sending an update where the new "completed" amount was less than before.

Added header doc to explain the usage of the progress class.

Forgot to mention I added a lldb-vscode implementation after someone told me they now support progress reporting in Visual Studio Code. See https://microsoft.github.io/debug-adapter-protocol/specification#Events_ProgressStart

clayborg updated this revision to Diff 327357.Mar 1 2021, 10:00 PM

Added a more granular progress when manaully indexing the DWARF where we report on unit of progress for parsing all dies in a unit, and one unit of progress for indexing each compile unit, and one unit of work when merging the IndexSets.

Improved the filename reported to use the module's description so that the progress shows archive names + object file ("libfoo.a(bar.o)") instead of just the archive name.

This is fully hooked up and working in Visual Studio Code now! Works well. The Visual Studio Code shows any progress that takes more than 0.5 seconds down in the bottom toolbar for me. We can and should discuss these changes now that they are working.

teemperor requested changes to this revision.Mar 2 2021, 1:00 AM
teemperor added a subscriber: teemperor.

Thanks for working on this. I really like the idea and I think this would be really cool to have. I do have some concerns about the way this is implemented though (see inline comments).

lldb/include/lldb/Core/Progress.h
20

I don't see why we even need formatv for the current examples which just concatenate two strings. I also don't see why the constructor needs to do any formatting in the first place.

FWIW, with formatv this could be done like this:

Progress bla(3423, llvm::formatv("Indexing file {0}", path))

Or, as this is also just concatenating two strings:

Progress bla(3423, "Indexing file " + path)

What's even better about this though is that we can move the total parameter to the end where we can just make it optional (and even better, a real llvm::Optional that defaults to llvm::None instead of a magic UINT64_MAX value).

101

std::string

lldb/source/Core/Module.cpp
1076

I want to ask why but I fear the answer.

lldb/source/Core/Progress.cpp
19

I would really like to avoid the global state that is introduced here, but I assume the idea here is to avoid having to pass around a Debugger object to all parts of LLDB (especially the ones which aren't tied to a Debugger).

Anyway, a few comments on the current approach:

  • One callback is rather limiting. If I have an IDE with a progress bar that has an embedded LLDB command line, then I guess both want to subscribe to progress events? Right now the one that starts later will steal the progress notifications from whatever client has subscribed earlier.
  • This needs to be made thread-safe (right now I can call lldb::SBDebugger::SetProgressCallback and just snitch away g_callback in the middle of printing the progress -> crash).

As we probably end up with some kind of global state object that manages this stuff, I would also suggest we put this into the normal subsystem init/deinit infrastructure (i.e., give it a Initialize/Terminate call). This way this won't be another "intentional leak" and we can properly use this in unit tests (where we can init/deinit subsystems and cleanup this state automatically).

40

So, just to be clear what's going on here: This is the function we expect several 'working' threads to call? From the current uses in ManualDWARFIndex that seems to be the case, but I don't see how this will work out.

  1. Whoever receives the "progress report" callback needs to be ready to handle receiving multiple events from multiple threads. The VSCode callback isn't thread safe from what I can see and user-callbacks will also not be thread-safe. Either we make it really, really, really clear that the progress callbacks arrive from several threads or we somehow try to make this code handle the multi-threading for the user.
  2. The logic here isn't thread-safe (m_completed is atomic, but that doesn't make the surrounding logic thread-safe).
This revision now requires changes to proceed.Mar 2 2021, 1:00 AM
clayborg added inline comments.Mar 2 2021, 4:02 PM
lldb/include/lldb/Core/Progress.h
101

Agreed, after I added the "m_id" member variable, the ConstString value is no longer relied upon to unique the progress.

lldb/source/Core/Module.cpp
1075–1076

This is the main cause for deadlocks with DWARF logging. We have spoken about this on the list before where we should drop the mutex from this call to avoid deadlocking logging calls, but that also applies to this case where Progress objects might be constructed on any thread. The m_arch, m_file and m_object variables are setup very early in the Module object's lifetime, so it should be fine to not have to lock this mutex. I can submit this as a separate patch so we can discuss, but I included it here to avoid deadlocks.

lldb/source/Core/Progress.cpp
19

Yeah, I started off putting stuff in the Debugger as static calls, but then it really had nothing to do with the debugger since we don't ever have a debugger to pass around, nor does it make sense since the module list is global and all debuggers can use it and this is where the bulk of the long running operations happen.

I can add multiple callback support and thread safety. The only issue I see with multiple callbacks is you would need an extra call to say "remove this callback + baton from the list" if we ever want to remove the callback. Currently you can set the callback to null.

40

Yes multiple threads can call this.

  1. The callback function must be made thread safe as these callbacks will come from one or more threads. I will add logic to make the callback thread safe in lldb-vscode.cpp.
  1. You are correct, I can add a mutex to allow the math that I recently added to be thread safe and switch m_completed to not be atomic.
clayborg updated this revision to Diff 327871.Mar 3 2021, 11:43 AM

Update fixes:

  • make progress callback and baton members of a lldb_private::Debugger object to allow each debugger to have their own callbacks.
  • switch to have std::string title and clients use llvm::formatv() to create the title to avoid printf variadic args in progress constructor
  • make the progress class thread safe using a mutex
  • verified that lldb-vscode callback is threadsafe

This version should address all comments and issue that I am aware of. Now the callbacks are registered with each debugger and the Progress class no longer has any global state. The Progress class calls a static method on the debugger which allows the Debugger class to iterate over the global debugger list and report progress to any debuggers that have callbacks. This also means that later we can modify the Progress class to allow debugger specific progress notifications by modifying the Progress class to take a debugger ID or pointer as an optional constructor argument and then that progress can be reported to only that debugger instance. I don't have any places I need this yet, so I haven't added support for it.

lldb/include/lldb/Core/Progress.h
107

Using llvm::Optional was too messy. I tried, but since the callback requires a completed amount and total to be supplied at all times, it was much easier and cleaner to use UINT64_MAX since that would need to be sent anyway.

clayborg updated this revision to Diff 327945.Mar 3 2021, 3:28 PM

Fix typo in headerdoc comment.

This looks really nice. The only thought I have is that you should accept multiple callbacks instead of only one, as the current implementation discards any existing callback if SetProgressCallback is called twice. I know that it'd be rare that two or more callbacks are registered simultaneously, but it might happen in the future and you could save someone's time by making this a little bit more flexible.

So we have had a vote for allowing multiple callbacks. Any other main issues with the approach in this patch? If everyone is mostly happy, please chime in and I will make unit tests and vscode tests.

I'm quite happy with it besides the multiple callback thing.

jingham added a comment.EditedMar 5 2021, 2:20 PM

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. Given that the candidate for progress that is most common is down in SymbolFile, it's going to be hard to fix that. You'd have to have some way of marking the API boundary from "where you have a Target" to "where you don't have a Target" and record (maybe with TSD) that this target is using this API on this thread... Then you could use that to send the progress to the right actor.

Note, this is the way Xcode runs lldb, so this isn't a trivial concern. OTOH, it might be more trouble than it's worth to fix. But we should make clear that each callback will receive events caused by any of the extant debuggers, regardless of which debugger it was registered for. That very much changes how you would present the results.

I also wonder a little bit why you went with a callback rather than sending progress events? You wouldn't have had to invent a callback mechanism, then. Whoever cared about progress events would sign up for the "ProgressEvent" bit on their Debugger, and could do what they like.

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.

We can also add debugger specific progress in the future when we do have a long running task that can be associated with a specific debugger.

Given that the candidate for progress that is most common is down in SymbolFile, it's going to be hard to fix that. You'd have to have some way of marking the API boundary from "where you have a Target" to "where you don't have a Target" and record (maybe with TSD) that this target is using this API on this thread... Then you could use that to send the progress to the right actor.

Yeah, we would need to try and catch all of the areas that end up making queries just to avoid broadcasting extra progress to multiple debuggers. But again, 99.9% of the time people are debugging one thing.

Note, this is the way Xcode runs lldb, so this isn't a trivial concern. OTOH, it might be more trouble than it's worth to fix. But we should make clear that each callback will receive events caused by any of the extant debuggers, regardless of which debugger it was registered for. That very much changes how you would present the results.

It isn't too often that people debug multiple things at the same time. It does happen. But it would be ok for the activity in each window to show what is going on for things that can't be tied back to a specific debugger since it will benefit them all. Kind of like the old Apple mail activity view if anyone remembers that, it would be find for each window to show the same thing.

This at least allows us to supply feedback for long running operations where clients right now have no way of knowing why the delays are happening and often just kill the debug session when it is taking too long. So some feedback is much better than none IMHO.

I also wonder a little bit why you went with a callback rather than sending progress events? You wouldn't have had to invent a callback mechanism, then. Whoever cared about progress events would sign up for the "ProgressEvent" bit on their Debugger, and could do what they like.

We can go with SBBroadcaster and SBListener events if that is what everyone thinks is the best way forward. SBDebugger currently doesn't vend any events so that would need to be added. SBDebugger but does use a logging callback which probably could be also converted to SBEvents.

SBDebugger currently doesn't vend any events so that would need to be added.

I should clarify this: SBDebugger doesn't have any SBDebugger specific SBEvents that it vends or that people can sign up for. It does vend events for the targets / processes that it owns, just no SBDebugger specific events.

Anyone else have any opinion on SBEvent versus a callback? I like the SBEvent idea because this allows API access to this which means this can be tested easily in the python API tests.

One serious vote against the SBEvent way of doing things is that it might stop progress notification from appearing in the UI immediately if someone is currently calling something that causes a long running operation from the main thread that is running the SBEvent loop...

I think that the mechanism used to show progress should be lock-free and mostly unblocked, as it's merely an observer without side effects within LLDB. So from the last thing you said it seems that SBEvent doesn't like a good solution.

I agree that we should avoid SBEvent after thinking about it.

jingham added a comment.EditedMar 5 2021, 3:29 PM

I agree that we should avoid SBEvent after thinking about it.

First off, doing long running operations on a thread that is the one handling the major lldb events is obviously a bad idea. The driver doesn't do it this way. Commands get run on the main thread, and events get processed on the event-handler thread.

Secondly, there's no reason that the listener for progress events has to call "WaitForEvents" along with all the other events the Debugger might be waiting for. You could just set up a listener thread for all these events (they would have their own event bit) if you wanted somebody just monitoring progress events.

It also has the advantage that callbacks can't stall the progress of lldb, since the event just gets fired off, and then you go on your way...

I agree that we should avoid SBEvent after thinking about it.

First off, doing long running operations on a thread that is the one handling the major lldb events is obviously a bad idea. The driver doesn't do it this way. Commands get run on the main thread, and events get processed on the event-handler thread.

True, but the diver isn't a great example of using the API like an IDE would. And anything can kick off expensive events in the debugger. Xcode, as you know, as a thread that listens for events and does process control. This process control thread is responsible for many things including fetching the frame variables when needed. So just the act of having a dynamic type that needs to lookup a class by name could cause this expensive event to trigger. And with many other APIs, you never know what might trigger some expensive lookup to happen. Just the act of consuming a process stopped event on the process control thread is enough to trigger a python script or other LLDB commands in the event handler itself that could cause the expensive event to trigger and cause long delays. So there are plenty of reasons that this could cause delays.

Secondly, there's no reason that the listener for progress events has to call "WaitForEvents" along with all the process events it's waiting for on the debugger. You could just set up a listener thread for all these events (they would have their own event bit) if you wanted somebody just monitoring progress events.

That is true. But spinning up a thread just to listen for progress events seems like a bit of overkill, but it can easily be done.

It also has the advantage that callbacks can't stall the progress of lldb, since the event just gets fired off, and then you go on your way...

Using events does also has the advantage being able to receive all progress events on the same thread. Though since most GUI programs can only draw to the window server on the main thread, they will still need to setup things to forward these events to the main thread. Same goes for the callback methods though.

I am happy to try the SBEvent approach if anyone else chimes in

friss added a subscriber: friss.Mar 5 2021, 8:33 PM

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.

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).

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.

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).

I don't want to push this too hard, either way will work. But note that one of the big advantages of the event version is that we aren't running arbitrary code fairly deep in the Symbol side of lldb. At the cost of taking a lock to deliver the event, we get all this code out of the depth of the symbol reader and into a separate reporter thread.

I think the important thing here is that the code that detects the progress event not stall the Symbol processing. But I don't see the need to be super-performant in reporting the event. If the progress events are going by so fast that taking a lock to get the event and report on it matters, then the progress was going fast enough that the user probably won't care about it.

Another nice thing about the event version is that it allows you to coalesce reports if they come in too quickly, since you can peek at the event queue when you get the first event, and pull any other pending ones off. So for instance if you get a "started event" and peeking at the queue find that you also already have the completed event, you could choose not to report progress, or just completed...

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.

clayborg updated this revision to Diff 331739.Mar 18 2021, 6:25 PM

Added support for getting progress events via SBEvent delivery.

Thanks for doing this! The event version looks pretty clean to me. I think we should go that way. I don't think we should have two ways, that seems confusing and still leaves us calling unknown user code in the middle of symbol updates...

Thanks for doing this! The event version looks pretty clean to me. I think we should go that way. I don't think we should have two ways, that seems confusing and still leaves us calling unknown user code in the middle of symbol updates...

Sounds good, just wanted to make sure this was the way we want to go. I will remove the callback stuff.

clayborg updated this revision to Diff 332383.Mar 22 2021, 11:53 AM

Removed all callback related code and now only use events.

clayborg added inline comments.Mar 22 2021, 3:37 PM
lldb/include/lldb/API/SBDebugger.h
46

@jingham: do we need this GetBroadcasterClass()? Or would this only be used to listen to all debugger events as new debuggers are created? Also see my question later that asks if we need the broadcast manager in the Broadcaster we are using in the Debugger.h/Debugger.cpp.

lldb/source/Core/Debugger.cpp
672

@jingham: do we need to the m_broadcaster_manager_sp in this broadcaster? Debugger inherits from Broadcaster, but we could change this to "has a" instead of "is a" if we need the broadcast manager.

This seems like the right way to go. I made a couple trivial comment comments.

But also: the feature has the problem that when multiple debuggers are present, there's no way to tell what debugger actually triggered the work, and in that case all Debuggers will get the event. That's not the Progress feature's fault, it's forced on us by the design of the global shared module cache. But we shouldn't let the fact that that happens be first set of things for which you want to add progress drive the design. Anyway, it seems to me like it would be simple to set this up to support other areas where there is a debugger, and doing that up front would keep people from accidentally adding progress but not providing the debugger because the API makes it seem like that isn't relevant. I think the only things that you would need to add are:

  1. Have Progress::ReportProgress take a Debugger * argument, and only use the static Debugger::ReportProgress when the debugger is NULL. That would make it clear to users of the API that they should provide a Debugger if one is available.
  2. Have another bit in the ProgressEventData that tells whether a debugger was supplied or not. That way IDE's that support many debugger's running simultaneously would know to not associate anonymous progress events with any particular debugger, but to put it up in some kind of general progress.
lldb/include/lldb/API/SBDebugger.h
50

remove the "that"

59

It -> If

lldb/include/lldb/Core/Progress.h
67

Not grammatical. Maybe "then an indeterminate progress indicator should be displayed".

lldb/source/API/SBDebugger.cpp
857

These are just drive by reformattings, right? Might be good not to include them in this change.

lldb/source/Core/Debugger.cpp
1188

Isn't ReportProgressPrivate the way that any part of the system that DOES have access to a Debugger should call to update Progress? If so I don't think it's right to call this API "Private". This should be documented as the preferred method, and the static ReportProgress should be documented as only to be used in situations where you can't find your way to a debugger.

lldb/source/Core/Progress.cpp
50

IMO it would be better for Progress::ReportProgress to take a Debugger *. If the Debugger* is non-null, then we would call Debugger->ReportProgressPrivate or whatever is the appropriate name, and only if the Debugger is NULL would we call the static function. It would be better if we have the right way to do things from the get-go, and since the Debugger is delivering the event, that's pretty clearly to call ReportProgress for the relevant Debugger.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
1900

Also an unrelated reformatting...

jingham added inline comments.Mar 22 2021, 4:17 PM
lldb/include/lldb/API/SBDebugger.h
46

It is needed by the mechanism for waiting for the events from all broadcasters of a given type. Not sure why you wouldn't want to do this for Debugger events.

GetBroadcasterClass is used in the Debugger DefaultEventHandler to separate the treatment for the various event sources the Debugger waits for. One of the missing pieces of this patch is to get the Driver's default event handling to do something with progress updates. I presume that would go through the same DefaultEventHandler, and you'd use the comparison of GetStaticBroadcasterClass and GetBroadcasterClass in the same way that code does for all the other event classes.

lldb/source/Core/Debugger.cpp
672

Given that at least in the first pass all the events are not specific to a debugger, in a Multi-Debugger UI I could very well imagine that instead of having any individual debugger's Listener listen for these events, you would make some thread listen for them all, and then put up some System Wide progress UI. That would make it much less confusing than seeming to associate events with debuggers you don't know they pertain to.

And even when we start reporting Progress that was associated with a Debugger, we will still have all these unassociated events from the module cache. So you could still imagine having a listener for all debuggers, that only reports progress that has no debugger associated with it, and then have individual Debugger's report progress pertaining directly to them.

So I do think we need to have a BroadcasterManager that allows you to sign up for all Debugger events from any debugger that will be created.

clayborg updated this revision to Diff 332485.Mar 22 2021, 5:41 PM

Added the option for people to add a debugger to a lldb_private::Progress instance. This allows progress to be reported only to specific debuggers.

clayborg marked 4 inline comments as done.Mar 22 2021, 5:49 PM
clayborg added inline comments.
lldb/source/API/SBDebugger.cpp
857

I submit with "arc diff" and it will cause lint errors if I don't allow it to fix the lint errors it finds.

lldb/source/Core/Debugger.cpp
1188

In the Progress class I currently only store a pointer to the debugger if one was specified. This means there is a lifetime issue that can arise. If I do the callback for Debugger::ReportProgress(...) above, then we will only notify debugger instances that are still around and in the global list. If not then we need to either store a DebuggerSP or DebuggerWP in the Progress object, which we can do if truly needed, but I don't like DebuggerSP as I don't want a Progress to keep a debugger around. DebuggerWP is possible if we really need this. Let me know what you think. Debugger::ReportProgressPrivate() could be make into a static function if needed so that no one sees it from Debugger.h.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
1900–1901

again, I use "arc diff" which must fix lint issues.

jingham added inline comments.Mar 22 2021, 6:21 PM
lldb/source/API/SBDebugger.cpp
857

I'm 100% not in favor of tools that force irrelevant changes to be included. But that is a suggested tool so somebody must like that.

lldb/source/Core/Debugger.cpp
1188

I think it's fine to drop progress notifications on the floor if they were debugger specific and that debugger is no longer around. I don't think it makes sense for the Progress to keep Debugger alive. As long as you check on the existence as you do here, you won't end up calling into a bad debugger, so the way you've done it looks fine. You might want to have an:

if (is_debugger_specific) break;

After the call to ReportProgressPrivate. It's unlikely we'll ever have enough debugger's that this a performance issue, but still looks weird to keep checking after we found the one we want.

BTW, it might be better to have the Progress store the DebuggerID rather than the pointer. It would make it clear we aren't making claims about keeping the debugger alive.. That would simplify this code, since you could:

if (debugger_id != LLDB_INVALID_DEBUGGER_ID) {
 DebuggerSP debugger_sp = Debugger::FindDebuggerWithID(debugger_id)
 if (debugger_sp) debugger_sp->ReportProgressPrivate(..., true);
 return;
}

Then your original loop just calling ReportProgressPrivate on all the debuggers, passing false for "debugger_specific".

dblaikie added inline comments.
lldb/source/API/SBDebugger.cpp
857

They aren't forced - you can submit with linter errors if they don't seem helpful.

Pre-committing format changes in standalone NFC commits would generally be preferable. & the linter shouldn't be flagging untouched lines - is it?

clayborg added inline comments.Mar 22 2021, 9:11 PM
lldb/source/API/SBDebugger.cpp
857

Actually come to thing about it, it is probably because my editor removes trailing newlines and then the linter deems those lines fair game...

lldb/source/Core/Debugger.cpp
1188

I like the debugger ID storage idea. I will do that, and break out once we find the right debugger.

clayborg updated this revision to Diff 332523.Mar 22 2021, 9:57 PM

Don't store a debugger pointer in the lldb_private::Progress class, store a debugger ID as an optional value. Then use this value in the debugger report progress function to do the right thing.

jingham accepted this revision.Mar 23 2021, 10:12 AM

This looks good to me.

lldb/source/API/SBDebugger.cpp
857

It looks like the file had changes 700 lines before & 900 lines after, but these lines weren't changed except by the linter...

lldb/source/Core/Debugger.cpp
1188

BTW, I made up LLDB_INVALID_DEBUGGER_ID, apparently...

dblaikie added inline comments.Mar 23 2021, 11:18 AM
lldb/source/API/SBDebugger.cpp
857

Yeah, if your editor is touching otherwise unmodified lines that'll be a problem (for the linter, or without the linter - you'd end up with unrelated changes in the diff, etc).

clang-format can be integrated into some editors (vim, for instance) to auto-format only changed lines on save, which is what I use - not sure if there's an option to use that in your workflow.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 24 2021, 12:58 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

I will take a look and apply this patch on my linux server and enable ASAN. Looks like lldb-vscode must be crashing from what the errors look like.

I built on mac with ASAN and ran the tests and they passed just fine. I was also able to compile on Debian linux and the tests ran ok for me. Can anyone else reproduce this failure?