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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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



================
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.
----------------
`std::string`


================
Comment at: lldb/include/lldb/Core/Progress.h:19
+public:
+  Progress(uint64_t total, const char *format, ...)
+      __attribute__((format(printf, 3, 4)));
----------------
clayborg wrote:
> clayborg wrote:
> > 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.
> If we do switch to "ConstString message" can you add inline code suggestions for using formatv for one of the Progress constructors to see how it would look?
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).


================
Comment at: lldb/source/Core/Module.cpp:1076
-  std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
   if (level >= eDescriptionLevelFull) {
----------------
I want to ask why but I fear the answer.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97739