Fix DAP launch and configurationDone requests

Co-workers at AdaCore pointed out that gdb incorrectly implements the
DAP launch and configurationDone requests.  It's somewhat strange to
me, but the spec does in fact say that configuration requests should
occur before the executable is known to gdb.  This was clarified in
this bug report against the spec:

    microsoft/debug-adapter-protocol#452

Fixing 'launch' to start the inferior was straightforward, but this
then required some changes to how breakpoints are handled.  In
particular, now gdb will emit the "pending" reason on a breakpoint,
and will suppress breakpoint events during breakpoint setting.

gdb/python/lib/gdb/dap/breakpoint.py

@@ -28,17 +28,6 @@
 from .typecheck import type_check
 
 
-@in_gdb_thread
-def _bp_modified(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "changed",
-            "breakpoint": _breakpoint_descriptor(event),
-        },
-    )
-
-
 # True when suppressing new breakpoint events.
 _suppress_bp = False
 
@@ -55,6 +44,19 @@ def suppress_new_breakpoint_event():
         _suppress_bp = saved
 
 
+@in_gdb_thread
+def _bp_modified(event):
+    global _suppress_bp
+    if not _suppress_bp:
+        send_event(
+            "breakpoint",
+            {
+                "reason": "changed",
+                "breakpoint": _breakpoint_descriptor(event),
+            },
+        )
+
+
 @in_gdb_thread
 def _bp_created(event):
     global _suppress_bp
@@ -70,13 +72,15 @@ def _bp_created(event):
 
 @in_gdb_thread
 def _bp_deleted(event):
-    send_event(
-        "breakpoint",
-        {
-            "reason": "removed",
-            "breakpoint": _breakpoint_descriptor(event),
-        },
-    )
+    global _suppress_bp
+    if not _suppress_bp:
+        send_event(
+            "breakpoint",
+            {
+                "reason": "removed",
+                "breakpoint": _breakpoint_descriptor(event),
+            },
+        )
 
 
 gdb.events.breakpoint_created.connect(_bp_created)
@@ -97,11 +101,10 @@ def _breakpoint_descriptor(bp):
     "Return the Breakpoint object descriptor given a gdb Breakpoint."
     result = {
         "id": bp.number,
-        # We always use True here, because this field just indicates
-        # that breakpoint creation was successful -- and if we have a
-        # breakpoint, the creation succeeded.
-        "verified": True,
+        "verified": not bp.pending,
     }
+    if bp.pending:
+        result["reason"] = "pending"
     if bp.locations:
         # Just choose the first location, because DAP doesn't allow
         # multiple locations.  See
@@ -146,52 +149,55 @@ def _set_breakpoints_callback(kind, specs, creator):
         saved_map = {}
     breakpoint_map[kind] = {}
     result = []
-    for spec in specs:
-        # It makes sense to reuse a breakpoint even if the condition
-        # or ignore count differs, so remove these entries from the
-        # spec first.
-        (condition, hit_condition) = _remove_entries(spec, "condition", "hitCondition")
-        keyspec = frozenset(spec.items())
-
-        # Create or reuse a breakpoint.  If asked, set the condition
-        # or the ignore count.  Catch errors coming from gdb and
-        # report these as an "unverified" breakpoint.
-        bp = None
-        try:
-            if keyspec in saved_map:
-                bp = saved_map.pop(keyspec)
-            else:
-                with suppress_new_breakpoint_event():
+    with suppress_new_breakpoint_event():
+        for spec in specs:
+            # It makes sense to reuse a breakpoint even if the condition
+            # or ignore count differs, so remove these entries from the
+            # spec first.
+            (condition, hit_condition) = _remove_entries(
+                spec, "condition", "hitCondition"
+            )
+            keyspec = frozenset(spec.items())
+
+            # Create or reuse a breakpoint.  If asked, set the condition
+            # or the ignore count.  Catch errors coming from gdb and
+            # report these as an "unverified" breakpoint.
+            bp = None
+            try:
+                if keyspec in saved_map:
+                    bp = saved_map.pop(keyspec)
+                else:
                     bp = creator(**spec)
 
-            bp.condition = condition
-            if hit_condition is None:
-                bp.ignore_count = 0
-            else:
-                bp.ignore_count = int(
-                    parse_and_eval(hit_condition, global_context=True)
+                bp.condition = condition
+                if hit_condition is None:
+                    bp.ignore_count = 0
+                else:
+                    bp.ignore_count = int(
+                        parse_and_eval(hit_condition, global_context=True)
+                    )
+
+                # Reaching this spot means success.
+                breakpoint_map[kind][keyspec] = bp
+                result.append(_breakpoint_descriptor(bp))
+            # Exceptions other than gdb.error are possible here.
+            except Exception as e:
+                # Don't normally want to see this, as it interferes with
+                # the test suite.
+                log_stack(LogLevel.FULL)
+                # Maybe the breakpoint was made but setting an attribute
+                # failed.  We still want this to fail.
+                if bp is not None:
+                    bp.delete()
+                # Breakpoint creation failed.
+                result.append(
+                    {
+                        "verified": False,
+                        "reason": "failed",
+                        "message": str(e),
+                    }
                 )
 
-            # Reaching this spot means success.
-            breakpoint_map[kind][keyspec] = bp
-            result.append(_breakpoint_descriptor(bp))
-        # Exceptions other than gdb.error are possible here.
-        except Exception as e:
-            # Don't normally want to see this, as it interferes with
-            # the test suite.
-            log_stack(LogLevel.FULL)
-            # Maybe the breakpoint was made but setting an attribute
-            # failed.  We still want this to fail.
-            if bp is not None:
-                bp.delete()
-            # Breakpoint creation failed.
-            result.append(
-                {
-                    "verified": False,
-                    "message": str(e),
-                }
-            )
-
     # Delete any breakpoints that were not reused.
     for entry in saved_map.values():
         entry.delete()

gdb/python/lib/gdb/dap/launch.py

@@ -23,16 +23,6 @@
 from .startup import exec_and_log, DAPException
 
 
-# The program being launched, or None.  This should only be accessed
-# from the gdb thread.
-_program = None
-
-
-# True if the program was attached, False otherwise.  This should only
-# be accessed from the gdb thread.
-_attach = False
-
-
 # Any parameters here are necessarily extensions -- DAP requires this
 # from implementations.  Any additions or changes here should be
 # documented in the gdb manual.
@@ -46,10 +36,6 @@ def launch(
     stopAtBeginningOfMainSubprogram: bool = False,
     **extra,
 ):
-    global _program
-    _program = program
-    global _attach
-    _attach = False
     if cwd is not None:
         exec_and_log("cd " + cwd)
     if program is not None:
@@ -64,6 +50,8 @@ def launch(
         inf.clear_env()
         for name, value in env.items():
             inf.set_env(name, value)
+    expect_process("process")
+    exec_and_expect_stop("run")
 
 
 @request("attach")
@@ -74,11 +62,6 @@ def attach(
     target: Optional[str] = None,
     **args,
 ):
-    # Ensure configurationDone does not try to run.
-    global _attach
-    _attach = True
-    global _program
-    _program = program
     if program is not None:
         exec_and_log("file " + program)
     if pid is not None:
@@ -93,9 +76,7 @@ def attach(
 
 
 @capability("supportsConfigurationDoneRequest")
-@request("configurationDone", response=False)
+@request("configurationDone")
 def config_done(**args):
-    global _attach
-    if not _attach:
-        expect_process("process")
-        exec_and_expect_stop("run")
+    # Nothing to do.
+    return None

gdb/testsuite/gdb.dap/ada-arrays.exp

@@ -29,7 +29,7 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable debug] != ""} {
   return -1
 }
 
-if {[dap_launch $testfile] == ""} {
+if {[dap_initialize] == ""} {
     return
 }
 
@@ -42,7 +42,11 @@ set obj [dap_check_request_and_response "set breakpoint by line number" \
 		  [list s cstuff.c] $line]]
 set line_bpno [dap_get_breakpoint_number $obj]
 
-dap_check_request_and_response "start inferior" configurationDone
+dap_check_request_and_response "configurationDone" configurationDone
+
+if {[dap_launch $testfile] == ""} {
+    return
+}
 dap_wait_for_event_and_check "inferior started" thread "body reason" started
 
 dap_wait_for_event_and_check "stopped at line breakpoint" stopped \

gdb/testsuite/gdb.dap/ada-nested.exp

@@ -27,7 +27,7 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
     return -1
 }
 
-if {[dap_launch $binfile] == ""} {
+if {[dap_initialize] == ""} {
     return
 }
 
@@ -39,8 +39,11 @@ set obj [dap_check_request_and_response "set breakpoint" \
 		  [list s $srcfile] $line]]
 set fn_bpno [dap_get_breakpoint_number $obj]
 
-dap_check_request_and_response "start inferior" configurationDone
+dap_check_request_and_response "configurationDone" configurationDone
 
+if {[dap_launch $binfile] == ""} {
+    return
+}
 dap_wait_for_event_and_check "stopped at breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $fn_bpno

gdb/testsuite/gdb.dap/ada-scopes.exp

@@ -25,7 +25,7 @@ if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
     return -1
 }
 
-if {[dap_launch $binfile] == ""} {
+if {[dap_initialize] == ""} {
     return
 }
 
@@ -37,8 +37,11 @@ set obj [dap_check_request_and_response "set breakpoint" \
 		  [list s $srcfile] $line]]
 set fn_bpno [dap_get_breakpoint_number $obj]
 
-dap_check_request_and_response "start inferior" configurationDone
+dap_check_request_and_response "configurationDone" configurationDone
 
+if {[dap_launch $binfile] == ""} {
+    return
+}
 dap_wait_for_event_and_check "stopped at breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $fn_bpno

gdb/testsuite/gdb.dap/args-env.exp

@@ -25,7 +25,7 @@ if {[build_executable ${testfile}.exp $testfile] == -1} {
     return
 }
 
-if {[dap_launch $testfile arguments {a "b c"} env {{DEI something}}] == ""} {
+if {[dap_initialize] == ""} {
     return
 }
 
@@ -36,7 +36,11 @@ set obj [dap_check_request_and_response "set breakpoint by line number" \
 		  [list s $srcfile] $line]]
 set line_bpno [dap_get_breakpoint_number $obj]
 
-dap_check_request_and_response "start inferior" configurationDone
+dap_check_request_and_response "configurationDone" configurationDone
+
+if {[dap_launch $testfile arguments {a "b c"} env {{DEI something}}] == ""} {
+    return
+}
 dap_wait_for_event_and_check "inferior started" thread "body reason" started
 
 dap_wait_for_event_and_check "stopped at line breakpoint" stopped \

gdb/testsuite/gdb.dap/assign.exp

@@ -31,7 +31,7 @@ set remote_python_file [gdb_remote_download host \
 save_vars GDBFLAGS {
     append GDBFLAGS " -iex \"source $remote_python_file\""
 
-    if {[dap_launch $testfile] == ""} {
+    if {[dap_initialize] == ""} {
 	return
     }
 }
@@ -43,8 +43,11 @@ set obj [dap_check_request_and_response "set breakpoint by line number" \
 		  [list s $srcfile] $line]]
 set line_bpno [dap_get_breakpoint_number $obj]
 
-dap_check_request_and_response "start inferior" configurationDone
+dap_check_request_and_response "configurationDone" configurationDone
 
+if {[dap_launch $testfile] == ""} {
+    return
+}
 dap_wait_for_event_and_check "stopped at line breakpoint" stopped \
     "body reason" breakpoint \
     "body hitBreakpointIds" $line_bpno