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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
