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

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/API/SBDebugger.cpp:857
+        error = m_opaque_sp->GetTargetList().CreateTarget(
+            *m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+            target_sp);
----------------
clayborg wrote:
> I submit with "arc diff" and it will cause lint errors if I don't allow it to fix the lint errors it finds.
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.


================
Comment at: lldb/source/Core/Debugger.cpp:1188
+        (*pos)->ReportProgressPrivate(progress_id, message, completed, total,
+                                      is_debugger_specific);
+    }
----------------
clayborg wrote:
> 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.
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739