Automatically run (most) DAP requests in gdb thread

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.

Reviewed-by: Kévin Le Gouguec <legouguec@adacore.com>

14 files changed, 116 insertions, 130 deletions

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index ae3a502..33aa18e 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 c63ca6d..a770b25 100644
--- a/gdb/python/lib/gdb/dap/bt.py
+++ b/gdb/python/lib/gdb/dap/bt.py
@@ -24,7 +24,7 @@ from .modules import module_id
 from .scopes import symbol_value
 from .server import request, capability
 from .sources import make_source
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import in_gdb_thread
 from .state import set_thread
 from .typecheck import type_check
 from .varref import apply_format
@@ -147,6 +147,4 @@ def stacktrace(
     if format is None:
         format = {}
     format = check_stack_frame(**format)
-    return send_gdb_with_response(
-        lambda: _backtrace(threadId, levels, startFrame, format)
-    )
+    return _backtrace(threadId, levels, startFrame, format)
diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index dda2f43..069549e 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 ea5a1e6..67e103e 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 d13037f..e81d284 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 a299e8d..032174d 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 85948bd..6b94f41 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 1aec1cb..87a4f6b 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 e5bb8d6..431020e 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", on_dap_thread=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 1e59d63..d96172c 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 e526dfb..63cd325 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
 
@@ -133,5 +133,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 62bf240..4430d2a 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, on_dap_thread: 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 ON_DAP_THREAD is True, the function will be invoked in the DAP
+    thread.  When ON_DAP_THREAD is True, RESPONSE may not be False.
+    """
+
+    # Validate the parameters.
+    assert not on_dap_thread 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 on_dap_thread:
+            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", on_dap_thread=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", on_dap_thread=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 00a7070..821205c 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 a4ca9e6..5159667 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,
     }