[1/3] DAP: Allow for deferring stop events from gdb thread
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
The existing `send_event_later()` method allows commands processed on
the DAP thread to queue an event for execution until after the response
has been sent to the client.
We now introduce a corresponding method for use by the gdb thread. This
method `send_event_maybe_later()` will queue the event just like
`send_event_later()`, but only if it has been configured to do so by a
new @request option `defer_stop_events`. As the name implies the
functionality is currently only used for handling stop events.
---
gdb/python/lib/gdb/dap/events.py | 4 +--
gdb/python/lib/gdb/dap/server.py | 56 +++++++++++++++++++++++++++++---
2 files changed, 53 insertions(+), 7 deletions(-)
Comments
>>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:
Johan> The existing `send_event_later()` method allows commands processed on
Johan> the DAP thread to queue an event for execution until after the response
Johan> has been sent to the client.
Johan> We now introduce a corresponding method for use by the gdb thread. This
Johan> method `send_event_maybe_later()` will queue the event just like
Johan> `send_event_later()`, but only if it has been configured to do so by a
Johan> new @request option `defer_stop_events`. As the name implies the
Johan> functionality is currently only used for handling stop events.
Thanks for the patch.
I have a couple questions.
Johan> + self.delayed_events_lock = threading.Lock()
Is a second lock really needed?
I didn't understand why the canceller lock isn't sufficient.
Johan> + @in_dap_thread
Johan> + def _handle_command_finish(self, params):
Johan> + req = params["seq"]
Johan> + self.canceller.done(req)
I guess this is needed to defer the 'done' call until after the result
is sent?
If so that seems fine. I may refactor this later but I don't want to
add stuff to your to-do list...
Johan> + @in_gdb_thread
Johan> + def send_event_maybe_later(self, event, body=None):
Johan> + """Send a DAP event back to the client, but if a request is in-flight
Johan> + within the dap thread and that request is configured to delay the event,
Johan> + wait until the response has been sent until the event is sent back to
Johan> + the client."""
Johan> + send_immediately = True
Johan> + with self.canceller.lock:
Johan> + if self.canceller.in_flight_dap_thread:
Johan> + with self.delayed_events_lock:
Johan> + if self.defer_stop_events:
Johan> + self.delayed_events.append((event, body))
Johan> + send_immediately = False
Johan> + if send_immediately:
Johan> + self.send_event(event, body)
This could be simpler if the defer case just does 'return'
Tom
On Wed, May 15, 2024 at 9:47 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:
>
> Johan> The existing `send_event_later()` method allows commands processed on
> Johan> the DAP thread to queue an event for execution until after the response
> Johan> has been sent to the client.
>
> Johan> We now introduce a corresponding method for use by the gdb thread. This
> Johan> method `send_event_maybe_later()` will queue the event just like
> Johan> `send_event_later()`, but only if it has been configured to do so by a
> Johan> new @request option `defer_stop_events`. As the name implies the
> Johan> functionality is currently only used for handling stop events.
>
> Thanks for the patch.
>
> I have a couple questions.
>
> Johan> + self.delayed_events_lock = threading.Lock()
>
> Is a second lock really needed?
> I didn't understand why the canceller lock isn't sufficient.
The second lock is not strictly needed. The reason I introduced it was
just to keep it
close to the data it protects within the server object and maybe
making it more likely
that you remember to actually take the lock when needed. Using the
canceller lock
did not seem that obvious since it is located within the canceller
object and has a
name that doesn't really suggest it has anything to do with the
events. But on the
other hand the double locking in send_event_maybe_later may seem a bit
over complex so I guess it can be done either way. Just let me know your
preference and I'll adapt.
>
> Johan> + @in_dap_thread
> Johan> + def _handle_command_finish(self, params):
> Johan> + req = params["seq"]
> Johan> + self.canceller.done(req)
>
> I guess this is needed to defer the 'done' call until after the result
> is sent?
Exactly
>
> If so that seems fine. I may refactor this later but I don't want to
> add stuff to your to-do list...
>
> Johan> + @in_gdb_thread
> Johan> + def send_event_maybe_later(self, event, body=None):
> Johan> + """Send a DAP event back to the client, but if a request is in-flight
> Johan> + within the dap thread and that request is configured to delay the event,
> Johan> + wait until the response has been sent until the event is sent back to
> Johan> + the client."""
> Johan> + send_immediately = True
> Johan> + with self.canceller.lock:
> Johan> + if self.canceller.in_flight_dap_thread:
> Johan> + with self.delayed_events_lock:
> Johan> + if self.defer_stop_events:
> Johan> + self.delayed_events.append((event, body))
> Johan> + send_immediately = False
> Johan> + if send_immediately:
> Johan> + self.send_event(event, body)
>
> This could be simpler if the defer case just does 'return'
>
Indeed yes. This probably went through some changes without being
fully cleaned up. Will fix.
> Tom
@@ -17,7 +17,7 @@ import gdb
from .modules import is_module, make_module
from .scopes import set_finish_value
-from .server import send_event
+from .server import send_event, send_event_maybe_later
from .startup import exec_and_log, in_gdb_thread, log
# True when the inferior is thought to be running, False otherwise.
@@ -241,7 +241,7 @@ def _on_stop(event):
global stop_reason_map
obj["reason"] = stop_reason_map[event.details["reason"]]
_expected_pause = False
- send_event("stopped", obj)
+ send_event_maybe_later("stopped", obj)
# This keeps a bit of state between the start of an inferior call and
@@ -124,6 +124,8 @@ class Server:
self.in_stream = in_stream
self.out_stream = out_stream
self.child_stream = child_stream
+ self.delayed_events_lock = threading.Lock()
+ self.defer_stop_events = False
self.delayed_events = []
# This queue accepts JSON objects that are then sent to the
# DAP client. Writing is done in a separate thread to avoid
@@ -177,9 +179,13 @@ class Server:
log_stack()
result["success"] = False
result["message"] = str(e)
- self.canceller.done(req)
return result
+ @in_dap_thread
+ def _handle_command_finish(self, params):
+ req = params["seq"]
+ self.canceller.done(req)
+
# Read inferior output and sends OutputEvents to the client. It
# is run in its own thread.
def _read_inferior_output(self):
@@ -239,8 +245,12 @@ class Server:
break
result = self._handle_command(cmd)
self._send_json(result)
- events = self.delayed_events
- self.delayed_events = []
+ self._handle_command_finish(cmd)
+ events = None
+ with self.delayed_events_lock:
+ events = self.delayed_events
+ self.delayed_events = []
+ self.defer_stop_events = False
for event, body in events:
self.send_event(event, body)
# Got the terminate request. This is handled by the
@@ -254,7 +264,24 @@ class Server:
def send_event_later(self, event, body=None):
"""Send a DAP event back to the client, but only after the
current request has completed."""
- self.delayed_events.append((event, body))
+ with self.delayed_events_lock:
+ self.delayed_events.append((event, body))
+
+ @in_gdb_thread
+ def send_event_maybe_later(self, event, body=None):
+ """Send a DAP event back to the client, but if a request is in-flight
+ within the dap thread and that request is configured to delay the event,
+ wait until the response has been sent until the event is sent back to
+ the client."""
+ send_immediately = True
+ with self.canceller.lock:
+ if self.canceller.in_flight_dap_thread:
+ with self.delayed_events_lock:
+ if self.defer_stop_events:
+ self.delayed_events.append((event, body))
+ send_immediately = False
+ if send_immediately:
+ self.send_event(event, body)
# Note that this does not need to be run in any particular thread,
# because it just creates an object and writes it to a thread-safe
@@ -287,6 +314,15 @@ def send_event(event, body=None):
_server.send_event(event, body)
+def send_event_maybe_later(event, body=None):
+ """Send a DAP event back to the client, but if a request is in-flight
+ within the dap thread and that request is configured to delay the event,
+ wait until the response has been sent until the event is sent back to
+ the client."""
+ global _server
+ _server.send_event_maybe_later(event, body)
+
+
# A helper decorator that checks whether the inferior is running.
def _check_not_running(func):
@functools.wraps(func)
@@ -307,7 +343,8 @@ def request(
*,
response: bool = True,
on_dap_thread: bool = False,
- expect_stopped: bool = True
+ expect_stopped: bool = True,
+ defer_stop_events: bool = False
):
"""A decorator for DAP requests.
@@ -328,6 +365,10 @@ def request(
fail with the 'notStopped' reason if it is processed while the
inferior is running. When EXPECT_STOPPED is False, the request
will proceed regardless of the inferior's state.
+
+ If DEFER_STOP_EVENTS is True, then make sure any stop events sent
+ during the request processing are not sent to the client until the
+ response has been sent.
"""
# Validate the parameters.
@@ -355,6 +396,11 @@ def request(
func = in_gdb_thread(func)
if response:
+ if defer_stop_events:
+ global _server
+ if _server is not None:
+ with _server.delayed_events_lock:
+ _server.defer_stop_events = True
def sync_call(**args):
return send_gdb_with_response(lambda: func(**args))