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

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

[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

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

[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..3b81fa43e39 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))
+        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 2/3] DAP: Allow gdb exception in exec_and_log to propagate

[Tom Tromey]

>>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:

Johan> This allows a request to specify that any gdb exception raised in
Johan> exec_and_log within the gdb thread to be propagated back to the DAP
Johan> thread (using the Canceller object as the orchestrator).

Thank you for the patch.

Johan> +    except gdb.error as e:
Johan> +        if propagate_exception:
Johan> +            raise DAPException(str(e))

This should probably be 'raise DAPException(str(e)) from e'

Tom

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

[Tom Tromey]

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

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

[Tom Tromey]

>>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:

Johan> Second version of patchset rewritten so that the underlying "finish"
Johan> command is run as a background task so that any immediate error may be
Johan> captured immediately and reported back to the client in the response.

Thanks once again.  This looks close to how it should work.
What's the state of your copyright assignment?

Tom

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

[Johan Sternerup]

On Wed, May 15, 2024 at 9:47 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:
>
> Johan> Second version of patchset rewritten so that the underlying "finish"
> Johan> command is run as a background task so that any immediate error may be
> Johan> captured immediately and reported back to the client in the response.
>
> Thanks once again.  This looks close to how it should work.
> What's the state of your copyright assignment?

You just made me aware of the copyright assignment and I've started
the process by emailing assign@fsf.org.

>
> Tom

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

[Johan Sternerup]

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

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

[Johan Sternerup]

On Wed, May 15, 2024 at 9:35 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:
>
> Johan> This allows a request to specify that any gdb exception raised in
> Johan> exec_and_log within the gdb thread to be propagated back to the DAP
> Johan> thread (using the Canceller object as the orchestrator).
>
> Thank you for the patch.
>
> Johan> +    except gdb.error as e:
> Johan> +        if propagate_exception:
> Johan> +            raise DAPException(str(e))
>
> This should probably be 'raise DAPException(str(e)) from e'
>

Will fix.

> 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