[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:1
+//===-- Progress.h ----------------------------------------------*- C++ -*-===//
+//
----------------
JDevlieghere wrote:
> I think this belongs more in Utility than Core. 
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.


================
Comment at: lldb/include/lldb/Core/Progress.h:19
+public:
+  Progress(uint64_t total, const char *format, ...)
+      __attribute__((format(printf, 3, 4)));
----------------
JDevlieghere wrote:
> 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? 
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.


================
Comment at: lldb/include/lldb/Core/Progress.h:24
+
+  static void SetProgressCallback(lldb::ProgressCallback callback, void *baton);
+
----------------
JDevlieghere wrote:
> 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. 
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.


================
Comment at: lldb/include/lldb/Core/Progress.h:30
+  static lldb::ProgressCallback g_callback;
+  static void *g_callback_baton;
+  ConstString m_message;
----------------
JDevlieghere wrote:
> What do you imagine this baton to be used for?
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.


================
Comment at: lldb/include/lldb/Core/Progress.h:32
+  ConstString m_message;
+  std::atomic<uint32_t> m_completed;
+  const uint64_t m_total;
----------------
JDevlieghere wrote:
> Why not `uint64_t`? 
Yes, will fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739