clayborg added 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.
----------------
teemperor wrote:
> `std::string`
Agreed, after I added the "m_id" member variable, the ConstString value is no
longer relied upon to unique the progress.
================
Comment at: lldb/source/Core/Module.cpp:1075-1076
lldb::DescriptionLevel level) {
- std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
if (level >= eDescriptionLevelFull) {
----------------
This is the main cause for deadlocks with DWARF logging. We have spoken about
this on the list before where we should drop the mutex from this call to avoid
deadlocking logging calls, but that also applies to this case where Progress
objects might be constructed on any thread. The m_arch, m_file and m_object
variables are setup very early in the Module object's lifetime, so it should be
fine to not have to lock this mutex. I can submit this as a separate patch so
we can discuss, but I included it here to avoid deadlocks.
================
Comment at: lldb/source/Core/Progress.cpp:18
+void *Progress::g_callback_baton = nullptr;
+uint64_t Progress::g_progress_id = 0;
+
----------------
teemperor wrote:
> 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).
Yeah, I started off putting stuff in the Debugger as static calls, but then it
really had nothing to do with the debugger since we don't ever have a debugger
to pass around, nor does it make sense since the module list is global and all
debuggers can use it and this is where the bulk of the long running operations
happen.
I can add multiple callback support and thread safety. The only issue I see
with multiple callbacks is you would need an extra call to say "remove this
callback + baton from the list" if we ever want to remove the callback.
Currently you can set the callback to null.
================
Comment at: lldb/source/Core/Progress.cpp:39
+
+void Progress::Increment(uint64_t amount) {
+ if (amount > 0) {
----------------
teemperor wrote:
> 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).
Yes multiple threads can call this.
1. The callback function must be made thread safe as these callbacks will come
from one or more threads. I will add logic to make the callback thread safe in
lldb-vscode.cpp.
2. You are correct, I can add a mutex to allow the math that I recently added
to be thread safe and switch m_completed to not be atomic.
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
