Add SBDebugger:: AddNotificationCallback API #111206
Conversation
|
@llvm/pr-subscribers-lldb Author: None (jeffreytan81) ChangesThis PR adds new SBDebugger::AddCreateCallback()/RemoveCreateCallback() APIs. This API enables any lldb client to subscribe to debugger creation and perform action on it. For example, we have telemetry code that uses Full diff: https://github.com/llvm/llvm-project/pull/111206.diff 11 Files Affected:
diff --git a/lldb/bindings/python/python-swigsafecast.swig b/lldb/bindings/python/python-swigsafecast.swig
index 0127ac6bfa4681..dc2b9c0ca69693 100644
--- a/lldb/bindings/python/python-swigsafecast.swig
+++ b/lldb/bindings/python/python-swigsafecast.swig
@@ -133,5 +133,10 @@ PythonObject SWIGBridge::ToSWIGWrapper(
return ToSWIGHelper(module_spec_sb.release(), SWIGTYPE_p_lldb__SBModuleSpec);
}
+PythonObject SWIGBridge::ToSWIGWrapper(
+ std::unique_ptr<lldb::SBDebugger> debugger_sb) {
+ return ToSWIGHelper(debugger_sb.release(), SWIGTYPE_p_lldb__SBDebugger);
+}
+
} // namespace python
} // namespace lldb_private
diff --git a/lldb/bindings/python/python-typemaps.swig b/lldb/bindings/python/python-typemaps.swig
index f8c33e15c03e66..c61edc31bad26f 100644
--- a/lldb/bindings/python/python-typemaps.swig
+++ b/lldb/bindings/python/python-typemaps.swig
@@ -452,6 +452,25 @@ template <> bool SetNumberFromPyObject<double>(double &number, PyObject *obj) {
$1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
}
+// For lldb::SBDebuggerCreateCallback
+%typemap(in) (lldb::SBDebuggerCreateCallback create_callback, void *baton) {
+ if (!($input == Py_None ||
+ PyCallable_Check(reinterpret_cast<PyObject *>($input)))) {
+ PyErr_SetString(PyExc_TypeError, "Need a callable object or None!");
+ SWIG_fail;
+ }
+
+ // Don't lose the callback reference
+ Py_INCREF($input);
+ $1 = LLDBSwigPythonCallPythonSBDebuggerCreateCallback;
+ $2 = $input;
+}
+
+%typemap(typecheck) (lldb::SBDebuggerCreateCallback create_callback, void *baton) {
+ $1 = $input == Py_None;
+ $1 = $1 || PyCallable_Check(reinterpret_cast<PyObject *>($input));
+}
+
// For lldb::SBDebuggerDestroyCallback
%typemap(in) (lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
if (!($input == Py_None ||
diff --git a/lldb/bindings/python/python-wrapper.swig b/lldb/bindings/python/python-wrapper.swig
index 810673aaec5d19..9eef0479290073 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -1024,6 +1024,26 @@ static void LLDBSwigPythonCallPythonLogOutputCallback(const char *str,
}
}
+// For DebuggerCreateCallback functions
+static void LLDBSwigPythonCallPythonSBDebuggerCreateCallback(lldb::SBDebugger &debugger,
+ void *baton) {
+ if (baton != Py_None) {
+ SWIG_PYTHON_THREAD_BEGIN_BLOCK;
+ PythonObject debugger_arg = SWIGBridge::ToSWIGWrapper(
+ std::make_unique<SBDebugger>(debugger));
+
+ // Create a tuple of arguments
+ PyObject *args = PyTuple_New(1);
+ PyTuple_SetItem(args, 0, debugger_arg.get());
+
+ // Call the Python function
+ PyObject *result = PyObject_CallFunction(
+ reinterpret_cast<PyObject *>(baton), const_cast<char *>("O"), args);
+ Py_XDECREF(result);
+ SWIG_PYTHON_THREAD_END_BLOCK;
+ }
+}
+
// For DebuggerTerminateCallback functions
static void LLDBSwigPythonCallPythonSBDebuggerTerminateCallback(lldb::user_id_t debugger_id,
void *baton) {
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 84ea9c0f772e16..cef5307418fa2d 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -336,6 +336,16 @@ class LLDB_API SBDebugger {
void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
void *baton);
+ /// Add a callback for when the debugger is created. Return a token, which
+ /// can be used to remove said callback. Multiple callbacks can be added by
+ /// calling this function multiple times, and will be invoked in FIFO order.
+ static lldb::callback_token_t
+ AddCreateCallback(lldb::SBDebuggerCreateCallback create_callback,
+ void *baton);
+
+ /// Remove the specified callback. Return true if successful.
+ static bool RemoveCreateCallback(lldb::callback_token_t token);
+
/// Add a callback for when the debugger is destroyed. Return a token, which
/// can be used to remove said callback. Multiple callbacks can be added by
/// calling this function multiple times, and will be invoked in FIFO order.
diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h
index 9543ebc08a320f..104bdc8c79f44c 100644
--- a/lldb/include/lldb/API/SBDefines.h
+++ b/lldb/include/lldb/API/SBDefines.h
@@ -139,6 +139,8 @@ typedef bool (*SBBreakpointHitCallback)(void *baton, lldb::SBProcess &process,
lldb::SBThread &thread,
lldb::SBBreakpointLocation &location);
+typedef void (*SBDebuggerCreateCallback)(lldb::SBDebugger &debugger,
+ void *baton);
typedef void (*SBDebuggerDestroyCallback)(lldb::user_id_t debugger_id,
void *baton);
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index a72c2596cc2c5e..16e3298f7b1ed5 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -569,6 +569,16 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback,
void *baton);
+ /// Add a callback for when the debugger is created. Return a token, which
+ /// can be used to remove said callback. Multiple callbacks can be added by
+ /// calling this function multiple times, and will be invoked in FIFO order.
+ static lldb::callback_token_t
+ AddCreateCallback(lldb_private::DebuggerCreateCallback create_callback,
+ void *baton, void *original_callback);
+
+ /// Remove the specified callback. Return true if successful.
+ static bool RemoveCreateCallback(lldb::callback_token_t token);
+
/// Add a callback for when the debugger is destroyed. Return a token, which
/// can be used to remove said callback. Multiple callbacks can be added by
/// calling this function multiple times, and will be invoked in FIFO order.
@@ -737,19 +747,32 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
lldb::TargetSP m_dummy_target_sp;
Diagnostics::CallbackID m_diagnostics_callback_id;
- std::mutex m_destroy_callback_mutex;
- lldb::callback_token_t m_destroy_callback_next_token = 0;
- struct DestroyCallbackInfo {
- DestroyCallbackInfo() {}
- DestroyCallbackInfo(lldb::callback_token_t token,
- lldb_private::DebuggerDestroyCallback callback,
- void *baton)
+ template <typename T> struct CallbackInfo {
+ CallbackInfo() {}
+ CallbackInfo(lldb::callback_token_t token, T callback, void *baton)
: token(token), callback(callback), baton(baton) {}
lldb::callback_token_t token;
- lldb_private::DebuggerDestroyCallback callback;
+ T callback;
void *baton;
};
- llvm::SmallVector<DestroyCallbackInfo, 2> m_destroy_callbacks;
+ template <typename T, typename ExtraT>
+ struct CallbackInfoExtraData : public CallbackInfo<T> {
+ CallbackInfoExtraData() {}
+ CallbackInfoExtraData(lldb::callback_token_t token, T callback, void *baton,
+ ExtraT extra_data)
+ : CallbackInfo<T>(token, callback, baton), extra_data(extra_data) {}
+ ExtraT extra_data;
+ };
+ static std::mutex s_create_callback_mutex;
+ static lldb::callback_token_t s_create_callback_next_token;
+ static llvm::SmallVector<
+ CallbackInfoExtraData<lldb_private::DebuggerCreateCallback, void *>, 2>
+ s_create_callbacks;
+
+ std::mutex m_destroy_callback_mutex;
+ lldb::callback_token_t m_destroy_callback_next_token = 0;
+ llvm::SmallVector<CallbackInfo<lldb_private::DebuggerDestroyCallback>, 2>
+ m_destroy_callbacks;
uint32_t m_interrupt_requested = 0; ///< Tracks interrupt requests
std::mutex m_interrupt_mutex;
diff --git a/lldb/include/lldb/lldb-private-types.h b/lldb/include/lldb/lldb-private-types.h
index b82a2b8aa05744..c997538f62827a 100644
--- a/lldb/include/lldb/lldb-private-types.h
+++ b/lldb/include/lldb/lldb-private-types.h
@@ -143,6 +143,8 @@ typedef struct type256 { uint64_t x[4]; } type256;
using ValueObjectProviderTy =
std::function<lldb::ValueObjectSP(ConstString, StackFrame *)>;
+typedef void (*DebuggerCreateCallback)(lldb::DebuggerSP debugger, void *baton,
+ void *original_callback);
typedef void (*DebuggerDestroyCallback)(lldb::user_id_t debugger_id,
void *baton);
typedef bool (*CommandOverrideCallbackWithResult)(
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 6b72994fc96afb..806415aa7b3de0 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1703,6 +1703,26 @@ void SBDebugger::SetDestroyCallback(
}
}
+lldb::callback_token_t
+SBDebugger::AddCreateCallback(lldb::SBDebuggerCreateCallback create_callback,
+ void *baton) {
+ LLDB_INSTRUMENT_VA(create_callback, baton);
+
+ DebuggerCreateCallback callback = [](lldb::DebuggerSP debugger, void *baton,
+ void *original_callback) {
+ SBDebugger sb_debugger(debugger);
+ lldb::SBDebuggerCreateCallback original_callback_func =
+ (lldb::SBDebuggerCreateCallback)original_callback;
+ original_callback_func(sb_debugger, baton);
+ };
+ return Debugger::AddCreateCallback(callback, baton, (void *)create_callback);
+}
+
+bool SBDebugger::RemoveCreateCallback(lldb::callback_token_t token) {
+ LLDB_INSTRUMENT_VA(token);
+ return Debugger::RemoveCreateCallback(token);
+}
+
lldb::callback_token_t
SBDebugger::AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
void *baton) {
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 9bdc5a3949751d..7fee7eee78f7c0 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -106,6 +106,13 @@ static Debugger::DebuggerList *g_debugger_list_ptr =
nullptr; // NOTE: intentional leak to avoid issues with C++ destructor chain
static llvm::DefaultThreadPool *g_thread_pool = nullptr;
+std::mutex Debugger::s_create_callback_mutex;
+lldb::callback_token_t Debugger::s_create_callback_next_token = 0;
+llvm::SmallVector<Debugger::CallbackInfoExtraData<
+ lldb_private::DebuggerCreateCallback, void *>,
+ 2>
+ Debugger::s_create_callbacks;
+
static constexpr OptionEnumValueElement g_show_disassembly_enum_values[] = {
{
Debugger::eStopDisassemblyTypeNever,
@@ -739,6 +746,15 @@ DebuggerSP Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
g_debugger_list_ptr->push_back(debugger_sp);
}
debugger_sp->InstanceInitialize();
+
+ // Invoke all debugger create callbacks.
+ {
+ std::lock_guard<std::mutex> guard(s_create_callback_mutex);
+ for (const auto &callback_info : s_create_callbacks) {
+ callback_info.callback(debugger_sp, callback_info.baton,
+ callback_info.extra_data);
+ }
+ }
return debugger_sp;
}
@@ -748,7 +764,7 @@ void Debugger::HandleDestroyCallback() {
// added during this loop will be appended, invoked and then removed last.
// Callbacks which are removed during this loop will not be invoked.
while (true) {
- DestroyCallbackInfo callback_info;
+ CallbackInfo<DebuggerDestroyCallback> callback_info;
{
std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
if (m_destroy_callbacks.empty())
@@ -1447,6 +1463,28 @@ void Debugger::SetDestroyCallback(
m_destroy_callbacks.emplace_back(token, destroy_callback, baton);
}
+lldb::callback_token_t Debugger::AddCreateCallback(
+ lldb_private::DebuggerCreateCallback create_callback, void *baton,
+ void *original_callback) {
+ std::lock_guard<std::mutex> guard(s_create_callback_mutex);
+ const lldb::callback_token_t token = s_create_callback_next_token++;
+ s_create_callbacks.emplace_back(token, create_callback, baton,
+ original_callback);
+ return token;
+}
+
+bool Debugger::RemoveCreateCallback(lldb::callback_token_t token) {
+ std::lock_guard<std::mutex> guard(s_create_callback_mutex);
+ for (auto it = s_create_callbacks.begin(); it != s_create_callbacks.end();
+ ++it) {
+ if (it->token == token) {
+ s_create_callbacks.erase(it);
+ return true;
+ }
+ }
+ return false;
+}
+
lldb::callback_token_t Debugger::AddDestroyCallback(
lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
std::lock_guard<std::mutex> guard(m_destroy_callback_mutex);
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
index 97a3837fd7aa62..f050fb3a214a6c 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
@@ -33,6 +33,7 @@ class SBStructuredData;
class SBFileSpec;
class SBModuleSpec;
class SBStringList;
+class SBDebugger;
} // namespace lldb
namespace lldb_private {
@@ -111,6 +112,8 @@ class SWIGBridge {
ToSWIGWrapper(std::unique_ptr<lldb::SBFileSpec> file_spec_sb);
static PythonObject
ToSWIGWrapper(std::unique_ptr<lldb::SBModuleSpec> module_spec_sb);
+ static PythonObject
+ ToSWIGWrapper(std::unique_ptr<lldb::SBDebugger> debugger_sb);
static python::ScopedPythonObject<lldb::SBCommandReturnObject>
ToSWIGWrapper(CommandReturnObject &cmd_retobj);
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 646ccce36530d4..6c7433f2c93163 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -286,3 +286,25 @@ def remove_bar(dbg_id):
('remove bar ret', False), # remove_bar should fail, because it's already invoked and removed
('foo called', original_dbg_id), # foo should be called
])
+
+ def test_AddRemoveDebuggerCreateCallback(self):
+ """
+ Test SBDebugger::AddCreateCallback and SBDebugger::RemoveCreateCallback
+ """
+ created_debuggers = []
+ def debugger_create_callback(debugger):
+ created_debuggers.append(debugger)
+
+ create_callback_token = lldb.SBDebugger.AddCreateCallback(debugger_create_callback)
+ debugger1 = lldb.SBDebugger.Create()
+ debugger2 = lldb.SBDebugger.Create()
+
+ lldb.SBDebugger.RemoveCreateCallback(create_callback_token)
+ debugger3 = lldb.SBDebugger.Create()
+
+ self.assertNotEqual(debugger1.GetID(), debugger2.GetID())
+ self.assertNotEqual(debugger1.GetID(), debugger3.GetID())
+
+ self.assertEqual(len(created_debuggers), 2)
+ self.assertEqual(debugger1.GetID(), created_debuggers[0].GetID())
+ self.assertEqual(debugger2.GetID(), created_debuggers[1].GetID())
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
I'm of two minds about this. OTOH, it is certainly useful to be able to observe Debugger creation. OTOH, it would be equally useful to be able to observe Target creation and Process creation and probably other "debugger lifecycle" events. One of the tasks I've not had time to get to yet is to make a "debugger lifecycle" extension to "target stop-hook" where you can add hooks to all these other lifecycle events. I worry a little bit that backing into such a system piecemeal will make the ultimate structure harder to implement. |
|
Also, if you are going to hook creation, you should also hook destruction, shouldn't you? Whatever accounting you are doing for a debugger's once it exists might need to be torn down when that debugger goes away. |
There was a problem hiding this comment.
As Jim was saying, we might be able to make something a bit more useful so that we can easily extend for other notifications of a debug session lifetime. Maybe instead of:
typedef void (*SBDebuggerCreateCallback)(lldb::SBDebugger &debugger, void *baton);
We can add something like:
enum NotificationType {
eDebuggerWasCreated,
eDebuggerWillBeDestroyed, // Call before debugger object is destroyed
eTargetWasCreated,
eTargetWillBeDestroyed,
eProcess...
};
typedef void (*SBNotificationCallback)(lldb::NotificationType type, lldb::SBDebugger &, lldb::SBExecutionContext &exe_ctx, void *baton);
Then we can easily add new registration callbacks that all use the same callback function.
Right, |
|
@jimingham , @clayborg, do you guys prefer @clayborg, for |
But using |
|
I have wanted to be able to hook into target creation to be able to setup targets when they get created. It would be also nice to get an event when a process is not longer being debugged due to process exit or detach. |
|
I don't think there's a lot of reason to support registering lists of lldb commands for the event, just having a python callback should be enough. I think it's better to just do a scripted callback, and then have some command that lets you add callbacks by module & class name, etc.
I do think that the callbacks from the hooks should pass back a "return control to the user?" boolean value - the way stop hooks do. That would be convenient for "return control to me when this particular target, process, etc is created". I would also like eventually to have the stop-hooks use this mechanism and it needs that feature.
The other thing I really don't like about the target stop-hook mechanism is that there's no general way to access the results of the hook. For instance, if I have a stop hook that displays some values, once that's printed to the terminal, there's no way to look at that report again. It would have been better if the stop hooks had a "result" type object the way the commands do, which it accumulates results to. Then when the process stopped, by default the driver would gather and print this info (it would also mean we could control when it's printed better). But you could still look it up later. In the case of stop-hooks you'd probably flush this on continue. Not sure what a good model for the other hooks is.
Jim
… On Oct 7, 2024, at 10:36 AM, jeffreytan81 ***@***.***> wrote:
@jimingham <https://github.com/jimingham> , @clayborg <https://github.com/clayborg>, do you guys prefer target stop-hook style approach or SBNotificationCallback style python API callback?
@clayborg <https://github.com/clayborg>, for SBNotificationCallback, does lldb::SBExecutionContext wrap the process, target or debugger object that is creating/destroying?
—
Reply to this email directly, view it on GitHub <#111206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW2TVG3VA2UB6Q2YXR3Z2LBARAVCNFSM6AAAAABPMR6QDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJXGUYTGNBTGQ>.
You are receiving this because you were mentioned.
|
|
Hello all, I'm parachuting in here because we also have a use case that might benefit from something like this. In our case, we want to run some actions (set up data formatters, source paths, etc.) if we find that we're working with a certain kind of binary (which contains information on how to find these). We're currently using a combination of "module loaded" events and stop hooks for this, but neither of them is ideal because the events are asynchronous (so lldb doesn't wait for our processing to finish), and stop hooks happen slightly too late. Ideally, we'd like our hook to run (synchronously) after target creation (or after its executable has been set). I don't really have much to say about the implementation except a note that a one-callback-to-rule-them all might not be ideal for the case where one wants to hook only a certain kind of events. Sure, you can do filtering in the callback, but it means you still get called, which may matter if some of the events occur frequently. This probably doesn't matter for target creation, but it may not be ideal to get called e.g. after every process stop if all you wanted to hook was debugger creation. |
|
On Oct 8, 2024, at 2:16 AM, Pavel Labath ***@***.***> wrote:
Hello all,
I'm parachuting in here because we also have a use case that might benefit from something like this. In our case, we want to run some actions (set up data formatters, source paths, etc.) if we find that we're working with a certain kind of binary (which contains information on how to find these). We're currently using a combination of "module loaded" events and stop hooks for this, but neither of them is ideal because the events are asynchronous (so lldb doesn't wait for our processing to finish), and stop hooks happen slightly too late. Ideally, we'd like our hook to run (synchronously) after target creation (or after its executable has been set).
I don't really have much to say about the implementation except a note that a one-callback-to-rule-them all might not be ideal for the case where one wants to hook only a certain kind of events. Sure, you can do filtering in the callback, but it means you still get called, which may matter if some of the events occur frequently. This probably doesn't matter for target creation, but it may not be ideal to get called e.g. after every process stop if all you wanted to hook was debugger creation.
I agree, a one-callback-to-rule-them-all approach would be awkward. It would be annoying go make everybody that writes targat hooks has to filter out the ones that come when other changes are reported. But I'm not sure that was what was being suggested. I understood the question to be between `target stop-hooks` which started life in a very lldb command-line centric view, and a purely script callback based approach. I think the latter is the better way to go.
But I was still expecting that you would specify at what level you wanted to be called, so you'd do something like:
SBDebugger::SetLifecycleCallback(lldb::eCallbackLevelTarget, "my_module.my_callback_class")
If all the entities we want to watch, we could make the callback take an ExecutionContext that conveys the new info. So "my_callback_class" would have:
class my_callback_class:
def __call__(self, exe_ctx : SBExecutionContext):
if you registered for Target level callback, the exe_ctx would have the new target (and the debugger if we decide to add that to the ExecutionContext.
However, we should think a bit about that. Do we also want to observe module loading this way (we don't have another way to do that?). If so, we can't JUST use the ExecutionContext to convey "what has happened" to the callback. So we either need to make different classes of callbacks that can receive different "interesting entities".
Jim
… —
Reply to this email directly, view it on GitHub <#111206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZ6HHEYMMCQF7KD7FDZ2OPIXAVCNFSM6AAAAABPMR6QDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZGMYDSMZQGI>.
You are receiving this because you were mentioned.
|
|
On Oct 8, 2024, at 9:53 AM, Jim Ingham ***@***.***> wrote:
>
>
>
>> On Oct 8, 2024, at 2:16 AM, Pavel Labath ***@***.***> wrote:
>>
>>
>> Hello all,
>>
>> I'm parachuting in here because we also have a use case that might benefit from something like this. In our case, we want to run some actions (set up data formatters, source paths, etc.) if we find that we're working with a certain kind of binary (which contains information on how to find these). We're currently using a combination of "module loaded" events and stop hooks for this, but neither of them is ideal because the events are asynchronous (so lldb doesn't wait for our processing to finish), and stop hooks happen slightly too late. Ideally, we'd like our hook to run (synchronously) after target creation (or after its executable has been set).
>>
>> I don't really have much to say about the implementation except a note that a one-callback-to-rule-them all might not be ideal for the case where one wants to hook only a certain kind of events. Sure, you can do filtering in the callback, but it means you still get called, which may matter if some of the events occur frequently. This probably doesn't matter for target creation, but it may not be ideal to get called e.g. after every process stop if all you wanted to hook was debugger creation.
>>
> I agree, a one-callback-to-rule-them-all approach would be awkward. It would be annoying go make everybody that writes targat hooks has to filter out the ones that come when other changes are reported. But I'm not sure that was what was being suggested. I understood the question to be between `target stop-hooks` which started life in a very lldb command-line centric view, and a purely script callback based approach. I think the latter is the better way to go.
>
> But I was still expecting that you would specify at what level you wanted to be called, so you'd do something like:
>
> SBDebugger::SetLifecycleCallback(lldb::eCallbackLevelTarget, "my_module.my_callback_class")
>
> If all the entities we want to watch
[missing phrase]
are contained in the ExecutionContext
…> , we could make the callback take an ExecutionContext that conveys the new info. So "my_callback_class" would have:
>
> class my_callback_class:
> def __call__(self, exe_ctx : SBExecutionContext):
>
> if you registered for Target level callback, the exe_ctx would have the new target (and the debugger if we decide to add that to the ExecutionContext.
>
> However, we should think a bit about that. Do we also want to observe module loading this way (we don't have another way to do that?). If so, we can't JUST use the ExecutionContext to convey "what has happened" to the callback. So we either need to make different classes of callbacks that can receive different "interesting entities".
>
> Jim
>
>
>> —
>> Reply to this email directly, view it on GitHub <#111206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZ6HHEYMMCQF7KD7FDZ2OPIXAVCNFSM6AAAAABPMR6QDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZGMYDSMZQGI>.
>> You are receiving this because you were mentioned.
>>
>
>
|
jeffreytan81
force-pushed
the
add_debugger_create_callback
branch
from
4f11996 to
d5820bd
Compare
jeffreytan81
force-pushed
the
add_debugger_create_callback
branch
from
d5820bd to
f2d35f6
Compare
|
The PR is revised based on feedback. In this PR, it supports Let me know if this makes sense. |
jeffreytan81
changed the title
| @@ -1362,6 +1362,12 @@ enum Severity { | |||
| eSeverityInfo, // Equivalent to Remark used in clang. | |||
| }; | |||
|
|
|||
| enum NotificationType { | |||
| eDebuggerWillBeCreated = (1 << 0), | |||
There was a problem hiding this comment.
Should this be eDebuggerWasCreated to indicate that a debugger object was created and that the user can do something with it?
There was a problem hiding this comment.
Yes, it's not useful to call this before the new Debugger is fully initialized.
| lldb::SBDebugger &, | ||
| lldb::SBExecutionContext &exe_ctx, |
There was a problem hiding this comment.
Can we add accessors to the SBExecutionContext for accessing a debugger? We can modify the lldb_private::ExecutionContext to add a lldb::DebuggerSP and add accessors to the debugger object and then add that to the `lldb::SBExecutionContext" API?
There was a problem hiding this comment.
It's always been a little weird that ExecutionContext didn't have a debugger - though TTTT that only mattered for default constructed ones: so long as any of the other entities are filled in you can get to the debugger.
OTOH, that's a pretty big patch so if we want to go that way, that should be a separate patch.
Another way to do this would be to add an SBExecutionContext::GetDebugger and manage that all on the SB side? After all, we haven't actually needed a Debugger in the ExecutionContext that I can remember, and this would be a much more limited change.
I'm also fine with passing the debugger alongside the SBExecutionContext, we do that in some other places.
| @@ -569,6 +569,18 @@ class Debugger : public std::enable_shared_from_this<Debugger>, | |||
| SetDestroyCallback(lldb_private::DebuggerDestroyCallback destroy_callback, | |||
| void *baton); | |||
|
|
|||
| /// Add a notification callback when notification type event happens. Return a | |||
There was a problem hiding this comment.
Read literally this says you are adding the notification callback when the event happens. Maybe:
Add a notification callback that will trigger when an event of the given NotificationType occurs.
You should also state when the creation and destruction callbacks trigger w.r.t. the action the event is reporting. I.e. Creation callbacks trigger synchronously after the given object is fully set up, and Destruction callbacks trigger right before the object is destroyed.
| /// Add a notification callback when notification type event happens. Return a | ||
| /// token, which can be used to remove said callback. Multiple callbacks can | ||
| /// be added by calling this function multiple times, and will be invoked in | ||
| /// FIFO order. |
There was a problem hiding this comment.
What is original_callback for? You should document this if it's needed.
| @@ -1362,6 +1362,12 @@ enum Severity { | |||
| eSeverityInfo, // Equivalent to Remark used in clang. | |||
| }; | |||
|
|
|||
| enum NotificationType { | |||
There was a problem hiding this comment.
Might be good to put a comment here saying (a) this is used for SBNotificationCallbacks and (b) this is where you should add any other notification callbacks (i.e. target creation and destruction...) That should be made clear so we don't invent another mechanism for this elsewhere...
|
I couldn't see what the We should also add a command way to add these callbacks, but it doesn't make sense to do that for the debugger created and destroyed ones because the command interpreter really lives inside of one debugger. So I don't think you need to do that in this patch. |
| }; | ||
| static std::mutex s_notification_callback_mutex; | ||
| static lldb::callback_token_t s_notification_callback_next_token; | ||
| static llvm::SmallVector< |
There was a problem hiding this comment.
Every action we perform with the notifications except for "deleting by token" knows the type of the notification that it's adding/looking up. We could even encode the type in the token and we'd always know. So it seems awkward to store them in a flat list. When we get around to listing these and other management tasks, those will very likely also be requests by type.
Maybe storing them in a map of type -> vector of callbacks for that type would be a better data structure here?
|
(This is in reply to Jim's comment here. I can't quote it, because github garbles it beyond recognition, probably because it was sent by email :/) You're right, I have misunderstood the intention. The thing that threw me off was the discussion of about the callback signature (which still looks like it wants to be universal). Given that, I'd like to agree with what you said in the last part of your email -- I think it'd be better for the callback signatures to not be universal as well (no one-signature-to-rule-them-all). Here are my reasons for that:
If we do this right, I don't believe this should result in a lot of duplicated code, as we could write (template?) code to handle the (de)registration in a generic way. |
| enum NotificationType { | ||
| eDebuggerWillBeCreated = (1 << 0), | ||
| eDebuggerWillBeDestroyed = | ||
| (1 << 1), // Call before debugger object is destroyed |
There was a problem hiding this comment.
Do we actually want to treat this as a bitfield? If not, maybe they should just get the usual sequential numbers?
| original_callback_func(type, sb_debugger, sb_exe_ctx, baton); | ||
| }; | ||
| return Debugger::AddNotificationCallback(type, callback, baton, | ||
| (void *)notification_callback); |
| lldb::SBNotificationCallback original_callback_func = | ||
| (lldb::SBNotificationCallback)original_callback; |
There was a problem hiding this comment.
| lldb::SBNotificationCallback original_callback_func = | |
| (lldb::SBNotificationCallback)original_callback; | |
| auto original_callback_func = | |
| reinterpret_cast<lldb::SBNotificationCallback>(original_callback); |
(use c++ cast, and avoid repeating the type)
| std::lock_guard<std::mutex> guard(s_notification_callback_mutex); | ||
| for (const auto &callback_info : s_notification_callbacks) { | ||
| if ((callback_info.type & notify_type) == notify_type) | ||
| callback_info.callback(notify_type, debugger_sp, nullptr, |
There was a problem hiding this comment.
Note this will deadlock if any of the callbacks try to register another callback.
You may want to consider making a copy of the list and iterating over that (without holding the mutex).
| // Create a tuple of arguments (type, debugger, exe_ctx) | ||
| PyObject *args = PyTuple_New(3); | ||
|
|
||
| // Add NotificationType as an integer to the tuple | ||
| PyTuple_SetItem(args, 0, PyLong_FromLong(static_cast<long>(type))); | ||
|
|
||
| // Add debugger and exe_ctx to the tuple | ||
| Py_INCREF(debugger_arg.get()); | ||
| PyTuple_SetItem(args, 1, debugger_arg.get()); | ||
|
|
||
| Py_INCREF(exe_ctx_arg.get()); | ||
| PyTuple_SetItem(args, 2, exe_ctx_arg.get()); | ||
|
|
||
| // Call the Python function with the tuple of arguments (type, debugger, exe_ctx) | ||
| PyObject *result = PyObject_CallFunction( | ||
| reinterpret_cast<PyObject *>(baton), const_cast<char *>("O"), args); | ||
|
|
||
| // Clean up | ||
| Py_XDECREF(result); | ||
| Py_DECREF(args); // Decrement reference count for args |
There was a problem hiding this comment.
Could this use PythonObject wrappers instead?
Something like PythonCallable(baton)(PythonInteger(type), debugger_arg, exe_ctx_arg) ?
| void Debugger::HandleDestroyCallback() { | ||
| const lldb::user_id_t user_id = GetID(); | ||
| // Invoke and remove all the callbacks in an FIFO order. Callbacks which are | ||
| // added during this loop will be appended, invoked and then removed last. | ||
| // Callbacks which are removed during this loop will not be invoked. | ||
| while (true) { | ||
| DestroyCallbackInfo callback_info; | ||
| CallbackInfo<DebuggerDestroyCallback> callback_info; |
There was a problem hiding this comment.
How about treating the destroy callbacks as (legacy) synonyms for NotificationCallbacks of the "destroy" kind?
Basically, have SBDebugger::AddDestroyCallback do a AddNotificationCallback(eDebuggerWillBeDestroyed, callback, baton)
Then we could delete all of this code...
I'm trying to remember not to believe the bit where the footer in the email says you can reply to it directly. But it's SO convenient...
I don't think we need to have very many of these signatures, but I can think of more than just ExecutionContext ones pretty easily. Your example of Symbol side ones later on is the other really obvious example. And as you say, notifications are fine if you don't need to do something before the rest of the system sees the change, but sometimes you very much want to do that, particularly for module loading. So having a callback for this makes a lot of sense. It also seems to me that Platform lifecycle events would be something you might want callbacks for, and they don't neatly fit in the ExecutionContext mold. It's worthwhile to have a unified system for this. For instance, sometimes you want to observe and record information about some change, but you really don't want to junk up the console output with the results of that information. So having a way for the callback to produce a stored result, and then some commands to say "show me the stored results for callback X" or even "show me the stored results for all callbacks between StopID 100 and StopID 200, etc. would be handy. It would be sad if callback writers had to roll their own version of this sort of convenience...
Yes, particularly in environments like Xcode where all the concurrent debug session to a device share one lldb, it would be useful to scope these at least to the debugger. I think it's worth figuring out a convenient way to do this.
Registration should just require a callback type, that part should be easy. To do this in a generic manner a callback entity would also need a filter, and a "callback-hit-instance-data". The filter would take the instance data, but and get a yes or no back. Other than that the generic callback management shouldn't need to reason about either of these bits, just pass them to the filter and the callback. This should be amenable to a generic approach. |
Something I have wanted for a long time is a stop hook just BEFORE the target is created, to that I can change the setting for target.debug-file-search-paths before LLDB tries to read all the debug info (that it can't find because the search path setting hasn't been properly set up for us). |
|
That seems a common enough problem that there should be a more straightforward way to do it than having to barge into the middle of target setup...
The SBPlatform already has a "module location callback" that you can hook in to find debug modules in a programmatic way. It also knows things like the SDK Path on the host. So it seems consistent with this design that you could put the "debug file location for this platform" into the platform, and then pass the platform to the target as you create it (you can already do that), and have the Target consult that first for where to find debug files.
Jim
… On Oct 16, 2024, at 1:19 PM, cmtice ***@***.***> wrote:
I have wanted to be able to hook into target creation to be able to setup targets when they get created. It would be also nice to get an event when a process is not longer being debugged due to process exit or detach.
Something I have wanted for a long time is a stop hook just BEFORE the target is created, to that I can change the setting for target.debug-file-search-paths before LLDB tries to read all the debug info (that it can't find because the search path setting hasn't been properly set up for us).
—
Reply to this email directly, view it on GitHub <#111206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWYWL24PEAN2SVJDXITZ33C4TAVCNFSM6AAAAABPMR6QDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXHA3DKMZUHA>.
You are receiving this because you were mentioned.
|
| DestroyCallbackInfo(lldb::callback_token_t token, | ||
| lldb_private::DebuggerDestroyCallback callback, | ||
| void *baton) | ||
| template <typename T> struct CallbackInfo { |
There was a problem hiding this comment.
IIUC, CallbackInfo is the base template callback class for all kinds of callback (debugger create/destroy, and some other callbacks to be added in the future). It's better to place it somewhere else to make easier to extended in the future.
|
Is there any update on this PR? I'm interested in working on a similar callback that can be triggered when target is created, but I want to wait for this one to get landed first as they will share some common code. |
|
There were a bunch of review comments that need to be addressed. But I also am a bit nervous about adding a feature that is intended to be a general "lldb interesting classes lifecycle notification" but when designed only supports Debugger Lifecycle Events. Those are the simplest and least common of all the events we'd want to handle. |
This PR adds new
SBDebugger::AddNotificationCallback()\RemoveNotificationCallback()APIs.This API enables any lldb client to subscribe to interesting events in debugger lifecycle. For this PR, I am supporting two notification types(eDebuggerWillBeCreated and eDebuggerWillBeDestroyed). Future PRs can extend and add more types as desired.