* [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame @ 2024-06-01 16:16 Johan Sternerup 2024-06-01 16:16 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Johan Sternerup @ 2024-06-01 16:16 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup Add support for stepping commands to be run as background tasks so that immediate errors can be caught and reported back to the client. Specifically, make use of this for "stepOut" so that in the case of trying to send a "stepOut" request in the outermost frame we can now report the error back to the client. Then the client knows that it should still expect the inferior to be stopped and should not wait for a stoppedEvent. Johan Sternerup (3): DAP: Allow for deferring stop events from gdb thread DAP: Allow gdb exception in exec_and_log to propagate DAP: Handle "stepOut" request in outermost frame gdb/python/lib/gdb/dap/events.py | 8 ++--- gdb/python/lib/gdb/dap/next.py | 4 +-- gdb/python/lib/gdb/dap/server.py | 54 +++++++++++++++++++++++++++--- gdb/python/lib/gdb/dap/startup.py | 9 +++-- gdb/testsuite/gdb.dap/step-out.exp | 11 ++++++ 5 files changed, 72 insertions(+), 14 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread 2024-06-01 16:16 [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup @ 2024-06-01 16:16 ` Johan Sternerup 2024-06-01 16:16 ` [PATCH 2/3] DAP: Allow gdb exception in exec_and_log to propagate Johan Sternerup ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Johan Sternerup @ 2024-06-01 16:16 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup 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 | 54 +++++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py index 276d3145b9a..80a259a1422 100644 --- a/gdb/python/lib/gdb/dap/events.py +++ b/gdb/python/lib/gdb/dap/events.py @@ -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 diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py index 7eb87177710..8c6d90822f3 100644 --- a/gdb/python/lib/gdb/dap/server.py +++ b/gdb/python/lib/gdb/dap/server.py @@ -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,22 @@ 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.""" + 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)) + return + 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 +312,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 +341,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 +363,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 +394,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)) -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] DAP: Allow gdb exception in exec_and_log to propagate 2024-06-01 16:16 [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup 2024-06-01 16:16 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup @ 2024-06-01 16:16 ` Johan Sternerup 2024-06-01 16:16 ` [PATCH 3/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup 2024-06-04 14:01 ` [PATCH v3 0/3] " Tom Tromey 3 siblings, 0 replies; 11+ messages in thread From: Johan Sternerup @ 2024-06-01 16:16 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup This allows a request to specify that any gdb exception raised in exec_and_log within the gdb thread to be propagated back to the DAP thread (using the Canceller object as the orchestrator). --- gdb/python/lib/gdb/dap/events.py | 4 ++-- gdb/python/lib/gdb/dap/startup.py | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py index 80a259a1422..2e6fe989e22 100644 --- a/gdb/python/lib/gdb/dap/events.py +++ b/gdb/python/lib/gdb/dap/events.py @@ -161,7 +161,7 @@ _expected_pause = False @in_gdb_thread -def exec_and_expect_stop(cmd, expected_pause=False): +def exec_and_expect_stop(cmd, expected_pause=False, propagate_exception=False): """A wrapper for exec_and_log that sets the continue-suppression flag. When EXPECTED_PAUSE is True, a stop that looks like a pause (e.g., @@ -174,7 +174,7 @@ def exec_and_expect_stop(cmd, expected_pause=False): # continuing. _suppress_cont = not expected_pause # FIXME if the call fails should we clear _suppress_cont? - exec_and_log(cmd) + exec_and_log(cmd, propagate_exception) # Map from gdb stop reasons to DAP stop reasons. Some of these can't diff --git a/gdb/python/lib/gdb/dap/startup.py b/gdb/python/lib/gdb/dap/startup.py index 58591c00b97..3952447e4d7 100644 --- a/gdb/python/lib/gdb/dap/startup.py +++ b/gdb/python/lib/gdb/dap/startup.py @@ -204,7 +204,7 @@ def log_stack(level=LogLevel.DEFAULT): @in_gdb_thread -def exec_and_log(cmd): +def exec_and_log(cmd, propagate_exception=False): """Execute the gdb command CMD. If logging is enabled, log the command and its output.""" log("+++ " + cmd) @@ -212,5 +212,8 @@ def exec_and_log(cmd): output = gdb.execute(cmd, from_tty=True, to_string=True) if output != "": log(">>> " + output) - except gdb.error: - log_stack() + except gdb.error as e: + if propagate_exception: + raise DAPException(str(e)) from e + else: + log_stack() -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] DAP: Handle "stepOut" request in outermost frame 2024-06-01 16:16 [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup 2024-06-01 16:16 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup 2024-06-01 16:16 ` [PATCH 2/3] DAP: Allow gdb exception in exec_and_log to propagate Johan Sternerup @ 2024-06-01 16:16 ` Johan Sternerup 2024-06-04 14:01 ` [PATCH v3 0/3] " Tom Tromey 3 siblings, 0 replies; 11+ messages in thread From: Johan Sternerup @ 2024-06-01 16:16 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup Previously a "stepOut" request when in the outermost frame would result in a sucessful response even though gdb internally would reject the associated "finish" request, which means no stoppedEvent would ever be sent back to the client. Thus the client would believe the inferior was still running and as a consequence reject subsequent "next" and "stepIn" requests from the user. The solution is to execute the underlying finish command as a background command, i.e. `finish &`. If we're in the outermost frame an exception will be raised immediately, which we can now capture and report back to the client as success=False so then the absence of a `stopped` event is no longer a problem. We also make use of the `defer_stop_event` option to prevent a stop event from reaching the client until the response has been sent. --- gdb/python/lib/gdb/dap/next.py | 4 ++-- gdb/testsuite/gdb.dap/step-out.exp | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py index 1dc1d6dd74d..7e06b1b79dc 100644 --- a/gdb/python/lib/gdb/dap/next.py +++ b/gdb/python/lib/gdb/dap/next.py @@ -73,10 +73,10 @@ def step_in( exec_and_expect_stop(cmd) -@request("stepOut", response=False) +@request("stepOut", defer_stop_events=True) def step_out(*, threadId: int, singleThread: bool = False, **args): _handle_thread_step(threadId, singleThread, True) - exec_and_expect_stop("finish") + exec_and_expect_stop("finish &", propagate_exception=True) # This is a server-side request because it is funny: it wants to diff --git a/gdb/testsuite/gdb.dap/step-out.exp b/gdb/testsuite/gdb.dap/step-out.exp index 757f4ebdaca..31c50bb81ec 100644 --- a/gdb/testsuite/gdb.dap/step-out.exp +++ b/gdb/testsuite/gdb.dap/step-out.exp @@ -79,4 +79,15 @@ gdb_assert {[dict get $varlist variablesReference] > 0} \ gdb_assert {[dict get $varlist name] == "(return)"} \ "variable is return value" +set response_and_events [dap_request_and_response stepOut {o threadId [i 1]}] +set response [lindex $response_and_events 0] +if {[dict get $response success] == "true"} { + fail "stepOut from outermost frame should not succeed" +} else { + pass "stepOut from outermost frame failed like it should" +} + +dap_check_request_and_response "still stopped and may request backtrace" \ + stackTrace {o threadId [i 1]} + dap_shutdown -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame 2024-06-01 16:16 [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup ` (2 preceding siblings ...) 2024-06-01 16:16 ` [PATCH 3/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup @ 2024-06-04 14:01 ` Tom Tromey 2024-06-05 13:54 ` Johan Sternerup 3 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2024-06-04 14:01 UTC (permalink / raw) To: Johan Sternerup; +Cc: gdb-patches, tom >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes: Johan> Add support for stepping commands to be run as background tasks so that Johan> immediate errors can be caught and reported back to the client. Johan> Specifically, make use of this for "stepOut" so that in the case of trying to Johan> send a "stepOut" request in the outermost frame we can now report the error back Johan> to the client. Then the client knows that it should still expect the inferior to Johan> be stopped and should not wait for a stoppedEvent. Hi. Thanks for the patches. These look good to me. Approved-By: Tom Tromey <tom@tromey.com> If you think you'll be writing more gdb patches, email me and I can get you set up with write-after-approval access, and you can push your own series. However if you would prefer, let me know and I can check these in for you. thanks, Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame 2024-06-04 14:01 ` [PATCH v3 0/3] " Tom Tromey @ 2024-06-05 13:54 ` Johan Sternerup 2024-06-05 16:01 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Johan Sternerup @ 2024-06-05 13:54 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches I have no plans for more gdb patches at this time so if you can handle the check-in of these patches it would be great. It was a pleasure to be able to contribute to a project that has been so useful for me over the years. If I bump into other issues I now know how the contribution process works. Thanks / Johan On Tue, Jun 4, 2024 at 4:01 PM Tom Tromey <tom@tromey.com> wrote: > > >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes: > > Johan> Add support for stepping commands to be run as background tasks so that > Johan> immediate errors can be caught and reported back to the client. > > Johan> Specifically, make use of this for "stepOut" so that in the case of trying to > Johan> send a "stepOut" request in the outermost frame we can now report the error back > Johan> to the client. Then the client knows that it should still expect the inferior to > Johan> be stopped and should not wait for a stoppedEvent. > > Hi. Thanks for the patches. These look good to me. > > Approved-By: Tom Tromey <tom@tromey.com> > > > If you think you'll be writing more gdb patches, email me and I can get > you set up with write-after-approval access, and you can push your own > series. However if you would prefer, let me know and I can check these > in for you. > > thanks, > Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame 2024-06-05 13:54 ` Johan Sternerup @ 2024-06-05 16:01 ` Tom Tromey 2024-06-06 17:03 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2024-06-05 16:01 UTC (permalink / raw) To: Johan Sternerup; +Cc: Tom Tromey, gdb-patches >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes: Johan> I have no plans for more gdb patches at this time so if you can Johan> handle the check-in of these patches it would be great. No problem. I'll take care of it today. Johan> It was a pleasure to be able to contribute to a project that has Johan> been so useful for me over the years. If I bump into other issues Johan> I now know how the contribution process works. Of course I hope there won't be other bugs but on the other hand, we appreciate your contribution, come back any time :) Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame 2024-06-05 16:01 ` Tom Tromey @ 2024-06-06 17:03 ` Tom Tromey 0 siblings, 0 replies; 11+ messages in thread From: Tom Tromey @ 2024-06-06 17:03 UTC (permalink / raw) To: Tom Tromey; +Cc: Johan Sternerup, gdb-patches Johan> I have no plans for more gdb patches at this time so if you can Johan> handle the check-in of these patches it would be great. Tom> No problem. I'll take care of it today. I got distracted yesterday but I finally got to it. Thanks again. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 0/3] DAP: Handle "stepOut" request in outermost frame @ 2024-04-29 19:43 Johan Sternerup 2024-04-29 19:43 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup 0 siblings, 1 reply; 11+ messages in thread From: Johan Sternerup @ 2024-04-29 19:43 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup Second version of patchset rewritten so that the underlying "finish" command is run as a background task so that any immediate error may be captured immediately and reported back to the client in the response. Johan Sternerup (3): DAP: Allow for deferring stop events from gdb thread DAP: Allow gdb exception in exec_and_log to propagate DAP: Handle "stepOut" request in outermost frame gdb/python/lib/gdb/dap/events.py | 8 ++--- gdb/python/lib/gdb/dap/next.py | 4 +-- gdb/python/lib/gdb/dap/server.py | 56 +++++++++++++++++++++++++++--- gdb/python/lib/gdb/dap/startup.py | 9 +++-- gdb/testsuite/gdb.dap/step-out.exp | 11 ++++++ 5 files changed, 74 insertions(+), 14 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread 2024-04-29 19:43 [PATCH v2 " Johan Sternerup @ 2024-04-29 19:43 ` Johan Sternerup 2024-05-15 19:47 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Johan Sternerup @ 2024-04-29 19:43 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Johan Sternerup 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(-) diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py index 276d3145b9a..80a259a1422 100644 --- a/gdb/python/lib/gdb/dap/events.py +++ b/gdb/python/lib/gdb/dap/events.py @@ -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 diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py index 7eb87177710..ea62fa84077 100644 --- a/gdb/python/lib/gdb/dap/server.py +++ b/gdb/python/lib/gdb/dap/server.py @@ -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)) -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread 2024-04-29 19:43 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup @ 2024-05-15 19:47 ` Tom Tromey 2024-05-16 19:48 ` Johan Sternerup 0 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2024-05-15 19:47 UTC (permalink / raw) To: Johan Sternerup; +Cc: gdb-patches, tom >>>>> "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread 2024-05-15 19:47 ` Tom Tromey @ 2024-05-16 19:48 ` Johan Sternerup 0 siblings, 0 replies; 11+ messages in thread From: Johan Sternerup @ 2024-05-16 19:48 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-06-06 17:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-01 16:16 [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup 2024-06-01 16:16 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup 2024-06-01 16:16 ` [PATCH 2/3] DAP: Allow gdb exception in exec_and_log to propagate Johan Sternerup 2024-06-01 16:16 ` [PATCH 3/3] DAP: Handle "stepOut" request in outermost frame Johan Sternerup 2024-06-04 14:01 ` [PATCH v3 0/3] " Tom Tromey 2024-06-05 13:54 ` Johan Sternerup 2024-06-05 16:01 ` Tom Tromey 2024-06-06 17:03 ` Tom Tromey -- strict thread matches above, loose matches on Subject: below -- 2024-04-29 19:43 [PATCH v2 " Johan Sternerup 2024-04-29 19:43 ` [PATCH 1/3] DAP: Allow for deferring stop events from gdb thread Johan Sternerup 2024-05-15 19:47 ` Tom Tromey 2024-05-16 19:48 ` Johan Sternerup
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).