Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.Thu, Mar 18, 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.Mon, Mar 22, 11:53 AM

Removed all callback related code and now only use events.

clayborg added inline comments.Mon, Mar 22, 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.Mon, Mar 22, 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.Mon, Mar 22, 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.Mon, Mar 22, 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.Mon, Mar 22, 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.Mon, Mar 22, 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.Mon, Mar 22, 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.Tue, Mar 23, 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.Tue, Mar 23, 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.Wed, Mar 24, 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?