SBDebugger: Add new APIs AddDestroyCallback and RemoveDestroyCallback
#89868
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: None (royitaqi) ChangesMotivationIndividual callers of This PRAllows multiple destroy callbacks to be registered and all called when the debugger is destroyed. Semantic change to
|
There was a problem hiding this comment.
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, I'm not sure I agree with either.
- The method is called
SetDestroyCallbackwhich makes it pretty clear that it's a setter, which is generally expected to change the value, rather than add to it. If the method had been calledAddDestroyCallbackon the other hand, I would expect it to add the callback rather than overwrite it. - I don't think there's anything racy about a setter. Even if it were, nothing in this PR addresses the racy-ness.
The setter also makes it possible to remove a callback by overwriting it with a callback that does nothing. With the current approach that's no longer possible. I'm not sure if that's important, but something to consider.
If the goal is to be able to specify multiple callbacks, a potential solution could be to store the callbacks in a list as you do in this PR, but add a new method AddDestroyCallback which appends to the list. We could keep the existing behavior for SetDestroyCallback by clearing the list and adding it as the first entry. That would also solve the problem of not being able to remove existing callbacks.
Regardless of the direction, this definitely needs a corresponding test and updated documentation.
|
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 also tests... |
|
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):
I agree that adding new methods have more clear naming and don't change the semantics of the eixsting method. I will update this PR (with test). Or, should I create a new PR? - Noob I am. |
|
I don't have a strong preference for new PR vrs. reuse this one. |
|
Hi @JDevlieghere and @jimingham, I have updated the PR to add Hope it looks better now. LMK if anything else needs to be added/changed. --
|
TestsManual testRun the added unit tests |
|
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 commented on this pull request.
In lldb/include/lldb/API/SBDebugger.h <#89868 (comment)>:
> void SetDestroyCallback(lldb::SBDebuggerDestroyCallback destroy_callback,
void *baton);
+ void ClearDestroyCallback();
It seems wrong that one client can only remove its destroy callback by removing ones it didn't install.
Makes sense to me.
I think you are suggesting:
TToken AddDestroyCallback(callback, baton)
bool RemoveDestroyCallback(TToken token) - returns success/fail
No/remove ClearDestroyCallbacks - because no one is supposed to clear
Then what should SetDestroyCallback be?
It clears existing callbacks, so I think in this new world this semantics isn't allowed, similar to how ClearDestroyCallbacks shouldn't exist.
That is, unless, we introduce the concept of a client ID, where SetDestroyCallback will remove all callbacks from the specified client ID. But then I feel the introduction of the client ID opens up even more questions, and it's not an existing pattern in the Debugger class. So it's probably not a good idea.
—
Reply to this email directly, view it on GitHub <#89868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5DTSDZRU33KPW67BTY7BK23AVCNFSM6AAAAABGWDO5TCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRRGIYTQMRVGY>.
You are receiving this because you were mentioned.
|
|
Hi @jimingham , I have updated the code. Now we have Thread-safety is done through a Didn't understand one of your comment about "one destroy callback can add/remove others" and how that relates to the code. See my reply in the above. Another thing I am not sure is, in python API test, how do I make concurrent calls to these APIs. Do you know if this should be tested (e.g. if other existing functions has similar tests)? If the answer is "yes", do you happen to know what existing test I can see as examples? Thanks for your reviews so far. Hope this PR is getting better as we converse. Best, |
|
Gentle ping to make progress on this PR. :) @JDevlieghere , could you see my rely to your comment about why I think signed int makes sense for @JDevlieghere and @bulbazord , about the necessity of thread safety, could you see if my proposal is agreeable? TL;DR: currently the class isn't thread safe; let's skip thread safety in this PR; we can add it to the whole class in one go when, in the future, the class is decided to need thread safety. |
|
On May 7, 2024, at 2:51 PM, royitaqi ***@***.***> wrote:
@royitaqi commented on this pull request.
In lldb/include/lldb/Core/Debugger.h <#89868 (comment)>:
> @@ -731,8 +747,11 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
lldb::TargetSP m_dummy_target_sp;
Diagnostics::CallbackID m_diagnostics_callback_id;
- lldb_private::DebuggerDestroyCallback m_destroy_callback = nullptr;
- void *m_destroy_callback_baton = nullptr;
+ std::recursive_mutex m_destroy_callback_mutex;
BTW:
I think you need to at least protect adding.
To me, it's either none, or all.
IME you don't want to just keep adding mutexes willy-nilly, you do want to know that the resource could be contended for by operations on more than one thread. My only point was that we couldn't think of a plausible reason to lock when iterating for destroy, but it's easy to see that you need to lock adding and removing since there's nothing that keeps that from happening.
If we decide to protect adding (multiple adds from different threads not during destroy), then we should just protect all of add/remove/destroy, because any of these operations can happen at the same time from multiple threads.
FWIW I agree with what @JDevlieghere <https://github.com/JDevlieghere> said:
I'd say let's not overthink this and if we want to change that, let's do it holistically for the whole class instead of piecemeal.
I didn't really understand that statement. Adding debuggers, or querying them is controlled by the debugger list mutex. Manipulating the I/O handlers is governed by the io handler stack mutex, there's a lock around the interrupt requests. Debugger doesn't seem like a class that is not locking the lists of things it controls as a general rule.
When you have a collection that can be accessed from multiple threads, adding and removing, then you really do need to protect those accesses. I'm not sure it's good reasoning to say "That's not likely, let's wait till it crashes on someone to add the protection".
Jim
… —
Reply to this email directly, view it on GitHub <#89868 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVWZU2E5VV5VVISC4TFTZBFEFHAVCNFSM6AAAAABGWDO5TCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBUGI2TSMBXGM>.
You are receiving this because you were mentioned.
|
This is fair. I somehow missed them the last time I was searching for mutex in the same code. -- With the above, I'm on the fence. Since I have less knowledge and context, at this point I will depend on @jimingham & @JDevlieghere you guys aligning with each other and pointing me in the right direction. LMK. Either ways it should be an easy change in the PR (since we already have thread-safety implemented). |
|
Gentle ping. :) @jimingham and @JDevlieghere I'm hoping that you guys can align on whether or not this PR should contain thread-safety. My thoughts (from my last comment):
-- Note: in below, mistakenly closed and reopened this PR. |
lldb/source/Core/Debugger.cpp
Outdated
| // Using `token` to erase, because elements may have been added/removed, and | ||
| // that will cause error "invalid iterator access!" if `iter` is used | ||
| // instead. | ||
| m_destroy_callback_and_baton.erase(token); |
There was a problem hiding this comment.
Any reason we remove after calling the callback?
There was a problem hiding this comment.
The callbacks, when executed by this loop, can add/remove other callbacks. I.e. the content in the container can change during the loop.
To handle this, the loop always take the first element, call it, then remove it, so that the next "first element" will be a new one.
If we don't remove, I wonder how we can make sure that the callbacks which are added during the loop are executed.
There was a problem hiding this comment.
The biggest concern is that, this implementation completely changes the semantics -- now, client using this API has to re-register the callbacks after destroying debugger which is weird and not expected because there is no removeCallback() called from client. For example, the following user case won't work anymore:
void MyDestroyCallback() {
...
}
SBDebugger::AddDestroyCallbac(MyDestroyCallback);
SBDebugger::Create();
...
SBDebugger::Destroy();
// Recreate another debugger
SBDebugger::Create();
SBDebugger::Destroy(); // Now the MyDestroyCallback won't be called even user did not call RemoveDestroyCallback() which is not expected
There are several ways to handle this issue without clearing m_destroy_callback_and_baton. One simply way is making a copy of m_destroy_callback_and_baton, and calling callbacks from the copy (by checking if it still exists in original m_destroy_callback_and_baton). And at the end, checking there is no new entries in m_destroy_callback_and_baton, otherwise, getting the delta of the local copy and original copy, and redo the process in a loop.
There was a problem hiding this comment.
The callbacks shouldn't be removed when called, that is for sure
There was a problem hiding this comment.
I was more concerned if AddDestroyCallback() is a static API but since it is an instance method, I am less concerned now.
lldb/include/lldb/lldb-types.h
Outdated
| @@ -62,12 +62,15 @@ typedef void *thread_arg_t; // Host thread argument type | |||
| typedef void *thread_result_t; // Host thread result type | |||
| typedef void *(*thread_func_t)(void *); // Host thread function type | |||
| typedef int pipe_t; // Host pipe type | |||
| typedef int destroy_callback_token_t; // Debugger destroy callback token type | |||
There was a problem hiding this comment.
Since we are adding this to the public API, we should remove the "destroy" from this so we could re-use this for other callback tokens. So change this to:
typedef int callback_token_t;
And remove the comment since making a comment of // Callback token type doesn't really tell you anything more than the name already does. The reason being we don't want to have to add a ton of callback token types for every callback we might want to register in the future (process_callback_token_t, thread_callback_token_t, ...). We can just use one for all of them in the future.
You can test this locally with the following command:darker --check --diff -r 7ecdf620330d8e044a48b6f59f8eddd2f88f01d4...25a51ed4e87042411b03ba4e57cb4a595679c37b lldb/test/API/python_api/debugger/TestDebuggerAPI.pyView the diff from darker here.--- TestDebuggerAPI.py 2024-05-17 00:49:05.000000 +0000
+++ TestDebuggerAPI.py 2024-05-20 22:39:20.840610 +0000
@@ -167,69 +167,72 @@
called = []
def foo(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal called
- called += [('foo', dbg_id)]
+ called += [("foo", dbg_id)]
def bar(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal called
- called += [('bar', dbg_id)]
+ called += [("bar", dbg_id)]
token_foo = self.dbg.AddDestroyCallback(foo)
token_bar = self.dbg.AddDestroyCallback(bar)
self.dbg.Destroy(self.dbg)
# Should call both `foo()` and `bar()`.
- self.assertEqual(called, [
- ('foo', original_dbg_id),
- ('bar', original_dbg_id),
- ])
+ self.assertEqual(
+ called,
+ [
+ ("foo", original_dbg_id),
+ ("bar", original_dbg_id),
+ ],
+ )
def test_RemoveDestroyCallback(self):
original_dbg_id = self.dbg.GetID()
called = []
def foo(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal called
- called += [('foo', dbg_id)]
+ called += [("foo", dbg_id)]
def bar(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal called
- called += [('bar', dbg_id)]
+ called += [("bar", dbg_id)]
token_foo = self.dbg.AddDestroyCallback(foo)
token_bar = self.dbg.AddDestroyCallback(bar)
ret = self.dbg.RemoveDestroyCallback(token_foo)
self.dbg.Destroy(self.dbg)
# `Remove` should be successful
self.assertTrue(ret)
# Should only call `bar()`
- self.assertEqual(called, [('bar', original_dbg_id)])
+ self.assertEqual(called, [("bar", original_dbg_id)])
def test_RemoveDestroyCallback_invalid_token(self):
original_dbg_id = self.dbg.GetID()
magic_token_that_should_not_exist = 32413
called = []
def foo(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal called
- called += [('foo', dbg_id)]
+ called += [("foo", dbg_id)]
token_foo = self.dbg.AddDestroyCallback(foo)
ret = self.dbg.RemoveDestroyCallback(magic_token_that_should_not_exist)
self.dbg.Destroy(self.dbg)
# `Remove` should be unsuccessful
self.assertFalse(ret)
# Should call `foo()`
- self.assertEqual(called, [('foo', original_dbg_id)])
+ self.assertEqual(called, [("foo", original_dbg_id)])
def test_HandleDestroyCallback(self):
"""
Validates:
1. AddDestroyCallback and RemoveDestroyCallback work during debugger destroy.
@@ -240,45 +243,51 @@
bar_token = None
def foo(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal events
- events.append(('foo called', dbg_id))
+ events.append(("foo called", dbg_id))
def bar(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal events
- events.append(('bar called', dbg_id))
+ events.append(("bar called", dbg_id))
def add_foo(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal events
- events.append(('add_foo called', dbg_id))
- events.append(('foo token', self.dbg.AddDestroyCallback(foo)))
+ events.append(("add_foo called", dbg_id))
+ events.append(("foo token", self.dbg.AddDestroyCallback(foo)))
def remove_bar(dbg_id):
# Need nonlocal to modify closure variable.
nonlocal events
- events.append(('remove_bar called', dbg_id))
- events.append(('remove bar ret', self.dbg.RemoveDestroyCallback(bar_token)))
+ events.append(("remove_bar called", dbg_id))
+ events.append(("remove bar ret", self.dbg.RemoveDestroyCallback(bar_token)))
# Setup
- events.append(('add_foo token', self.dbg.AddDestroyCallback(add_foo)))
+ events.append(("add_foo token", self.dbg.AddDestroyCallback(add_foo)))
bar_token = self.dbg.AddDestroyCallback(bar)
- events.append(('bar token', bar_token))
- events.append(('remove_bar token', self.dbg.AddDestroyCallback(remove_bar)))
+ events.append(("bar token", bar_token))
+ events.append(("remove_bar token", self.dbg.AddDestroyCallback(remove_bar)))
# Destroy
self.dbg.Destroy(self.dbg)
- self.assertEqual(events, [
- # Setup
- ('add_foo token', 0), # add_foo should be added
- ('bar token', 1), # bar should be added
- ('remove_bar token', 2), # remove_bar should be added
- # Destroy
- ('add_foo called', original_dbg_id), # add_foo should be called
- ('foo token', 3), # foo should be added
- ('bar called', original_dbg_id), # bar should be called
- ('remove_bar called', original_dbg_id), # remove_bar should be called
- ('remove bar ret', False), # remove_bar should fail, because it's already invoked and removed
- ('foo called', original_dbg_id), # foo should be called
- ])
+ self.assertEqual(
+ events,
+ [
+ # Setup
+ ("add_foo token", 0), # add_foo should be added
+ ("bar token", 1), # bar should be added
+ ("remove_bar token", 2), # remove_bar should be added
+ # Destroy
+ ("add_foo called", original_dbg_id), # add_foo should be called
+ ("foo token", 3), # foo should be added
+ ("bar called", original_dbg_id), # bar should be called
+ ("remove_bar called", original_dbg_id), # remove_bar should be called
+ (
+ "remove bar ret",
+ False,
+ ), # remove_bar should fail, because it's already invoked and removed
+ ("foo called", original_dbg_id), # foo should be called
+ ],
+ )
|
|
@royitaqi Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Hi @royitaqi , |
|
Hi @antmox, I'm sorry that it broke the lldb-aarch64-windows build. My bad. Your fix makes sense, though I do have the same comment as @labath had, i.e. to move that typedef outside platform-specific region. From the latest comments in that PR it seems it's already got moved. If that's the case then I think we are all good. My apologies that I didn't respond earlier - I was watching my email and the build bot (https://lab.llvm.org/buildbot/#/changes/134099), saw all green and I thought it's went well. Questions for my learning (I'm noob to all these): I realize that the build bot I quoted didn't run the windows build you mentioned (lldb-aarch64-windows). This tells me that there are multiple build bots that I need to watch in the future. Is there an exhaustive list somewhere? Email obviously failed to inform me about the build break (i.e. I searched for "lldb-aarch64-windows" in my email and nothing except your message). Thanks, |
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 caller survives, and all previous callers are forgotten, which might be a surprise to them. Worse, if this is called in a race condition, which callback survives is less predictable, which may case confusing behavior elsewhere.This PR
Allows multiple destroy callbacks to be registered and all called when the debugger is destroyed.
EDIT: Adds two new APIs:
AddDestroyCallback()andClearDestroyCallback().SetDestroyCallback()will first clear then add the given callback. Tests are added for the new APIs.Tests
(out-dated, see comments below) Semantic change to
SetDestroyCallback()Currently, the method overwrites the old callback with the new one. With this PR, it will NOT overwrite. Instead, it will hold on to both. Both callbacks get called during destroy.Risk: Although the documentation ofSetDestroyCallback()(see C++ and python) doesn't really specify the behavior, there is a risk: if existing call sites rely on the "overwrite" behavior, they will be surprised because now the old callback will get called. But as the above said, the current behavior of "overwrite" itself might be unintended, so I don't anticipate users to rely on this behavior. In short, this risk might be less of a problem if we correct it sooner rather than later (which is what this PR is trying to do).(out-dated, see comments below) Implementation
The implementation holds astd::vector<std::pair<callback, baton>>. WhenSetDestroyCallback()is called, callbacks and batons are appended to thestd::vector. When destroy event happen, the(callback, baton)pairs are invoked FIFO. Finally, thestd::vectoris cleared.(out-dated, see comments below) Alternatives considered
Instead of changingSetDestroyCallback(), a new methodAddDestroyCallback()can be added, which use the samestd::vector<std::pair<>>implementation. Together withClearDestroyCallback()(see below), they will replace and deprecateSetDestroyCallback(). Meanwhile, in order to be backward compatible,SetDestroyCallback()need to be updated to clear thestd::vectorand then add the new callback. Pros: The end state is semantically more correct. Cons: More steps to take; potentially maintaining an "incorrect" behavior (of "overwrite").A new methodClearDestroyCallback()can be added. Might be unnecessary at this point, because workflows which need to set then clear callbacks may exist but shouldn't be too common at least for now. Such method can be added later when needed.Thestd::vectormay bring slight performance drawback if its implementation doesn't handle small size efficiently. However, even if that's the case, this path should be very cold (only used during init and destroy). Such performance drawback should be negligible.A different implementation was also considered. Instead of usingstd::vector, the currentm_destroy_callbackfield can be kept unchanged. WhenSetDestroyCallback()is called, a lambda function can be stored intom_destroy_callback. This lambda function will first call the old callback, then the new one. This way,std::vectoris avoided. However, this implementation is more complex, thus less readable, with not much perf to gain.