royitaqi wrote:
Hi @JDevlieghere ,
The PR has changed quite a bit. If you have the time, maybe it's a good time to
take another look.
Below is a TL;DR of some of the changes:
* Aadded Add/Remove
* Leave Set unchanged (but marked as deprecated)
* Added token in Add/Remove
* Handle concurrency
royitaqi wrote:
Hi @jimingham and @bulbazord ,
I *think* I have addressed all your comments (thanks for those!). I have also
eye-balled through my PR again. But it's possible that I missed something.
Will be great to have you taking a look when you have the time.
Thanks,
Roy
@@ -321,13 +321,22 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
+ /// DEPRECATED: We used to only support one Destroy callback. Now that we
+ /// support Add and Remove, you should only remove Destroy
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/12] Allow multiple destroy callbacks in
@@ -321,13 +321,22 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
+ /// DEPRECATED: We used to only support one Destroy callback. Now that we
+ /// support Add and Remove, you should only remove Destroy
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -743,9 +743,19 @@ DebuggerSP
Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
}
void Debugger::HandleDestroyCallback() {
- if (m_destroy_callback) {
-m_destroy_callback(GetID(), m_destroy_callback_baton);
-m_destroy_callback = nullptr;
+
@@ -321,13 +321,22 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
+ /// DEPRECATED: We used to only support one Destroy callback. Now that we
+ /// support Add and Remove, you should only remove Destroy
@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
- void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
- void *baton);
+
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/11] Allow multiple destroy callbacks in
@@ -1686,13 +1686,33 @@ void
SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
}
}
-void SBDebugger::SetDestroyCallback(
-lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+lldb::SBDebuggerDestroyCallbackToken
@@ -568,10 +569,22 @@ class Debugger : public
std::enable_shared_from_this,
static void ReportSymbolChange(const ModuleSpec _spec);
- void
+ /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
+ /// Clear all previously added callbacks and only
@@ -121,6 +121,7 @@ typedef struct type256 { uint64_t x[4]; } type256;
using ValueObjectProviderTy =
std::function;
+typedef int DebuggerDestroyCallbackToken;
royitaqi wrote:
Added `lldb::destroy_callback_token_t`.
@@ -743,10 +743,11 @@ DebuggerSP
Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
}
void Debugger::HandleDestroyCallback() {
- if (m_destroy_callback) {
-m_destroy_callback(GetID(), m_destroy_callback_baton);
-m_destroy_callback = nullptr;
+ const
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 01/10] Allow multiple destroy callbacks in
@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
- void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
- void *baton);
+
jimingham wrote:
> Hi @jimingham ,
>
> I have updated the code. Now we have `Add` and `Remove` (with tokens as you
> suggested). `Set` clears everything then add the given one (also returns a
> token).
>
> Thread-safety is done through a `std::lock_guard`. This
> is consistent with some of
@@ -321,8 +321,15 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
- void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
- void *baton);
+
@@ -743,10 +743,11 @@ DebuggerSP
Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
}
void Debugger::HandleDestroyCallback() {
- if (m_destroy_callback) {
-m_destroy_callback(GetID(), m_destroy_callback_baton);
-m_destroy_callback = nullptr;
+ const
@@ -1686,13 +1686,33 @@ void
SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
}
}
-void SBDebugger::SetDestroyCallback(
-lldb::SBDebuggerDestroyCallback destroy_callback, void *baton) {
+lldb::SBDebuggerDestroyCallbackToken
@@ -568,10 +569,22 @@ class Debugger : public
std::enable_shared_from_this,
static void ReportSymbolChange(const ModuleSpec _spec);
- void
+ /// DEPRECATED. Use AddDestroyCallback and RemoveDestroyCallback instead.
+ /// Clear all previously added callbacks and only
@@ -121,6 +121,7 @@ typedef struct type256 { uint64_t x[4]; } type256;
using ValueObjectProviderTy =
std::function;
+typedef int DebuggerDestroyCallbackToken;
jimingham wrote:
There's no need to make separate lldb_private & SB versions of the
royitaqi wrote:
Hi @jimingham ,
I have updated the code. Now we have `Add` and `Remove` (with tokens as you
suggested). `Set` clears everything then add the given one (also returns a
token).
Thread-safety is done through a `std::lock_guard`. This
is consistent with some of the other
@@ -743,10 +743,11 @@ DebuggerSP
Debugger::CreateInstance(lldb::LogOutputCallback log_callback,
}
void Debugger::HandleDestroyCallback() {
- if (m_destroy_callback) {
-m_destroy_callback(GetID(), m_destroy_callback_baton);
-m_destroy_callback = nullptr;
+ const
@@ -1425,10 +1426,19 @@ void
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
std::make_shared(log_callback, baton);
}
+void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+
@@ -1425,10 +1426,19 @@ void
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
std::make_shared(log_callback, baton);
}
+void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+
@@ -321,9 +321,14 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
+ void AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+ void *baton);
+
void
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/8] Allow multiple destroy callbacks in
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/7] Allow multiple destroy callbacks in
jimingham wrote:
For legacy reasons, I think we have to keep SetDestroyCallbacks doing what it
did, but we should comment that it is deprecated and to use the Add and Remove
to only affect your own callbacks.
Jim
> On Apr 24, 2024, at 6:19 PM, royitaqi ***@***.***> wrote:
>
>
> @royitaqi
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
@@ -321,9 +321,14 @@ class LLDB_API SBDebugger {
void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
+ void AddDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
+ void *baton);
+
void
@@ -1425,10 +1426,19 @@ void
Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
std::make_shared(log_callback, baton);
}
+void Debugger::AddDestroyCallback(
+lldb_private::DebuggerDestroyCallback destroy_callback, void *baton) {
+
royitaqi wrote:
# Tests that I did
## Manual test
```
username-mac ~/public_llvm/build % bin/lldb
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> lldb.debugger.AddDestroyCallback(lambda user_id: print('foo %s' % user_id))
>>>
royitaqi wrote:
Hi @JDevlieghere and @jimingham,
I have updated the PR to add `AddDestroyCallback()`, `ClearDestroyCallback()`,
and tests for these. Also updated `SetDestroyCallback()` to work with the new
field `std::vector>`.
Hope it looks better now. LMK if anything else needs to be
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/6] Allow multiple destroy callbacks in
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/5] Allow multiple destroy callbacks in
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/4] Allow multiple destroy callbacks in
jimingham wrote:
I don't have a strong preference for new PR vrs. reuse this one.
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
royitaqi wrote:
Thank you, @JDevlieghere and @jimingham for the input.
IIUR, you are basically advocating for the following alternative approach (that
was mentioned in the "Alternatives considered" section in the above):
> Instead of changing SetDestroyCallback(), a new method
jimingham wrote:
I would also find it surprising if "SetDestroyCallback" did
"AddDestroyCallback". That's not what the name says it does. I have no
problem with supporting multiple Destroy callbacks. But I agree with Jonas,
that needs to be a separate API with an appropriate name. And
https://github.com/JDevlieghere requested changes to this pull request.
Can you elaborate a bit more about why you want to change the behavior? Your
motivation touches on the fact that it might be surprising or racy. While I
think having the ability to register multiple callbacks makes sense,
https://github.com/royitaqi updated
https://github.com/llvm/llvm-project/pull/89868
>From 079a550481d4cdcb69ad01c376b5e1f0632a07d6 Mon Sep 17 00:00:00 2001
From: Roy Shi
Date: Tue, 23 Apr 2024 18:10:21 -0700
Subject: [PATCH 1/2] Allow multiple destroy callbacks in
https://github.com/royitaqi edited
https://github.com/llvm/llvm-project/pull/89868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: None (royitaqi)
Changes
# Motivation
Individual callers of `SBDebugger::SetDestroyCallback()` might think that they
have registered their callback and expect it to be called when the debugger is
destroyed. In reality, only the last
github-actions[bot] wrote:
Thank you for submitting a Pull Request (PR) to the LLVM Project!
This PR will be automatically labeled and the relevant teams will be
notified.
If you wish to, you can add reviewers by using the "Reviewers" section on this
page.
If this is not working for you,
https://github.com/royitaqi created
https://github.com/llvm/llvm-project/pull/89868
# Motivation
Individual callers of `SBDebugger::SetDestroyCallback()` might think that they
have registered their callback and expect it to be called when the debugger is
destroyed. In reality, only the last
50 matches
Mail list logo
