Toolchain/binutils-gdb
1
0
Fork 0
forked from Imagelibrary/binutils-gdb
Code Pull Requests Activity

Fix DAP defer_stop_events implementation

Browse Source 
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>
This commit is contained in:
Tom Tromey
2025-02-13 07:24:08 -07:00
parent 568ec5b983
commit cbaa41b330
5 changed files with 64 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gdb/python/lib/gdb/dap/evaluate.py
View 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
):

4
gdb/python/lib/gdb/dap/events.py
View 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

15
gdb/python/lib/gdb/dap/next.py
View 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}

90
gdb/python/lib/gdb/dap/server.py
View 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

4
gdb/testsuite/gdb.dap/attach.exp
View 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