[PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[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

[PATCH 1/3] DAP: Allow for deferring stop events from gdb thread

[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

[PATCH 2/3] DAP: Allow gdb exception in exec_and_log to propagate

[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

[PATCH 3/3] DAP: Handle "stepOut" request in outermost frame

[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

Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[Tom Tromey]

>>>>> "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

Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[Johan Sternerup]

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

Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[Tom Tromey]

>>>>> "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

Re: [PATCH v3 0/3] DAP: Handle "stepOut" request in outermost frame

[Tom Tromey]

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

[PATCH 3/3] DAP: Handle "stepOut" request in outermost frame

[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