From cbaa41b3302e15ead6af1d21de9140c02d6c7ab5 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Thu, 13 Feb 2025 07:24:08 -0700 Subject: [PATCH] Fix DAP defer_stop_events implementation DAP requests have a "defer_stop_events" option that is intended to defer the emission of any "stopped" event until after the current request completes. This was needed to handle async continues like "finish &". However, I noticed that sometimes DAP tests can fail, because a stop event does arrive before the response to the "stepOut" request. I've only noticed this when the machine is fairly loaded -- for instance when I'm regression-testing a series, it may occur in some of the tests mid-series. I believe the problem is that the implementation in the "request" function is incorrect -- the flag is set when "request" is invoked, but instead it must be deferred until the request itself is run. That is, the setting must be captured in one of the wrapper functions. Following up on this, Simon pointed out that introducing a delay before sending a request's response will cause test case failures. That is, there's a race here that is normally hidden. Investigation showed that that deferred requests can't force event deferral. This patch implements this; but more testing showed many more race failures. Some of these are due to how the test suite is written. Anyway, in the end I took the radical approach of deferring all events by default. Most DAP requests are asynchronous by nature, so this seemed ok. The only case I found that really required this is pause.exp, where the test (rightly) expects to see a 'continued' event while performing an inferior function call. I went through all events and all requests and tried to convince myself that this patch will cause acceptable behavior in every case. However, it's hard to be completely sure about this approach. Maybe there are cases that do still need an event before the response, but we just don't have tests for them. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32685 Acked-By: Simon Marchi --- gdb/python/lib/gdb/dap/evaluate.py | 6 +- gdb/python/lib/gdb/dap/events.py | 4 +- gdb/python/lib/gdb/dap/next.py | 15 ++--- gdb/python/lib/gdb/dap/server.py | 90 +++++++++++++++++------------- gdb/testsuite/gdb.dap/attach.exp | 4 +- 5 files changed, 64 insertions(+), 55 deletions(-) diff --git a/gdb/python/lib/gdb/dap/evaluate.py b/gdb/python/lib/gdb/dap/evaluate.py index 55538e66229..fcbcc99acfc 100644 --- a/gdb/python/lib/gdb/dap/evaluate.py +++ b/gdb/python/lib/gdb/dap/evaluate.py @@ -69,7 +69,7 @@ def _repl(command, frame_id): } -@request("evaluate") +@request("evaluate", defer_events=False) @capability("supportsEvaluateForHovers") @capability("supportsValueFormattingOptions") def eval_request( @@ -110,7 +110,7 @@ def variables( @capability("supportsSetExpression") -@request("setExpression") +@request("setExpression", defer_events=False) def set_expression( *, expression: str, value: str, frameId: Optional[int] = None, format=None, **args ): @@ -126,7 +126,7 @@ def set_expression( @capability("supportsSetVariable") -@request("setVariable") +@request("setVariable", defer_events=False) def set_variable( *, variablesReference: int, name: str, value: str, format=None, **args ): diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py index 4dc9d65e53e..4dc4e95f053 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, send_event_maybe_later +from .server import send_event from .startup import exec_and_log, in_gdb_thread, log # True when the inferior is thought to be running, False otherwise. @@ -240,7 +240,7 @@ def _on_stop(event): else: obj["reason"] = stop_reason_map[event.details["reason"]] _expected_pause = False - send_event_maybe_later("stopped", obj) + send_event("stopped", obj) # This keeps a bit of state between the start of an inferior call and diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py index ddf4e057ef3..898fff1936a 100644 --- a/gdb/python/lib/gdb/dap/next.py +++ b/gdb/python/lib/gdb/dap/next.py @@ -16,7 +16,7 @@ import gdb from .events import exec_and_expect_stop -from .server import capability, request, send_gdb, send_gdb_with_response +from .server import capability, request from .startup import in_gdb_thread from .state import set_thread @@ -73,19 +73,14 @@ def step_in( exec_and_expect_stop(cmd) -@request("stepOut", defer_stop_events=True) +@request("stepOut") def step_out(*, threadId: int, singleThread: bool = False, **args): _handle_thread_step(threadId, singleThread, True) exec_and_expect_stop("finish &", propagate_exception=True) -# This is a server-side request because it is funny: it wants to -# 'continue' but also return a result, which precludes using -# response=False. Using 'continue &' would mostly work ok, but this -# yields races when a stop occurs before the response is sent back to -# the client. -@request("continue", on_dap_thread=True) +@request("continue") def continue_request(*, threadId: int, singleThread: bool = False, **args): - locked = send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread)) - send_gdb(lambda: exec_and_expect_stop("continue")) + locked = _handle_thread_step(threadId, singleThread) + exec_and_expect_stop("continue &") return {"allThreadsContinued": not locked} diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py index c4fa7818346..7dab582d0c3 100644 --- a/gdb/python/lib/gdb/dap/server.py +++ b/gdb/python/lib/gdb/dap/server.py @@ -73,6 +73,13 @@ class DeferredRequest: self._req = req self._result = result + @in_dap_thread + def defer_events(self): + """Return True if events should be deferred during execution. + + This may be overridden by subclasses.""" + return True + @in_dap_thread def invoke(self): """Implement the deferred request. @@ -95,7 +102,10 @@ class DeferredRequest: """ with _server.canceller.current_request(self._req): + if self.defer_events(): + _server.set_defer_events() _server.invoke_request(self._req, self._result, self.invoke) + _server.emit_pending_events() # A subclass of Exception that is used solely for reporting that a @@ -230,7 +240,7 @@ class Server: self._out_stream = out_stream self._child_stream = child_stream self._delayed_fns_lock = threading.Lock() - self.defer_stop_events = False + self._defer_events = False self._delayed_fns = [] # This queue accepts JSON objects that are then sent to the # DAP client. Writing is done in a separate thread to avoid @@ -316,7 +326,7 @@ class Server: def _read_inferior_output(self): while True: line = self._child_stream.readline() - self.send_event( + self.send_event_maybe_later( "output", { "category": "stdout", @@ -355,6 +365,17 @@ class Server: # When we hit EOF, signal it with None. self._read_queue.put(None) + @in_dap_thread + def emit_pending_events(self): + """Emit any pending events.""" + fns = None + with self._delayed_fns_lock: + fns = self._delayed_fns + self._delayed_fns = [] + self._defer_events = False + for fn in fns: + fn() + @in_dap_thread def main_loop(self): """The main loop of the DAP server.""" @@ -371,13 +392,7 @@ class Server: req = cmd["seq"] with self.canceller.current_request(req): self._handle_command(cmd) - fns = None - with self._delayed_fns_lock: - fns = self._delayed_fns - self._delayed_fns = [] - self.defer_stop_events = False - for fn in fns: - fn() + self.emit_pending_events() # Got the terminate request. This is handled by the # JSON-writing thread, so that we can ensure that all # responses are flushed to the client before exiting. @@ -386,13 +401,13 @@ class Server: send_gdb("quit") @in_dap_thread - def send_event_later(self, event, body=None): - """Send a DAP event back to the client, but only after the - current request has completed.""" + def set_defer_events(self): + """Defer any events until the current request has completed.""" with self._delayed_fns_lock: - self._delayed_fns.append(lambda: self.send_event(event, body)) + self._defer_events = True - @in_gdb_thread + # Note that this does not need to be run in any particular thread, + # because it uses locks for thread-safety. 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, @@ -401,10 +416,10 @@ class Server: with self.canceller.lock: if self.canceller.in_flight_dap_thread: with self._delayed_fns_lock: - if self.defer_stop_events: - self._delayed_fns.append(lambda: self.send_event(event, body)) + if self._defer_events: + self._delayed_fns.append(lambda: self._send_event(event, body)) return - self.send_event(event, body) + self._send_event(event, body) @in_dap_thread def call_function_later(self, fn): @@ -415,7 +430,7 @@ class Server: # 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 # queue. - def send_event(self, event, body=None): + def _send_event(self, event, body=None): """Send an event to the DAP client. EVENT is the name of the event, a string. BODY is the body of the event, an arbitrary object.""" @@ -439,14 +454,6 @@ def send_event(event, body=None): """Send an event to the DAP client. EVENT is the name of the event, a string. BODY is the body of the event, an arbitrary object.""" - _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.""" _server.send_event_maybe_later(event, body) @@ -476,7 +483,7 @@ def request( response: bool = True, on_dap_thread: bool = False, expect_stopped: bool = True, - defer_stop_events: bool = False + defer_events: bool = True ): """A decorator for DAP requests. @@ -498,9 +505,9 @@ def request( 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. + If DEFER_EVENTS is True, then make sure any events sent during the + request processing are not sent to the client until the response + has been sent. """ # Validate the parameters. @@ -523,26 +530,33 @@ def request( # Verify that the function is run on the correct thread. if on_dap_thread: - cmd = in_dap_thread(func) + check_cmd = in_dap_thread(func) else: func = in_gdb_thread(func) if response: - if defer_stop_events: - 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)) - cmd = sync_call + check_cmd = sync_call else: def non_sync_call(**args): return send_gdb(lambda: func(**args)) - cmd = non_sync_call + check_cmd = non_sync_call + + if defer_events: + + def deferring(**args): + _server.set_defer_events() + return check_cmd(**args) + + cmd = deferring + + else: + cmd = check_cmd # If needed, check that the inferior is not running. This # wrapping is done last, so the check is done first, before @@ -582,7 +596,7 @@ def client_bool_capability(name, default=False): @request("initialize", on_dap_thread=True) def initialize(**args): _server.config = args - _server.send_event_later("initialized") + _server.send_event_maybe_later("initialized") global _lines_start_at_1 _lines_start_at_1 = client_bool_capability("linesStartAt1", True) global _columns_start_at_1 diff --git a/gdb/testsuite/gdb.dap/attach.exp b/gdb/testsuite/gdb.dap/attach.exp index 37e867c7d2b..5e1f6344ae3 100644 --- a/gdb/testsuite/gdb.dap/attach.exp +++ b/gdb/testsuite/gdb.dap/attach.exp @@ -33,11 +33,11 @@ set attach_id [dap_attach $testpid $binfile] dap_check_request_and_response "configurationDone" configurationDone +dap_check_response "attach response" attach $attach_id + dap_wait_for_event_and_check "stopped" stopped \ "body reason" attach -dap_check_response "attach response" attach $attach_id - dap_shutdown true kill_wait_spawned_process $test_spawn_id -- 2.39.5