]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
Fix DAP defer_stop_events implementation
authorTom Tromey <tromey@adacore.com>
Thu, 13 Feb 2025 14:24:08 +0000 (07:24 -0700)
committerTom Tromey <tromey@adacore.com>
Mon, 2 Jun 2025 17:46:53 +0000 (11:46 -0600)
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 <simon.marchi@efficios.com>
gdb/python/lib/gdb/dap/evaluate.py
gdb/python/lib/gdb/dap/events.py
gdb/python/lib/gdb/dap/next.py
gdb/python/lib/gdb/dap/server.py
gdb/testsuite/gdb.dap/attach.exp

index 55538e6622995053cbcdf6289b89689c141be29b..fcbcc99acfce4d0d54fc087633407891821b6d30 100644 (file)
@@ -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
 ):
index 4dc9d65e53e12c079a39b1081bb5035b75a03fce..4dc4e95f0530ffc16fafbe1e454772fe70c558fc 100644 (file)
@@ -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
index ddf4e057ef3026374ec1f7a37355428a9d04444f..898fff1936af19ca451bf878746be3b3ae4145cd 100644 (file)
@@ -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}
index c4fa78183468851ba309765505277c6d5f4ef083..7dab582d0c37452e072fddf2e6d58e4754769e06 100644 (file)
@@ -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
index 37e867c7d2b93351be4d4f5a1dc6f02ccf5e6fc9..5e1f6344ae3824275507e5dd1f5536fdf4b9467f 100644 (file)
@@ -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