[PATCH] Handle DAP "stepOut" request in outermost frame

[PATCH] Handle DAP "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 here is to make the same check that is made in
'infcmd.c/finish_command()' to make sure we're not in the outermost
frame, but now we do it _before_ actually running the "finish" command
so that we can send an error response immediately.
---
 gdb/python/lib/gdb/dap/next.py     | 10 ++++++----
 gdb/testsuite/gdb.dap/step-out.exp | 11 +++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py
index 1dc1d6dd74d..05a9946b752 100644
--- a/gdb/python/lib/gdb/dap/next.py
+++ b/gdb/python/lib/gdb/dap/next.py
@@ -17,7 +17,7 @@ import gdb
 
 from .events import exec_and_expect_stop
 from .server import capability, request, send_gdb, send_gdb_with_response
-from .startup import in_gdb_thread
+from .startup import DAPException, in_gdb_thread
 from .state import set_thread
 
 
@@ -73,10 +73,12 @@ def step_in(
     exec_and_expect_stop(cmd)
 
 
-@request("stepOut", response=False)
+@request("stepOut", on_dap_thread=True)
 def step_out(*, threadId: int, singleThread: bool = False, **args):
-    _handle_thread_step(threadId, singleThread, True)
-    exec_and_expect_stop("finish")
+    if not gdb.selected_frame().older():
+        raise DAPException("\"finish\" not meaningful in the outermost frame.")
+    send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread))
+    send_gdb(lambda: exec_and_expect_stop("finish"))
 
 
 # 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..207eb73e191 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] Handle DAP "stepOut" request in outermost frame

[Tom Tromey]

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

Johan> Previously a "stepOut" request when in the outermost frame would result
Johan> in a sucessful response even though gdb internally would reject the
Johan> associated "finish" request, which means no stoppedEvent would ever be
Johan> sent back to the client. Thus the client would believe the inferior was
Johan> still running and as a consequence reject subsequent "next" and "stepIn"
Johan> requests from the user.

Thanks for the patch.

Johan> The solution here is to make the same check that is made in
Johan> 'infcmd.c/finish_command()' to make sure we're not in the outermost
Johan> frame, but now we do it _before_ actually running the "finish" command
Johan> so that we can send an error response immediately.

Johan> -@request("stepOut", response=False)

... I guess the reason this doesn't "just work" is that response=False
means the exception propagation doesn't really work.

Johan> +@request("stepOut", on_dap_thread=True)
Johan>  def step_out(*, threadId: int, singleThread: bool = False, **args):
Johan> -    _handle_thread_step(threadId, singleThread, True)
Johan> -    exec_and_expect_stop("finish")
Johan> +    if not gdb.selected_frame().older():
Johan> +        raise DAPException("\"finish\" not meaningful in the outermost frame.")

gdb.* APIs can only be used on the gdb thread, so this placement isn't
correct.

Johan> +    send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread))

Maybe one way forward would be a new helper function to be run on the
gdb thread, that incorporates the new check.

Johan> +if {[dict get $response success] == "true"} {
Johan> +	fail "stepOut from outermost frame should not succeed"
Johan> +} else {
Johan> +    pass "stepOut from outermost frame failed like it should"
Johan> +}
Johan> +
Johan> +dap_check_request_and_response "still stopped and may request backtrace" \
Johan> +	stackTrace {o threadId [i 1]}

Indentation here & before the 'fail' looks wrong.

Tom

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

[Tom Tromey]

Tom> Maybe one way forward would be a new helper function to be run on the
Tom> gdb thread, that incorporates the new check.

Another way might be to revisit this idea, documented in a comment in
next.py:

# 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.

That is, solve the stop event race by deferring such events, and then
have all stepping commands add "&" for background execution.  Then they
could all be ordinary requests (on the gdb thread with response), and
any exceptions thrown early by the commands would affect the response.

Tom

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

[Johan Sternerup]

On Thu, Apr 25, 2024 at 8:05 PM Tom Tromey <tom@tromey.com> wrote:
>
> Tom> Maybe one way forward would be a new helper function to be run on the
> Tom> gdb thread, that incorporates the new check.
>
> Another way might be to revisit this idea, documented in a comment in
> next.py:
>
> # 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.
>
> That is, solve the stop event race by deferring such events, and then
> have all stepping commands add "&" for background execution.  Then they
> could all be ordinary requests (on the gdb thread with response), and
> any exceptions thrown early by the commands would affect the response.
>
> Tom

Thanks for your swift reply and comments.

I think your suggestion to use background execution makes a lot of sense.
I'm not sure I've understood the details though. For all the stepping commands
we would have a request with response=True and in_dap_thread=False, i.e. we
would pass our stepping command with a '&' to the gdb thread while
waiting for the
response in the dap thread. So then the dap thread would be blocked in
send_gdb_with_response().
Eventually the gdb thread would process the request and I guess
immediately return
any error or success. Only after the step request has been made can a
stop event be
triggered and since the gdb thread is busy handling the request the
_on_stop shouldn't be
allowed to run right? Then the stopped event will also always reach
the dap thread after
the dap thread received the response. Are there maybe several gdb
threads or are gdb
events/commands not processed in an event loop? Maybe the background command
is executed in a real asynchronous way so that I also need a two-step
procedure in the
gdb thread, one for the request and another one for the answer. Then I
could understand
the race.
As you can understand I'm not (yet) that familiar with the code base
so need some further
guidance to pull this off.

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

[Tom Tromey]

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

Johan> I think your suggestion to use background execution makes a lot
Johan> of sense.  I'm not sure I've understood the details though. For
Johan> all the stepping commands we would have a request with
Johan> response=True and in_dap_thread=False, i.e. we would pass our
Johan> stepping command with a '&' to the gdb thread while waiting for
Johan> the response in the dap thread.

Yes.

Johan> Eventually the gdb thread would process the request and I guess
Johan> immediately return any error or success. Only after the step
Johan> request has been made can a stop event be triggered and since the
Johan> gdb thread is busy handling the request the _on_stop shouldn't be
Johan> allowed to run right?
...
Johan> Are there maybe several gdb threads or are gdb events/commands
Johan> not processed in an event loop?

The tricky case is a classic race: the gdb thread continues the
inferior, which immediately stops, causing gdb to emit a stop event,
which then causes the DAP event listener to generate a DAP event -- all
before the DAP function manages to return.

So, somewhere we'd need a bit of code to ensure that stop events are
delayed, when necessary.

This might be as simple as always calling send_event_later, and then
making send_event_later a little bit smarter so that it checks to see if
a request is "in flight".  That information might already be available
due to the support for cancellation, though I'd have to re-read the code
to check.

The event-generating code is in events.py.  It's a bit messy, I'm
afraid.

Tom

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

[Johan Sternerup]

On Fri, Apr 26, 2024 at 10:28 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Johan" == Johan Sternerup <johan.sternerup@gmail.com> writes:
>
> Johan> I think your suggestion to use background execution makes a lot
> Johan> of sense.  I'm not sure I've understood the details though. For
> Johan> all the stepping commands we would have a request with
> Johan> response=True and in_dap_thread=False, i.e. we would pass our
> Johan> stepping command with a '&' to the gdb thread while waiting for
> Johan> the response in the dap thread.
>
> Yes.
>
> Johan> Eventually the gdb thread would process the request and I guess
> Johan> immediately return any error or success. Only after the step
> Johan> request has been made can a stop event be triggered and since the
> Johan> gdb thread is busy handling the request the _on_stop shouldn't be
> Johan> allowed to run right?
> ...
> Johan> Are there maybe several gdb threads or are gdb events/commands
> Johan> not processed in an event loop?
>
> The tricky case is a classic race: the gdb thread continues the
> inferior, which immediately stops, causing gdb to emit a stop event,
> which then causes the DAP event listener to generate a DAP event -- all
> before the DAP function manages to return.
>

I think I get it now. The _on_stop() function might actually be called
within the
call chain from gdb.execute() when executing our request in the gdb thread.

> So, somewhere we'd need a bit of code to ensure that stop events are
> delayed, when necessary.
>
> This might be as simple as always calling send_event_later, and then
> making send_event_later a little bit smarter so that it checks to see if
> a request is "in flight".  That information might already be available
> due to the support for cancellation, though I'd have to re-read the code
> to check.
>

It seems to me that by using the cancelation mechanism we're almost there.
If I could utilize the locking provided by the CancellationHandler and the
in_flight_dap_thread bool then I could use that in send_event_later() to check
whether we should send the event immediately or just add it to delayed_events.
For this to work I would need to postpone clearing in_flight_dap_thread until
the result json has been passed to the write_queue. Basically that would mean
removing self.canceller.done() from _handle_command() and calling it after the
_send_json() call in the main_loop.
I would also need to make delayed_events thread-safe so that it can be called
from the gdb thread.
Does this sound like a reasonable way forward to you?

> The event-generating code is in events.py.  It's a bit messy, I'm
> afraid.
>
> Tom