[PATCH 0/3] Implement the DAP notStopped response

[PATCH 0/3] Implement the DAP notStopped response

[Tom Tromey]

This series implements the DAP 'notStopped' response.
Tested on x86-64 Fedora 38.

This probably conflicts with other DAP patches I've sent, but of
course I'll rebase & fix up as needed if/when those land.

---
Tom Tromey (3):
      Automatically run (most) DAP requests in gdb thread
      Remove ExecutionInvoker
      Implement the notStopped DAP response

 gdb/python/lib/gdb/dap/breakpoint.py  |  13 ++---
 gdb/python/lib/gdb/dap/bt.py          |  23 +++-----
 gdb/python/lib/gdb/dap/disassemble.py |   6 +-
 gdb/python/lib/gdb/dap/evaluate.py    |  23 +++-----
 gdb/python/lib/gdb/dap/events.py      |  44 +++++++--------
 gdb/python/lib/gdb/dap/launch.py      |  23 +++-----
 gdb/python/lib/gdb/dap/locations.py   |   4 +-
 gdb/python/lib/gdb/dap/memory.py      |  21 ++-----
 gdb/python/lib/gdb/dap/modules.py     |   4 +-
 gdb/python/lib/gdb/dap/next.py        |  29 ++++++----
 gdb/python/lib/gdb/dap/pause.py       |   7 +--
 gdb/python/lib/gdb/dap/scopes.py      |   5 +-
 gdb/python/lib/gdb/dap/server.py      | 103 ++++++++++++++++++++++++++++------
 gdb/python/lib/gdb/dap/sources.py     |  32 ++++-------
 gdb/python/lib/gdb/dap/threads.py     |  12 +---
 gdb/testsuite/gdb.dap/pause.exp       |   7 +++
 16 files changed, 187 insertions(+), 169 deletions(-)
---
base-commit: 8489362f002d1a844e1a0713438922cdafae7b7c
change-id: 20231107-dap-not-stopped-361e6df17333

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/3] Automatically run (most) DAP requests in gdb thread

[Tom Tromey]

Nearly every DAP request implementation forwards its work to the gdb
thread, using send_gdb_with_response.  This patch refactors the
'request' decorator to make this automatic, and to provide some
parameters so that the unusual requests can express their needs as
well.

In a few spots this simplifies the code by removing an unnecessary
helper function.  This could be done in more places as well if we
wanted.

The main motivation for this patch is that I thought it would be
helpful for cancellation.  I am still working on that, but meanwhile
the parameterization of 'request' makes it easy to handle the
'notStopped' response as well.
---
 gdb/python/lib/gdb/dap/breakpoint.py  | 13 +++----
 gdb/python/lib/gdb/dap/bt.py          | 23 ++++--------
 gdb/python/lib/gdb/dap/disassemble.py |  6 +--
 gdb/python/lib/gdb/dap/evaluate.py    | 23 ++++--------
 gdb/python/lib/gdb/dap/launch.py      | 21 ++++-------
 gdb/python/lib/gdb/dap/locations.py   |  4 +-
 gdb/python/lib/gdb/dap/memory.py      | 21 ++---------
 gdb/python/lib/gdb/dap/modules.py     |  4 +-
 gdb/python/lib/gdb/dap/next.py        | 25 ++++++++-----
 gdb/python/lib/gdb/dap/pause.py       |  5 +--
 gdb/python/lib/gdb/dap/scopes.py      |  5 +--
 gdb/python/lib/gdb/dap/server.py      | 69 +++++++++++++++++++++++++++--------
 gdb/python/lib/gdb/dap/sources.py     | 32 +++++-----------
 gdb/python/lib/gdb/dap/threads.py     | 12 +-----
 14 files changed, 121 insertions(+), 142 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index ae3a5027f35..33aa18e65bc 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -24,7 +24,7 @@ from typing import Optional, Sequence
 
 from .server import request, capability, send_event
 from .sources import make_source
-from .startup import send_gdb_with_response, in_gdb_thread, log_stack
+from .startup import in_gdb_thread, log_stack
 from .typecheck import type_check
 
 
@@ -287,7 +287,7 @@ def set_breakpoint(*, source, breakpoints: Sequence = (), **args):
         # Be sure to include the path in the key, so that we only
         # clear out breakpoints coming from this same source.
         key = "source:" + source["path"]
-        result = send_gdb_with_response(lambda: _set_breakpoints(key, specs))
+        result = _set_breakpoints(key, specs)
     return {
         "breakpoints": result,
     }
@@ -315,9 +315,8 @@ def _rewrite_fn_breakpoint(
 @capability("supportsFunctionBreakpoints")
 def set_fn_breakpoint(*, breakpoints: Sequence, **args):
     specs = [_rewrite_fn_breakpoint(**bp) for bp in breakpoints]
-    result = send_gdb_with_response(lambda: _set_breakpoints("function", specs))
     return {
-        "breakpoints": result,
+        "breakpoints": _set_breakpoints("function", specs),
     }
 
 
@@ -351,9 +350,8 @@ def set_insn_breakpoints(
     *, breakpoints: Sequence, offset: Optional[int] = None, **args
 ):
     specs = [_rewrite_insn_breakpoint(**bp) for bp in breakpoints]
-    result = send_gdb_with_response(lambda: _set_breakpoints("instruction", specs))
     return {
-        "breakpoints": result,
+        "breakpoints": _set_breakpoints("instruction", specs),
     }
 
 
@@ -432,7 +430,6 @@ def set_exception_breakpoints(
     options = [{"filterId": filter} for filter in filters]
     options.extend(filterOptions)
     options = [_rewrite_exception_breakpoint(**bp) for bp in options]
-    result = send_gdb_with_response(lambda: _set_exception_catchpoints(options))
     return {
-        "breakpoints": result,
+        "breakpoints": _set_exception_catchpoints(options),
     }
diff --git a/gdb/python/lib/gdb/dap/bt.py b/gdb/python/lib/gdb/dap/bt.py
index 982d501edf5..df388c11469 100644
--- a/gdb/python/lib/gdb/dap/bt.py
+++ b/gdb/python/lib/gdb/dap/bt.py
@@ -21,16 +21,17 @@ from .frames import frame_id
 from .modules import module_id
 from .server import request, capability
 from .sources import make_source
-from .startup import send_gdb_with_response, in_gdb_thread
 from .state import set_thread
 from .varref import apply_format
 
 
-# Helper function to compute a stack trace.
-@in_gdb_thread
-def _backtrace(thread_id, levels, startFrame, value_format):
-    with apply_format(value_format):
-        set_thread(thread_id)
+@request("stackTrace")
+@capability("supportsDelayedStackTraceLoading")
+def stacktrace(
+    *, levels: int = 0, startFrame: int = 0, threadId: int, format=None, **extra
+):
+    with apply_format(format):
+        set_thread(threadId)
         frames = []
         if levels == 0:
             # Zero means all remaining frames.
@@ -70,13 +71,3 @@ def _backtrace(thread_id, levels, startFrame, value_format):
         return {
             "stackFrames": frames,
         }
-
-
-@request("stackTrace")
-@capability("supportsDelayedStackTraceLoading")
-def stacktrace(
-    *, levels: int = 0, startFrame: int = 0, threadId: int, format=None, **extra
-):
-    return send_gdb_with_response(
-        lambda: _backtrace(threadId, levels, startFrame, format)
-    )
diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index dda2f43b5a3..069549eb7f8 100644
--- a/gdb/python/lib/gdb/dap/disassemble.py
+++ b/gdb/python/lib/gdb/dap/disassemble.py
@@ -16,7 +16,7 @@
 import gdb
 
 from .server import request, capability
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import in_gdb_thread
 
 
 @in_gdb_thread
@@ -54,6 +54,4 @@ def disassemble(
     **extra
 ):
     pc = int(memoryReference, 0) + offset
-    return send_gdb_with_response(
-        lambda: _disassemble(pc, instructionOffset, instructionCount)
-    )
+    return _disassemble(pc, instructionOffset, instructionCount)
diff --git a/gdb/python/lib/gdb/dap/evaluate.py b/gdb/python/lib/gdb/dap/evaluate.py
index ea5a1e61a08..67e103e2ca7 100644
--- a/gdb/python/lib/gdb/dap/evaluate.py
+++ b/gdb/python/lib/gdb/dap/evaluate.py
@@ -20,7 +20,7 @@ from typing import Optional
 
 from .frames import select_frame
 from .server import capability, request, client_bool_capability
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import in_gdb_thread
 from .varref import find_variable, VariableReference, apply_format
 
 
@@ -96,14 +96,12 @@ def eval_request(
 ):
     if context in ("watch", "variables"):
         # These seem to be expression-like.
-        return send_gdb_with_response(lambda: _evaluate(expression, frameId, format))
+        return _evaluate(expression, frameId, format)
     elif context == "hover":
-        return send_gdb_with_response(
-            lambda: _eval_for_hover(expression, frameId, format)
-        )
+        return _eval_for_hover(expression, frameId, format)
     elif context == "repl":
         # Ignore the format for repl evaluation.
-        return send_gdb_with_response(lambda: _repl(expression, frameId))
+        return _repl(expression, frameId)
     else:
         raise Exception('unknown evaluate context "' + context + '"')
 
@@ -127,10 +125,7 @@ def variables(
     if not client_bool_capability("supportsVariablePaging"):
         start = 0
         count = 0
-    result = send_gdb_with_response(
-        lambda: _variables(variablesReference, start, count, format)
-    )
-    return {"variables": result}
+    return {"variables": _variables(variablesReference, start, count, format)}
 
 
 @capability("supportsSetExpression")
@@ -138,9 +133,7 @@ def variables(
 def set_expression(
     *, expression: str, value: str, frameId: Optional[int] = None, format=None, **args
 ):
-    return send_gdb_with_response(
-        lambda: _set_expression(expression, value, frameId, format)
-    )
+    return _set_expression(expression, value, frameId, format)
 
 
 # Helper function to perform an assignment.
@@ -159,6 +152,4 @@ def _set_variable(ref, name, value, value_format):
 def set_variable(
     *, variablesReference: int, name: str, value: str, format=None, **args
 ):
-    return send_gdb_with_response(
-        lambda: _set_variable(variablesReference, name, value, format)
-    )
+    return _set_variable(variablesReference, name, value, format)
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index d13037fa476..e81d2849a8e 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -20,11 +20,11 @@ from typing import Mapping, Optional, Sequence
 
 from .events import ExecutionInvoker
 from .server import request, capability
-from .startup import send_gdb, send_gdb_with_response, in_gdb_thread, exec_and_log
+from .startup import in_gdb_thread, exec_and_log
 
 
-# The program being launched, or None.  This should only be access
-# from the DAP thread.
+# The program being launched, or None.  This should only be accessed
+# from the gdb thread.
 _program = None
 
 
@@ -49,7 +49,7 @@ def _launch_setup(program, cwd, args, env, stopAtBeginningOfMainSubprogram):
 # Any parameters here are necessarily extensions -- DAP requires this
 # from implementations.  Any additions or changes here should be
 # documented in the gdb manual.
-@request("launch")
+@request("launch", response=False)
 def launch(
     *,
     program: Optional[str] = None,
@@ -61,9 +61,7 @@ def launch(
 ):
     global _program
     _program = program
-    send_gdb(
-        lambda: _launch_setup(program, cwd, args, env, stopAtBeginningOfMainSubprogram)
-    )
+    _launch_setup(program, cwd, args, env, stopAtBeginningOfMainSubprogram)
 
 
 @request("attach")
@@ -77,17 +75,14 @@ def attach(*, pid: Optional[int] = None, target: Optional[str] = None, **args):
         cmd = "target remote " + target
     else:
         raise Exception("attach requires either 'pid' or 'target'")
-    # Use send_gdb_with_response to ensure we get an error if the
-    # attach fails.
-    send_gdb_with_response(cmd)
-    return None
+    exec_and_log(cmd)
 
 
 @capability("supportsConfigurationDoneRequest")
-@request("configurationDone")
+@request("configurationDone", response=False)
 def config_done(**args):
     global _program
     if _program is not None:
         # Suppress the continue event, but don't set any particular
         # expected stop.
-        send_gdb(ExecutionInvoker("run", None))
+        ExecutionInvoker("run", None)()
diff --git a/gdb/python/lib/gdb/dap/locations.py b/gdb/python/lib/gdb/dap/locations.py
index a299e8da959..032174df9c8 100644
--- a/gdb/python/lib/gdb/dap/locations.py
+++ b/gdb/python/lib/gdb/dap/locations.py
@@ -20,7 +20,7 @@ from typing import Optional
 
 from .server import capability, request
 from .sources import decode_source
-from .startup import in_gdb_thread, send_gdb_with_response
+from .startup import in_gdb_thread
 
 
 @in_gdb_thread
@@ -46,4 +46,4 @@ def _find_lines(source, start_line, end_line):
 def breakpoint_locations(*, source, line: int, endLine: Optional[int] = None, **extra):
     if endLine is None:
         endLine = line
-    return send_gdb_with_response(lambda: _find_lines(source, line, endLine))
+    return _find_lines(source, line, endLine)
diff --git a/gdb/python/lib/gdb/dap/memory.py b/gdb/python/lib/gdb/dap/memory.py
index 85948bda9f4..6b94f413045 100644
--- a/gdb/python/lib/gdb/dap/memory.py
+++ b/gdb/python/lib/gdb/dap/memory.py
@@ -17,35 +17,22 @@ import base64
 import gdb
 
 from .server import request, capability
-from .startup import send_gdb_with_response, in_gdb_thread
-
-
-@in_gdb_thread
-def _read_memory(addr, count):
-    buf = gdb.selected_inferior().read_memory(addr, count)
-    return base64.b64encode(buf).decode("ASCII")
 
 
 @request("readMemory")
 @capability("supportsReadMemoryRequest")
 def read_memory(*, memoryReference: str, offset: int = 0, count: int, **extra):
     addr = int(memoryReference, 0) + offset
-    buf = send_gdb_with_response(lambda: _read_memory(addr, count))
+    buf = gdb.selected_inferior().read_memory(addr, count)
     return {
         "address": hex(addr),
-        "data": buf,
+        "data": base64.b64encode(buf).decode("ASCII"),
     }
 
 
-@in_gdb_thread
-def _write_memory(addr, contents):
-    buf = base64.b64decode(contents)
-    gdb.selected_inferior().write_memory(addr, buf)
-
-
 @request("writeMemory")
 @capability("supportsWriteMemoryRequest")
 def write_memory(*, memoryReference: str, offset: int = 0, data: str, **extra):
     addr = int(memoryReference, 0) + offset
-    send_gdb_with_response(lambda: _write_memory(addr, data))
-    return {}
+    buf = base64.b64decode(data)
+    gdb.selected_inferior().write_memory(addr, buf)
diff --git a/gdb/python/lib/gdb/dap/modules.py b/gdb/python/lib/gdb/dap/modules.py
index 1aec1cba0ac..87a4f6be669 100644
--- a/gdb/python/lib/gdb/dap/modules.py
+++ b/gdb/python/lib/gdb/dap/modules.py
@@ -16,7 +16,7 @@
 import gdb
 
 from .server import capability, request
-from .startup import in_gdb_thread, send_gdb_with_response
+from .startup import in_gdb_thread
 
 
 @in_gdb_thread
@@ -63,4 +63,4 @@ def _modules(start, count):
 @capability("supportsModulesRequest")
 @request("modules")
 def modules(*, startModule: int = 0, moduleCount: int = 0, **args):
-    return send_gdb_with_response(lambda: _modules(startModule, moduleCount))
+    return _modules(startModule, moduleCount)
diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py
index e5bb8d64da0..ddab9d304a1 100644
--- a/gdb/python/lib/gdb/dap/next.py
+++ b/gdb/python/lib/gdb/dap/next.py
@@ -49,37 +49,42 @@ def _handle_thread_step(thread_id, single_thread, select=False):
     return result
 
 
-@request("next")
+@request("next", response=False)
 def next(
     *, threadId: int, singleThread: bool = False, granularity: str = "statement", **args
 ):
-    send_gdb(lambda: _handle_thread_step(threadId, singleThread))
+    _handle_thread_step(threadId, singleThread)
     cmd = "next"
     if granularity == "instruction":
         cmd += "i"
-    send_gdb(ExecutionInvoker(cmd, StopKinds.STEP))
+    ExecutionInvoker(cmd, StopKinds.STEP)()
 
 
 @capability("supportsSteppingGranularity")
 @capability("supportsSingleThreadExecutionRequests")
-@request("stepIn")
+@request("stepIn", response=False)
 def step_in(
     *, threadId: int, singleThread: bool = False, granularity: str = "statement", **args
 ):
-    send_gdb(lambda: _handle_thread_step(threadId, singleThread))
+    _handle_thread_step(threadId, singleThread)
     cmd = "step"
     if granularity == "instruction":
         cmd += "i"
-    send_gdb(ExecutionInvoker(cmd, StopKinds.STEP))
+    ExecutionInvoker(cmd, StopKinds.STEP)()
 
 
-@request("stepOut")
+@request("stepOut", response=False)
 def step_out(*, threadId: int, singleThread: bool = False, **args):
-    send_gdb(lambda: _handle_thread_step(threadId, singleThread, True))
-    send_gdb(ExecutionInvoker("finish", StopKinds.STEP))
+    _handle_thread_step(threadId, singleThread, True)
+    ExecutionInvoker("finish", StopKinds.STEP)()
 
 
-@request("continue")
+# 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", server=True)
 def continue_request(*, threadId: int, singleThread: bool = False, **args):
     locked = send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread))
     send_gdb(ExecutionInvoker("continue", None))
diff --git a/gdb/python/lib/gdb/dap/pause.py b/gdb/python/lib/gdb/dap/pause.py
index 1e59d630523..d96172c0757 100644
--- a/gdb/python/lib/gdb/dap/pause.py
+++ b/gdb/python/lib/gdb/dap/pause.py
@@ -15,9 +15,8 @@
 
 from .events import StopKinds, ExecutionInvoker
 from .server import request
-from .startup import send_gdb
 
 
-@request("pause")
+@request("pause", response=False)
 def pause(**args):
-    send_gdb(ExecutionInvoker("interrupt -a", StopKinds.PAUSE))
+    ExecutionInvoker("interrupt -a", StopKinds.PAUSE)()
diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
index 87f2ed7547f..d0c30c6b501 100644
--- a/gdb/python/lib/gdb/dap/scopes.py
+++ b/gdb/python/lib/gdb/dap/scopes.py
@@ -16,7 +16,7 @@
 import gdb
 
 from .frames import frame_for_id
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import in_gdb_thread
 from .server import request
 from .varref import BaseReference
 
@@ -120,5 +120,4 @@ def _get_scope(id):
 
 @request("scopes")
 def scopes(*, frameId: int, **extra):
-    scopes = send_gdb_with_response(lambda: _get_scope(frameId))
-    return {"scopes": scopes}
+    return {"scopes": _get_scope(frameId)}
diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index 62bf240c1e9..d2980ad4031 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -20,11 +20,14 @@ import sys
 
 from .io import start_json_writer, read_json
 from .startup import (
+    exec_and_log,
     in_dap_thread,
+    in_gdb_thread,
+    send_gdb,
+    send_gdb_with_response,
     start_thread,
     log,
     log_stack,
-    send_gdb_with_response,
 )
 from .typecheck import type_check
 
@@ -160,12 +163,27 @@ def send_event(event, body=None):
     _server.send_event(event, body)
 
 
-def request(name):
-    """A decorator that indicates that the wrapper function implements
-    the DAP request NAME."""
+def request(name: str, *, response: bool = True, server: bool = False):
+    """A decorator for DAP requests.
+
+    This registers the function as the implementation of the DAP
+    request NAME.  By default, the function is invoked in the gdb
+    thread, and its result is returned as the 'body' of the DAP
+    response.
+
+    Some keyword arguments are provided as well:
+
+    If RESPONSE is False, the result of the function will not be
+    waited for and no 'body' will be in the response.
+
+    If SERVER is True, the function will be invoked in the DAP thread.
+    When SERVER is True, RESPONSE may not be False.
+    """
+
+    # Validate the parameters.
+    assert not server or response
 
     def wrap(func):
-        global _commands
         code = func.__code__
         # We don't permit requests to have positional arguments.
         try:
@@ -176,11 +194,32 @@ def request(name):
         assert code.co_argcount == 0
         # A request must have a **args parameter.
         assert code.co_flags & inspect.CO_VARKEYWORDS
-        # All requests must run in the DAP thread.
-        # Also type-check the calls.
-        func = in_dap_thread(type_check(func))
-        _commands[name] = func
-        return func
+
+        # Type-check the calls.
+        func = type_check(func)
+
+        # Verify that the function is run on the correct thread.
+        if server:
+            cmd = in_dap_thread(func)
+        else:
+            func = in_gdb_thread(func)
+
+            if response:
+
+                def sync_call(**args):
+                    return send_gdb_with_response(lambda: func(**args))
+
+                cmd = sync_call
+            else:
+
+                def non_sync_call(**args):
+                    return send_gdb(lambda: func(**args))
+
+                cmd = non_sync_call
+
+        global _commands
+        _commands[name] = cmd
+        return cmd
 
     return wrap
 
@@ -208,7 +247,7 @@ def client_bool_capability(name):
     return False
 
 
-@request("initialize")
+@request("initialize", server=True)
 def initialize(**args):
     global _server, _capabilities
     _server.config = args
@@ -219,14 +258,12 @@ def initialize(**args):
 @request("terminate")
 @capability("supportsTerminateRequest")
 def terminate(**args):
-    # We can ignore the result here, because we only really need to
-    # synchronize.
-    send_gdb_with_response("kill")
+    exec_and_log("kill")
 
 
-@request("disconnect")
+@request("disconnect", server=True)
 @capability("supportTerminateDebuggee")
 def disconnect(*, terminateDebuggee: bool = False, **args):
     if terminateDebuggee:
-        terminate()
+        send_gdb_with_response("kill")
     _server.shutdown()
diff --git a/gdb/python/lib/gdb/dap/sources.py b/gdb/python/lib/gdb/dap/sources.py
index 00a70701d26..821205cedd1 100644
--- a/gdb/python/lib/gdb/dap/sources.py
+++ b/gdb/python/lib/gdb/dap/sources.py
@@ -18,7 +18,7 @@ import os
 import gdb
 
 from .server import request, capability
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import in_gdb_thread
 
 
 # The next available source reference ID.  Must be greater than 0.
@@ -76,8 +76,9 @@ def decode_source(source):
     return _id_map[ref]["path"]
 
 
-@in_gdb_thread
-def _sources():
+@request("loadedSources")
+@capability("supportsLoadedSourcesRequest")
+def loaded_sources(**extra):
     result = []
     for elt in gdb.execute_mi("-file-list-exec-source-files")["files"]:
         result.append(make_source(elt["fullname"], elt["file"]))
@@ -86,24 +87,6 @@ def _sources():
     }
 
 
-@request("loadedSources")
-@capability("supportsLoadedSourcesRequest")
-def loaded_sources(**extra):
-    return send_gdb_with_response(_sources)
-
-
-# This helper is needed because we must only access the globals here
-# from the gdb thread.
-@in_gdb_thread
-def _get_source(source):
-    filename = decode_source(source)
-    with open(filename) as f:
-        content = f.read()
-    return {
-        "content": content,
-    }
-
-
 @request("source")
 def source(*, source=None, sourceReference: int, **extra):
     # The 'sourceReference' parameter is required by the spec, but is
@@ -111,4 +94,9 @@ def source(*, source=None, sourceReference: int, **extra):
     # 'source' is preferred.
     if source is None:
         source = {"sourceReference": sourceReference}
-    return send_gdb_with_response(lambda: _get_source(source))
+    filename = decode_source(source)
+    with open(filename) as f:
+        content = f.read()
+    return {
+        "content": content,
+    }
diff --git a/gdb/python/lib/gdb/dap/threads.py b/gdb/python/lib/gdb/dap/threads.py
index a4ca9e61488..515966761ce 100644
--- a/gdb/python/lib/gdb/dap/threads.py
+++ b/gdb/python/lib/gdb/dap/threads.py
@@ -16,7 +16,6 @@
 import gdb
 
 from .server import request
-from .startup import send_gdb_with_response, in_gdb_thread
 
 
 def _thread_name(thr):
@@ -27,9 +26,8 @@ def _thread_name(thr):
     return None
 
 
-# A helper function to construct the list of threads.
-@in_gdb_thread
-def _get_threads():
+@request("threads")
+def threads(**args):
     result = []
     for thr in gdb.selected_inferior().threads():
         one_result = {
@@ -39,12 +37,6 @@ def _get_threads():
         if name is not None:
             one_result["name"] = name
         result.append(one_result)
-    return result
-
-
-@request("threads")
-def threads(**args):
-    result = send_gdb_with_response(_get_threads)
     return {
         "threads": result,
     }

-- 
2.41.0

[PATCH 2/3] Remove ExecutionInvoker

[Tom Tromey]

ExecutionInvoker is no longer really needed, due to the previous DAP
refactoring.  This patch removes it in favor of an ordinary function.
One spot (the 'continue' request) could still have used it, but is
more succinctly expressed as a lambda.
---
 gdb/python/lib/gdb/dap/events.py | 29 +++++++----------------------
 gdb/python/lib/gdb/dap/launch.py |  4 ++--
 gdb/python/lib/gdb/dap/next.py   | 10 +++++-----
 gdb/python/lib/gdb/dap/pause.py  |  4 ++--
 4 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py
index e9ddcab135f..09214ec3dc8 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -17,7 +17,7 @@ import enum
 import gdb
 
 from .server import send_event
-from .startup import in_gdb_thread, Invoker, log
+from .startup import exec_and_log, in_gdb_thread, log
 from .modules import is_module, make_module
 
 
@@ -111,29 +111,14 @@ _expected_stop = None
 
 
 @in_gdb_thread
-def expect_stop(reason):
-    """Indicate that a stop is expected, for the reason given."""
+def exec_and_expect_stop(cmd, reason):
+    """Indicate that a stop is expected, then execute CMD"""
     global _expected_stop
     _expected_stop = reason
-
-
-# A wrapper for Invoker that also sets the expected stop.
-class ExecutionInvoker(Invoker):
-    """A subclass of Invoker that sets the expected stop.
-    Note that this assumes that the command will restart the inferior,
-    so it will also cause ContinuedEvents to be suppressed."""
-
-    def __init__(self, cmd, expected):
-        super().__init__(cmd)
-        self.expected = expected
-
-    @in_gdb_thread
-    def __call__(self):
-        expect_stop(self.expected)
-        global _suppress_cont
-        _suppress_cont = True
-        # FIXME if the call fails should we clear _suppress_cont?
-        super().__call__()
+    global _suppress_cont
+    _suppress_cont = True
+    # FIXME if the call fails should we clear _suppress_cont?
+    exec_and_log(cmd)
 
 
 @in_gdb_thread
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index e81d2849a8e..ab704c7a7cc 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -18,7 +18,7 @@ import gdb
 # These are deprecated in 3.9, but required in older versions.
 from typing import Mapping, Optional, Sequence
 
-from .events import ExecutionInvoker
+from .events import exec_and_expect_stop
 from .server import request, capability
 from .startup import in_gdb_thread, exec_and_log
 
@@ -85,4 +85,4 @@ def config_done(**args):
     if _program is not None:
         # Suppress the continue event, but don't set any particular
         # expected stop.
-        ExecutionInvoker("run", None)()
+        exec_and_expect_stop("run", None)
diff --git a/gdb/python/lib/gdb/dap/next.py b/gdb/python/lib/gdb/dap/next.py
index ddab9d304a1..d12267ea354 100644
--- a/gdb/python/lib/gdb/dap/next.py
+++ b/gdb/python/lib/gdb/dap/next.py
@@ -15,7 +15,7 @@
 
 import gdb
 
-from .events import StopKinds, ExecutionInvoker
+from .events import StopKinds, exec_and_expect_stop
 from .server import capability, request
 from .startup import in_gdb_thread, send_gdb, send_gdb_with_response
 from .state import set_thread
@@ -57,7 +57,7 @@ def next(
     cmd = "next"
     if granularity == "instruction":
         cmd += "i"
-    ExecutionInvoker(cmd, StopKinds.STEP)()
+    exec_and_expect_stop(cmd, StopKinds.STEP)
 
 
 @capability("supportsSteppingGranularity")
@@ -70,13 +70,13 @@ def step_in(
     cmd = "step"
     if granularity == "instruction":
         cmd += "i"
-    ExecutionInvoker(cmd, StopKinds.STEP)()
+    exec_and_expect_stop(cmd, StopKinds.STEP)
 
 
 @request("stepOut", response=False)
 def step_out(*, threadId: int, singleThread: bool = False, **args):
     _handle_thread_step(threadId, singleThread, True)
-    ExecutionInvoker("finish", StopKinds.STEP)()
+    exec_and_expect_stop("finish", StopKinds.STEP)
 
 
 # This is a server-side request because it is funny: it wants to
@@ -87,5 +87,5 @@ def step_out(*, threadId: int, singleThread: bool = False, **args):
 @request("continue", server=True)
 def continue_request(*, threadId: int, singleThread: bool = False, **args):
     locked = send_gdb_with_response(lambda: _handle_thread_step(threadId, singleThread))
-    send_gdb(ExecutionInvoker("continue", None))
+    send_gdb(lambda: exec_and_expect_stop("continue", None))
     return {"allThreadsContinued": not locked}
diff --git a/gdb/python/lib/gdb/dap/pause.py b/gdb/python/lib/gdb/dap/pause.py
index d96172c0757..d276ab1cb92 100644
--- a/gdb/python/lib/gdb/dap/pause.py
+++ b/gdb/python/lib/gdb/dap/pause.py
@@ -13,10 +13,10 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from .events import StopKinds, ExecutionInvoker
+from .events import StopKinds, exec_and_expect_stop
 from .server import request
 
 
 @request("pause", response=False)
 def pause(**args):
-    ExecutionInvoker("interrupt -a", StopKinds.PAUSE)()
+    exec_and_expect_stop("interrupt -a", StopKinds.PAUSE)

-- 
2.41.0

[PATCH 3/3] Implement the notStopped DAP response

[Tom Tromey]

DAP specifies that a request can fail with the "notStopped" message if
the inferior is running but the request requires that it first be
stopped.

This patch implements this for gdb.  Most requests are assumed to
require a stopped inferior, and the exceptions are noted by a new
'request' parameter.

You may notice that the implementation is a bit racy.  I think this is
inherent -- unless the client waits for a stop event before sending a
request, the request may be processed at any time relative to a stop.

https://sourceware.org/bugzilla/show_bug.cgi?id=31037
---
 gdb/python/lib/gdb/dap/events.py | 15 +++++++++++++++
 gdb/python/lib/gdb/dap/pause.py  |  2 +-
 gdb/python/lib/gdb/dap/server.py | 38 +++++++++++++++++++++++++++++++++++---
 gdb/testsuite/gdb.dap/pause.exp  |  7 +++++++
 4 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py
index 09214ec3dc8..bfc3f9ee1dc 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -21,8 +21,17 @@ from .startup import exec_and_log, in_gdb_thread, log
 from .modules import is_module, make_module
 
 
+# True when the inferior is thought to be running, False otherwise.
+# This may be accessed from any thread, which can be racy.  However,
+# this unimportant because this global is only used for the
+# 'notStopped' response, which itself is inherently racy.
+inferior_running = False
+
+
 @in_gdb_thread
 def _on_exit(event):
+    global inferior_running
+    inferior_running = False
     code = 0
     if hasattr(event, "exit_code"):
         code = event.exit_code
@@ -48,6 +57,8 @@ def thread_event(event, reason):
 
 @in_gdb_thread
 def _new_thread(event):
+    global inferior_running
+    inferior_running = True
     thread_event(event, "started")
 
 
@@ -85,6 +96,8 @@ _suppress_cont = False
 
 @in_gdb_thread
 def _cont(event):
+    global inferior_running
+    inferior_running = True
     global _suppress_cont
     if _suppress_cont:
         log("_suppress_cont case")
@@ -123,6 +136,8 @@ def exec_and_expect_stop(cmd, reason):
 
 @in_gdb_thread
 def _on_stop(event):
+    global inferior_running
+    inferior_running = False
     log("entering _on_stop: " + repr(event))
     global _expected_stop
     obj = {
diff --git a/gdb/python/lib/gdb/dap/pause.py b/gdb/python/lib/gdb/dap/pause.py
index d276ab1cb92..3d9b4ae108e 100644
--- a/gdb/python/lib/gdb/dap/pause.py
+++ b/gdb/python/lib/gdb/dap/pause.py
@@ -17,6 +17,6 @@ from .events import StopKinds, exec_and_expect_stop
 from .server import request
 
 
-@request("pause", response=False)
+@request("pause", response=False, must_be_stopped=False)
 def pause(**args):
     exec_and_expect_stop("interrupt -a", StopKinds.PAUSE)
diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index d2980ad4031..d3b7f75c224 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -13,6 +13,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import functools
 import inspect
 import json
 import queue
@@ -163,7 +164,27 @@ def send_event(event, body=None):
     _server.send_event(event, body)
 
 
-def request(name: str, *, response: bool = True, server: bool = False):
+# A helper decorator that checks whether the inferior is running.
+def _check_not_running(func):
+    @functools.wraps(func)
+    def check(*args, **kwargs):
+        # Import this as late as possible.  FIXME.
+        from .events import inferior_running
+
+        if inferior_running:
+            raise Exception("notStopped")
+        return func(*args, **kwargs)
+
+    return check
+
+
+def request(
+    name: str,
+    *,
+    response: bool = True,
+    server: bool = False,
+    must_be_stopped: bool = True
+):
     """A decorator for DAP requests.
 
     This registers the function as the implementation of the DAP
@@ -178,6 +199,11 @@ def request(name: str, *, response: bool = True, server: bool = False):
 
     If SERVER is True, the function will be invoked in the DAP thread.
     When SERVER is True, RESPONSE may not be False.
+
+    If MUST_BE_STOPPED is True (the default), then the request will
+    fail with the 'notStopped' reason if it is processed while the
+    inferior is running.  When MUST_BE_STOPPED is False, the request
+    will proceed regardless of the inferior's state.
     """
 
     # Validate the parameters.
@@ -217,6 +243,12 @@ def request(name: str, *, response: bool = True, server: bool = False):
 
                 cmd = non_sync_call
 
+        # If needed, check that the inferior is not running.  This
+        # wrapping is done last, so the check is done first, before
+        # trying to dispatch the request to another thread.
+        if must_be_stopped:
+            cmd = _check_not_running(cmd)
+
         global _commands
         _commands[name] = cmd
         return cmd
@@ -255,13 +287,13 @@ def initialize(**args):
     return _capabilities.copy()
 
 
-@request("terminate")
+@request("terminate", must_be_stopped=False)
 @capability("supportsTerminateRequest")
 def terminate(**args):
     exec_and_log("kill")
 
 
-@request("disconnect", server=True)
+@request("disconnect", server=True, must_be_stopped=False)
 @capability("supportTerminateDebuggee")
 def disconnect(*, terminateDebuggee: bool = False, **args):
     if terminateDebuggee:
diff --git a/gdb/testsuite/gdb.dap/pause.exp b/gdb/testsuite/gdb.dap/pause.exp
index 27955d31526..558ede982ee 100644
--- a/gdb/testsuite/gdb.dap/pause.exp
+++ b/gdb/testsuite/gdb.dap/pause.exp
@@ -32,6 +32,13 @@ if {[dap_launch $testfile] == ""} {
 dap_check_request_and_response "start inferior" configurationDone
 dap_wait_for_event_and_check "inferior started" thread "body reason" started
 
+set resp [lindex [dap_request_and_response evaluate {o expression [s 23]}] \
+	      0]
+gdb_assert {[dict get $resp success] == "false"} \
+    "evaluate failed while inferior executing"
+gdb_assert {[dict get $resp message] == "notStopped"} \
+    "evaluate issued notStopped"
+
 dap_check_request_and_response pause pause \
     {o threadId [i 1]}
 

-- 
2.41.0

Re: [PATCH 3/3] Implement the notStopped DAP response

[Kévin Le Gouguec]

Tom Tromey <tromey@adacore.com> writes:

> @@ -21,8 +21,17 @@ from .startup import exec_and_log, in_gdb_thread, log
>  from .modules import is_module, make_module
>  
>  
> +# True when the inferior is thought to be running, False otherwise.
> +# This may be accessed from any thread, which can be racy.  However,
> +# this unimportant because this global is only used for the
> +# 'notStopped' response, which itself is inherently racy.
> +inferior_running = False
> +
> +

Out of curiosity, could `inferior_thread ()->state` have helped here?
(Exposing it as a gdb.Inferior method perhaps?)

Not necessarily with the "racy" part; thinking more of the "keep a
single source of truth" part (i.e. dispense with relying on event
notifications).

> +# A helper decorator that checks whether the inferior is running.
> +def _check_not_running(func):
> +    @functools.wraps(func)
> +    def check(*args, **kwargs):
> +        # Import this as late as possible.  FIXME.
> +        from .events import inferior_running
> +
> +        if inferior_running:
> +            raise Exception("notStopped")
> +        return func(*args, **kwargs)
> +
> +    return check

Assuming this is what the FIXME is about (apologies if not): an
alternative could be doing a bare `import .events`, then lookup
`.events.inferior_running` in check().  Or defining an accessor function
in .events and importing that instead of this global.

> @@ -178,6 +199,11 @@ def request(name: str, *, response: bool = True, server: bool = False):
>  
>      If SERVER is True, the function will be invoked in the DAP thread.
>      When SERVER is True, RESPONSE may not be False.
> +
> +    If MUST_BE_STOPPED is True (the default), then the request will
> +    fail with the 'notStopped' reason if it is processed while the
> +    inferior is running.  When MUST_BE_STOPPED is False, the request
> +    will proceed regardless of the inferior's state.
>      """
>  
>      # Validate the parameters.

(Feeling like EXPECT_STOPPED might better convey that parameter's role
as a precondition, rather than as a "call to action" for request(); tiny
nit though, the doc clarifies the intent anyway)

Re: [PATCH 3/3] Implement the notStopped DAP response

[Tom Tromey]

>>>>> "Kévin" == Kévin Le Gouguec <legouguec@adacore.com> writes:

>> +# True when the inferior is thought to be running, False otherwise.
>> +# This may be accessed from any thread, which can be racy.  However,
>> +# this unimportant because this global is only used for the
>> +# 'notStopped' response, which itself is inherently racy.
>> +inferior_running = False

Kévin> Out of curiosity, could `inferior_thread ()->state` have helped here?
Kévin> (Exposing it as a gdb.Inferior method perhaps?)

Kévin> Not necessarily with the "racy" part; thinking more of the "keep a
Kévin> single source of truth" part (i.e. dispense with relying on event
Kévin> notifications).

Possibly, but it couldn't be called from a non-gdb thread, so we'd have
to send a message to the gdb thread and then a message back.

>> +        # Import this as late as possible.  FIXME.
>> +        from .events import inferior_running

Kévin> Assuming this is what the FIXME is about (apologies if not): an
Kévin> alternative could be doing a bare `import .events`, then lookup
Kévin> `.events.inferior_running` in check().  Or defining an accessor function
Kévin> in .events and importing that instead of this global.

The problem is a circular import, as events imports from server.
I may just remove this comment?  Not sure.

Originally here I was thinking of putting more event listeners in
server.py, but it seemed strange not to just reuse the existing ones.

Tom

Re: [PATCH 3/3] Implement the notStopped DAP response

[Tom Tromey]

>>>>> "Kévin" == Kévin Le Gouguec <legouguec@adacore.com> writes:

>> @@ -178,6 +199,11 @@ def request(name: str, *, response: bool = True, server: bool = False):
>> 
>> If SERVER is True, the function will be invoked in the DAP thread.
>> When SERVER is True, RESPONSE may not be False.
>> +
>> +    If MUST_BE_STOPPED is True (the default), then the request will
>> +    fail with the 'notStopped' reason if it is processed while the
>> +    inferior is running.  When MUST_BE_STOPPED is False, the request
>> +    will proceed regardless of the inferior's state.
>> """
>> 
>> # Validate the parameters.

Kévin> (Feeling like EXPECT_STOPPED might better convey that parameter's role
Kévin> as a precondition, rather than as a "call to action" for request(); tiny
Kévin> nit though, the doc clarifies the intent anyway)

I made this change.

Tom

Re: [PATCH 3/3] Implement the notStopped DAP response

[Kévin Le Gouguec]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Kévin" == Kévin Le Gouguec <legouguec@adacore.com> writes:
>
>>> +# True when the inferior is thought to be running, False otherwise.
>>> +# This may be accessed from any thread, which can be racy.  However,
>>> +# this unimportant because this global is only used for the
>>> +# 'notStopped' response, which itself is inherently racy.
>>> +inferior_running = False
>
> Kévin> Out of curiosity, could `inferior_thread ()->state` have helped here?
> Kévin> (Exposing it as a gdb.Inferior method perhaps?)
>
> Kévin> Not necessarily with the "racy" part; thinking more of the "keep a
> Kévin> single source of truth" part (i.e. dispense with relying on event
> Kévin> notifications).
>
> Possibly, but it couldn't be called from a non-gdb thread, so we'd have
> to send a message to the gdb thread and then a message back.

Right, thanks for confirming.  No strong opinion on my side; if we are
fine with the notifications, might as well save the roundtrip.

>>> +        # Import this as late as possible.  FIXME.
>>> +        from .events import inferior_running
>
> Kévin> Assuming this is what the FIXME is about (apologies if not): an
> Kévin> alternative could be doing a bare `import .events`, then lookup
> Kévin> `.events.inferior_running` in check().  Or defining an accessor function
> Kévin> in .events and importing that instead of this global.
>
> The problem is a circular import, as events imports from server.
> I may just remove this comment?  Not sure.

Oh, hadn't caught that; somehow I convinced myself this was about "from
foo import bar" failing to do TRT when bar is re-assigned within foo,
IOW here a top-level "from .events import inferior_running" would fail
to reflect changes from event notifications.

Hm.  So I guess another solution would be moving inferior_running under
server.py, and making events set .server.inferior_running?  Or…

> Originally here I was thinking of putting more event listeners in
> server.py, but it seemed strange not to just reuse the existing ones.

… this.  The late-import is fine too of course.

Re: [PATCH 3/3] Implement the notStopped DAP response

[Tom Tromey]

>> Possibly, but it couldn't be called from a non-gdb thread, so we'd have
>> to send a message to the gdb thread and then a message back.

Kévin> Right, thanks for confirming.  No strong opinion on my side; if we are
Kévin> fine with the notifications, might as well save the roundtrip.

Yeah, I tend to think it's ok as-is.

>> Originally here I was thinking of putting more event listeners in
>> server.py, but it seemed strange not to just reuse the existing ones.

Kévin> … this.  The late-import is fine too of course.

I ended up just replacing the FIXME with an explanation.

Probably the organization here could be improved somehow but it would be
just to avoid a local import, which is ugly but more like a pet peeve
kind of ugly than something really bad.

Tom

Re: [PATCH 1/3] Automatically run (most) DAP requests in gdb thread

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

On Tue, Nov 7, 2023 at 7:29 PM Tom Tromey <tromey@adacore.com> wrote:

> Nearly every DAP request implementation forwards its work to the gdb
> thread, using send_gdb_with_response.  This patch refactors the
> 'request' decorator to make this automatic, and to provide some
> parameters so that the unusual requests can express their needs as
> well.
>
> In a few spots this simplifies the code by removing an unnecessary
> helper function.  This could be done in more places as well if we
> wanted.
>
> The main motivation for this patch is that I thought it would be
> helpful for cancellation.  I am still working on that, but meanwhile
> the parameterization of 'request' makes it easy to handle the
> 'notStopped' response as well.
>
> I can confirm this commit introduces no regressions  with Fedora Rawhide
ppc64le. Also I like the
simplification this brings.

Thanks,
Alexandra

Re: [PATCH 2/3] Remove ExecutionInvoker

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Tue, Nov 7, 2023 at 7:29 PM Tom Tromey <tromey@adacore.com> wrote:

> ExecutionInvoker is no longer really needed, due to the previous DAP
> refactoring.  This patch removes it in favor of an ordinary function.
> One spot (the 'continue' request) could still have used it, but is
> more succinctly expressed as a lambda.
> ---
>
>
I can confirm this work causes no regressions tested for Fedora Rawhide
ppc64le.