[LLDB] Add SBProgress so Python scripts can also report progress #119052
Conversation
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesRecently I've been working on a lot of internal Python tooling, and in certain cases I want to report async to the script over DAP. Progress.h already handles this, so I've exposed Progress via the SB API so Python scripts can also update progress objects. I actually have no idea how to test this, so I just wrote a toy command to test it
I also copied the first section of the extensive Progress.h class documentation to the docstrings. Full diff: https://github.com/llvm/llvm-project/pull/119052.diff 8 Files Affected:
diff --git a/lldb/bindings/headers.swig b/lldb/bindings/headers.swig
index c0dde905f986bd..5e7c54d1eb8393 100644
--- a/lldb/bindings/headers.swig
+++ b/lldb/bindings/headers.swig
@@ -52,6 +52,7 @@
#include "lldb/API/SBProcess.h"
#include "lldb/API/SBProcessInfo.h"
#include "lldb/API/SBProcessInfoList.h"
+#include "lldb/API/SBProgress.h"
#include "lldb/API/SBQueue.h"
#include "lldb/API/SBQueueItem.h"
#include "lldb/API/SBReproducer.h"
diff --git a/lldb/bindings/interface/SBProgressDocstrings.i b/lldb/bindings/interface/SBProgressDocstrings.i
new file mode 100644
index 00000000000000..016c02432c4117
--- /dev/null
+++ b/lldb/bindings/interface/SBProgressDocstrings.i
@@ -0,0 +1,14 @@
+%feature("docstring",
+"A Progress indicator helper class.
+
+Any potentially long running sections of code in LLDB should report
+progress so that clients are aware of delays that might appear during
+debugging. Delays commonly include indexing debug information, parsing
+symbol tables for object files, downloading symbols from remote
+repositories, and many more things.
+
+The Progress class helps make sure that progress is correctly reported
+and will always send an initial progress update, updates when
+Progress::Increment() is called, and also will make sure that a progress
+completed update is reported even if the user doesn't explicitly cause one
+to be sent.") lldb::SBProgress
diff --git a/lldb/bindings/interfaces.swig b/lldb/bindings/interfaces.swig
index 8a6fed95f0b729..08df9a1a8d5392 100644
--- a/lldb/bindings/interfaces.swig
+++ b/lldb/bindings/interfaces.swig
@@ -54,6 +54,7 @@
%include "./interface/SBPlatformDocstrings.i"
%include "./interface/SBProcessDocstrings.i"
%include "./interface/SBProcessInfoDocstrings.i"
+%include "./interface/SBProgressDocstrings.i"
%include "./interface/SBQueueDocstrings.i"
%include "./interface/SBQueueItemDocstrings.i"
%include "./interface/SBReproducerDocstrings.i"
@@ -133,6 +134,7 @@
%include "lldb/API/SBProcess.h"
%include "lldb/API/SBProcessInfo.h"
%include "lldb/API/SBProcessInfoList.h"
+%include "lldb/API/SBProgress.h"
%include "lldb/API/SBQueue.h"
%include "lldb/API/SBQueueItem.h"
%include "lldb/API/SBReproducer.h"
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 1f42ec3cdc7d51..a4c4e11f03dff9 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -203,7 +203,7 @@ class LLDB_API SBDebugger {
lldb::SBCommandInterpreter GetCommandInterpreter();
void HandleCommand(const char *command);
-
+
void RequestInterrupt();
void CancelInterruptRequest();
bool InterruptRequested();
@@ -513,6 +513,7 @@ class LLDB_API SBDebugger {
friend class SBPlatform;
friend class SBTarget;
friend class SBTrace;
+ friend class SBProgress;
lldb::SBTarget FindTargetWithLLDBProcess(const lldb::ProcessSP &processSP);
diff --git a/lldb/include/lldb/API/SBProgress.h b/lldb/include/lldb/API/SBProgress.h
new file mode 100644
index 00000000000000..38f4b4a81f42b7
--- /dev/null
+++ b/lldb/include/lldb/API/SBProgress.h
@@ -0,0 +1,56 @@
+//===-- SBProgress.h --------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_API_SBPROGRESS_H
+#define LLDB_API_SBPROGRESS_H
+
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBDefines.h"
+
+namespace lldb {
+
+/// A SB API Wrapper around lldb_private::Progress
+class LLDB_API SBProgress {
+public:
+ /// Construct a progress object with a title, details and a given debugger.
+ /// \param title
+ /// The title of the progress object.
+ /// \param details
+ /// The details of the progress object.
+ /// \param debugger
+ /// The debugger for this progress object to report to.
+ SBProgress(const char *title, const char *details, SBDebugger &debugger);
+
+ /// Construct a progress object with a title, details, the total units of work
+ /// to be done, and a given debugger.
+ /// \param title
+ /// The title of the progress object.
+ /// \param details
+ /// The details of the progress object.
+ /// \param total_units
+ /// The total number of units of work to be done.
+ /// \param debugger
+ /// The debugger for this progress object to report to.
+ SBProgress(const char *title, const char *details, uint64_t total_units,
+ SBDebugger &debugger);
+ SBProgress(const lldb::SBProgress &rhs);
+ ~SBProgress();
+
+ const SBProgress &operator=(const lldb::SBProgress &rhs);
+
+ void Increment(uint64_t amount = 1, const char *description = nullptr);
+
+protected:
+ lldb_private::Progress &ref() const;
+
+private:
+ std::unique_ptr<lldb_private::Progress> m_opaque_up;
+}; // SBProgress
+} // namespace lldb
+
+#endif // LLDB_API_SBPROGRESS_H
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index d09edeeccaff1a..fc7456a4b9a32e 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -233,6 +233,7 @@ class Symtab;
class SyntheticChildren;
class SyntheticChildrenFrontEnd;
class SystemRuntime;
+class Progress;
class Target;
class TargetList;
class TargetProperties;
diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt
index d8308841c05dba..147b30f3b00269 100644
--- a/lldb/source/API/CMakeLists.txt
+++ b/lldb/source/API/CMakeLists.txt
@@ -83,6 +83,7 @@ add_lldb_library(liblldb SHARED ${option_framework}
SBModule.cpp
SBModuleSpec.cpp
SBPlatform.cpp
+ SBProgress.cpp
SBProcess.cpp
SBProcessInfo.cpp
SBProcessInfoList.cpp
diff --git a/lldb/source/API/SBProgress.cpp b/lldb/source/API/SBProgress.cpp
new file mode 100644
index 00000000000000..adfe3c0b865845
--- /dev/null
+++ b/lldb/source/API/SBProgress.cpp
@@ -0,0 +1,34 @@
+//===-- SBProgress.cpp -------------------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/API/SBProgress.h"
+#include "lldb/Core/Progress.h"
+#include "lldb/Utility/Instrumentation.h"
+
+using namespace lldb;
+
+SBProgress::SBProgress(const char *title, const char *details,
+ SBDebugger &debugger) {
+ LLDB_INSTRUMENT_VA(this, title, details, debugger)
+ m_opaque_up = std::make_unique<lldb_private::Progress>(
+ title, details, std::nullopt, debugger.get());
+}
+
+SBProgress::SBProgress(const char *title, const char *details,
+ uint64_t total_units, SBDebugger &debugger) {
+ LLDB_INSTRUMENT_VA(this, title, details, total_units, debugger)
+ m_opaque_up = std::make_unique<lldb_private::Progress>(
+ title, details, total_units, debugger.get());
+}
+
+void SBProgress::Increment(uint64_t amount, const char *description) {
+ m_opaque_up->Increment(amount, description);
+}
+
+lldb_private::Progress &SBProgress::ref() const { return *m_opaque_up; }
|
There was a problem hiding this comment.
The patch itself looks good with some nits, but I'm (still) on the fence whether this a good idea.
In the past, a similar question has come up in the context of logging and whether we want to add SB APIs to make use of LLDB's internals. The argument in favor is that LLDB is meant to be extensible and that those extension points should be able to benefit from LLDB's infrastructure. The counter-argument is that nothing can prevent users from reporting arbitrary data and making it look like it originates from LLDB.
I'm curious to hear other's opinion on the matter. If it wasn't the SB API I think we could figure that out in the PR as we have more freedom to change our mind in the future, but given the stability guarantees for the SB API, this might be worth an RFC before we commit to something.
lldb/include/lldb/API/SBProgress.h
Outdated
|
|
||
| namespace lldb { | ||
|
|
||
| /// A SB API Wrapper around lldb_private::Progress |
There was a problem hiding this comment.
I hate recommending to duplicate anything, but the class should have the same (user facing) documentation as the docstring.
lldb/source/API/SBProgress.cpp
Outdated
| //===-- SBProgress.cpp -------------------------------------------*- C++ | ||
| //-*-===// |
There was a problem hiding this comment.
Broken ASCII art. Also new headers shouldn't contain - C++ anymore.
lldb/source/API/SBProgress.cpp
Outdated
|
|
||
| SBProgress::SBProgress(const char *title, const char *details, | ||
| SBDebugger &debugger) { | ||
| LLDB_INSTRUMENT_VA(this, title, details, debugger) |
There was a problem hiding this comment.
Nit: There should be a newline after LLDB_INSTRUMENT_VA. Same below. Also looks like the destructor hasn't been implemented, which needs to be instrumented as well.
Just to throw my opinion into the debate; I view Progress.h in the same way as I view any printing to the terminal (or stdout). We already allow Python commands, and I believe formatters, to print to the user's console. So restricting progress because we don't want a python script to be masquerading as LLDB seems less of a concern for me for SBProgress. |
I would agree with you if the progress events were always show in the debugger console, but except for the command-line lldb, all IDEs I know of that support LLDB's progress events show it in a part of the UI that's usually not tied to debugger but displays other progress events like indexing, building, etc. I have a similar concern for logging. When people log to the console or to file, I don't really care that a command or extension is logging arbitrary data. But when you're logging to the system log (which on Darwin becomes part of a sysdiagnose, which in turn has pretty strict privacy requirements). Those arbitrary logs then become a problem. A potential solution for my concerns could be a flag that's set when logs or progress reports are created from the SB API, and LLDB can potentially handle them differently. Concretely for progress progress events, that could mean that we have a different broadcast bit that allows IDEs to subscribe to events that include or exclude those events. |
Huh, cool to learn. I agree with you then. I think we can keep it even 'simpler' than having a bit and just differentiating between 'user-space' progress and 'lldb/system-space' progress? Maybe something as simple as an enum and an overload to the API? |
|
@JDevlieghere if I were to add a way to identify an 'internal' vs 'external' progress, do you want me to make all consumers aware to this distinction or are we cool with just exposing the property as a part of this patch? |
|
@Jlalond If it were up to me I'd create another PR which adds the internal/external property. All existing progress events would be internal so that should be NFC. We have unit tests for the progress events so you could create an external progress event and test that it's only broadcast to the new bit. Then I'd rebase this PR on top and make the progress event created through the SB API as external, update the default event handler to listen for the new bit in |
|
I think the patch seems good as it is without needing to differentiate really. What are we gaining by hiding something that is trying to report progress that can take some time? Wouldn't we want to know this in a sys diagnose situation? If a python command runs for a long time, I would want to be able to report what the script was doing via progress and see the output without having to go and modify lldb-dap to handle some new progress event type. We already have the issue with python commands where if we don't have the ability to write to stdout and have it show up, then we need to register an immediate file to the SBCommandReturnObject which is kind of hacky, and that only works if the debugger's STDOUT hasn't been redirected to /dev/null. |
I think we need to distinguish two kinds of users here. The first one are the ones that sit "outside" of lldb and use the SB API to debug something (in a scripted manner). For these, I think I'd agree with you. It's gets trickier with the other kind -- the ones running "inside" lldb. Here I include all the data formatters, stop hooks and other kinds of (python) callbacks. I can see how one would may want to report progress from inside these and have it go wherever the usual progress reports go (meaning, the console, if you're running lldb CLI; the dap connection if you happen to be running in lldb-dap; or whatever happens in xcode). That said, I can also see how the consumer might want to treat these progress events differently from "core" lldb events, so using a different bit for these makes sense as well. For example, the consumer may want/need to be prepared to handle progress objects that never terminate (the complete event is never sent). At that point, I suppose one can ask the question whether this functionality should be built on top of the Progress class (as opposed to e.g. direct SBListener/SBEvent broadcasts). For me, it's main value lies in its RAII nature (*), but that's something we can't guarantee for python anyway. So yeah, I think an RFC would be nice. (*) #119377 changes that somewhat, but I would hope that no python script is emitting events at such a frequency that this would be needed -- or if it is, I'd hope it could be implemented in a more traditional manner (i.e., with a mutex). |
As feedback on #119052, it was recommended I add a new bit to delineate internal and external progress events. This patch adds this new category, and sets up Progress.h to support external events via SBProgress.
|
✅ With the latest revision this PR passed the Python code formatter. |
|
@JDevlieghere Do we want to add new tests having the external bit by default? I added unit tests in the external category bit PR. Any additional test recommendations? I didn't find any 'default' broadcast bit test cases. |
|
@JDevlieghere friendly bump, I just wanna sneak this into our release this week, I think all the comments are addressed! |
There was a problem hiding this comment.
I'm happy with this: it addresses all my concerns.
@labath, you had some similar, and some different concerns (i.e. the RAII benefit of the C++ implementation). Should we have an RFC for this to flesh that out or how do you feel about the current state of this patch?
|
I can still make an RFC if that is desired, but I believed we reached a compromise with the new broadcast bit. I think we should frame the SBProgress very differently to @labath's recent improvements to Progress itself. I think the critical feature is the disposal of SBProgress and the deconstruction of the progress object. I think it's probably going to be fine if this deconstruction in non deterministic in Python. I can't imagine many workflows that would repeatedly creating Progresses where their lifetimes overlapping would interrupt with each other. @labath I'll leave final sign off to you in this case. I personally think it's fine to merge as is because the API is so small, and I think we can add new semantics or extend the API if needed. |
There was a problem hiding this comment.
I think this is fine. I was thinking this could be implemented purely from the python side by sending the appropriate events manually, but this doesn't work because we don't have the ability to construct the right kind of event objects. We could add that, but then again, we could just do this, and I don't think this is worse.
lldb/include/lldb/API/SBProgress.h
Outdated
|
|
||
| ~SBProgress(); | ||
|
|
||
| const SBProgress &operator=(const lldb::SBProgress &rhs); |
There was a problem hiding this comment.
If this has an assignment operator, it should probably get a copy constructor as well..
There was a problem hiding this comment.
But the assignment op is never defined, so maybe they should both be =deleted?
There was a problem hiding this comment.
I thought I deleted this. This was a hold over from when I copied an example heh.
I didn't go forward with deleting copy assignment and constructor. I don't believe I can do = delete; in the implementation because it's declared in the header, and I remember @bulbazord fixing SBSaveCoreOptions where I put a = default implementation in the header. So I just removed the copy operator.
| self.assertTrue(listener.PeekAtNextEvent(event)) | ||
| stream = lldb.SBStream() | ||
| event.GetDescription(stream) | ||
| self.assertTrue(expected_string in stream.GetData()) |
There was a problem hiding this comment.
| self.assertTrue(expected_string in stream.GetData()) | |
| self.assertIn(expected_string, stream.GetData()) |
…123826) Recently I added SBProgress (#119052), and during that original commit I tested if the progress event was sent over LLDB-DAP, and it was. However upon the suggestion of @JDevlieghere and @labath we added an external category (#120171), which I did not test. This small patch wires up DAP to listen for external events by default, and adds the external category to the SBDebugger enumeration.
…m#119052) Recently I've been working on a lot of internal Python tooling, and in certain cases I want to report async to the script over DAP. Progress.h already handles this, so I've exposed Progress via the SB API so Python scripts can also update progress objects. I actually have no idea how to test this, so I just wrote a [toy command to test it](https://gist.github.com/Jlalond/48d85e75a91f7a137e3142e6a13d0947)  I also copied the first section of the extensive Progress.h class documentation to the docstrings. (cherry picked from commit 6b048ae)
As feedback on llvm#119052, it was recommended I add a new bit to delineate internal and external progress events. This patch adds this new category, and sets up Progress.h to support external events via SBProgress. (cherry picked from commit 774c226)
…lvm#123826) Recently I added SBProgress (llvm#119052), and during that original commit I tested if the progress event was sent over LLDB-DAP, and it was. However upon the suggestion of @JDevlieghere and @labath we added an external category (llvm#120171), which I did not test. This small patch wires up DAP to listen for external events by default, and adds the external category to the SBDebugger enumeration. (cherry picked from commit b9813ce)
…lvm#123826) Recently I added SBProgress (llvm#119052), and during that original commit I tested if the progress event was sent over LLDB-DAP, and it was. However upon the suggestion of @JDevlieghere and @labath we added an external category (llvm#120171), which I did not test. This small patch wires up DAP to listen for external events by default, and adds the external category to the SBDebugger enumeration. (cherry picked from commit b9813ce)
Recently I've been working on a lot of internal Python tooling, and in certain cases I want to report async to the script over DAP. Progress.h already handles this, so I've exposed Progress via the SB API so Python scripts can also update progress objects.
I actually have no idea how to test this, so I just wrote a toy command to test it
I also copied the first section of the extensive Progress.h class documentation to the docstrings.