[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 inline comments.


================
Comment at: lldb/include/lldb/Core/Progress.h:100
+  /// The title of the progress activity.
+  ConstString m_message;
+  /// How much work ([0...m_total]) that has been completed.
----------------
teemperor wrote:
> `std::string`
Agreed, after I added the "m_id" member variable, the ConstString value is no longer relied upon to unique the progress.


================
Comment at: lldb/source/Core/Module.cpp:1075-1076
                             lldb::DescriptionLevel level) {
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
   if (level >= eDescriptionLevelFull) {
----------------
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.


================
Comment at: lldb/source/Core/Progress.cpp:18
+void *Progress::g_callback_baton = nullptr;
+uint64_t Progress::g_progress_id = 0;
+
----------------
teemperor wrote:
> 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).
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.


================
Comment at: lldb/source/Core/Progress.cpp:39
+
+void Progress::Increment(uint64_t amount) {
+  if (amount > 0) {
----------------
teemperor wrote:
> 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).
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.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739