[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 accepted this revision.
jingham added a comment.

This looks good to me.



================
Comment at: lldb/source/API/SBDebugger.cpp:857
+        error = m_opaque_sp->GetTargetList().CreateTarget(
+            *m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp,
+            target_sp);
----------------
dblaikie wrote:
> jingham wrote:
> > 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.
> 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?
It looks like the file had changes 700 lines before & 900 lines after, but these lines weren't changed except by the linter... 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739