[PATCH 3/7] Handle exceptions when creating DAP breakpoints

[Tom Tromey <tromey@adacore.com>]

When creating a DAP breakpoint, a failure should be returned by
setting "verified" to False.  gdb didn't properly implement this, and
there was a FIXME comment to this effect.  This patch fixes the
problem.
---
 gdb/python/lib/gdb/dap/breakpoint.py      | 89 +++++++++++++++++++------------
 gdb/testsuite/gdb.dap/catch-exception.exp | 24 ++++++---
 gdb/testsuite/gdb.dap/cond-bp.exp         | 19 +++++++
 3 files changed, 92 insertions(+), 40 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index f15f905c5f3..3f40cfb7f63 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -20,7 +20,8 @@ import os
 from typing import Optional, Sequence
 
 from .server import request, capability
-from .startup import send_gdb_with_response, in_gdb_thread
+from .startup import send_gdb_with_response, in_gdb_thread, log_stack
+from .typecheck import type_check
 
 
 # Map from the breakpoint "kind" (like "function") to a second map, of
@@ -34,33 +35,35 @@ breakpoint_map = {}
 @in_gdb_thread
 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,
+    }
     if bp.locations:
         # Just choose the first location, because DAP doesn't allow
         # multiple locations.  See
         # https://github.com/microsoft/debug-adapter-protocol/issues/13
         loc = bp.locations[0]
         (basename, line) = loc.source
-        result = {
-            "id": bp.number,
-            "verified": True,
-            "source": {
-                "name": os.path.basename(basename),
-                # We probably don't need this but it doesn't hurt to
-                # be explicit.
-                "sourceReference": 0,
-            },
-            "line": line,
-            "instructionReference": hex(loc.address),
-        }
+        result.update(
+            {
+                "source": {
+                    "name": os.path.basename(basename),
+                    # We probably don't need this but it doesn't hurt to
+                    # be explicit.
+                    "sourceReference": 0,
+                },
+                "line": line,
+                "instructionReference": hex(loc.address),
+            }
+        )
         path = loc.fullname
         if path is not None:
             result["source"]["path"] = path
-        return result
-    else:
-        return {
-            "id": bp.number,
-            "verified": False,
-        }
+    return result
 
 
 # Extract entries from a hash table and return a list of them.  Each
@@ -91,22 +94,42 @@ def _set_breakpoints_callback(kind, specs, creator):
         (condition, hit_condition) = _remove_entries(spec, "condition", "hitCondition")
         keyspec = frozenset(spec.items())
 
-        if keyspec in saved_map:
-            bp = saved_map.pop(keyspec)
-        else:
-            # FIXME handle exceptions here
-            bp = creator(**spec)
-
-        bp.condition = condition
-        if hit_condition is None:
-            bp.ignore_count = 0
-        else:
-            bp.ignore_count = int(
-                gdb.parse_and_eval(hit_condition, global_context=True)
+        # 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(
+                    gdb.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:
+            log_stack()
+            # 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),
+                }
             )
 
-        breakpoint_map[kind][keyspec] = bp
-        result.append(breakpoint_descriptor(bp))
     # Delete any breakpoints that were not reused.
     for entry in saved_map.values():
         entry.delete()
diff --git a/gdb/testsuite/gdb.dap/catch-exception.exp b/gdb/testsuite/gdb.dap/catch-exception.exp
index 7f2e750b32e..95e65556cc6 100644
--- a/gdb/testsuite/gdb.dap/catch-exception.exp
+++ b/gdb/testsuite/gdb.dap/catch-exception.exp
@@ -31,22 +31,32 @@ if {[dap_launch $binfile] == ""} {
 
 set obj [dap_check_request_and_response "set exception catchpoints" \
 	     setExceptionBreakpoints \
-	     {o filters [a [s assert]] \
+	     {o filters [a [s nosuchfilter] [s assert]] \
 		  filterOptions [a [o filterId [s exception] \
 					condition [s "Global_Var = 23"]]]}]
 
 set bps [dict get [lindex $obj 0] body breakpoints]
-gdb_assert {[llength $bps] == 2} "two breakpoints"
+# We should get three responses, because we requested three
+# breakpoints.  However, one of them should fail.
+gdb_assert {[llength $bps] == 3} "three breakpoints"
 
 # The "path" should never be "null".
 set i 1
 foreach spec $bps {
-    # If "path" does not exist, then that is fine as well.
-    if {![dict exists $spec source path]} {
-	pass "breakpoint $i path"
+    if {$i == 1} {
+	# First one should fail.
+	gdb_assert {[dict get $spec verified] == "false"} \
+	    "invalid first exception"
+	gdb_assert {[dict get $spec message] != ""} \
+	    "first exception had message"
     } else {
-	gdb_assert {[dict get $spec source path] != "null"} \
-	    "breakpoint $i path"
+	# If "path" does not exist, then that is fine as well.
+	if {![dict exists $spec source path]} {
+	    pass "breakpoint $i path"
+	} else {
+	    gdb_assert {[dict get $spec source path] != "null"} \
+		"breakpoint $i path"
+	}
     }
     incr i
 }
diff --git a/gdb/testsuite/gdb.dap/cond-bp.exp b/gdb/testsuite/gdb.dap/cond-bp.exp
index 6369b6f579c..262ab9d26c8 100644
--- a/gdb/testsuite/gdb.dap/cond-bp.exp
+++ b/gdb/testsuite/gdb.dap/cond-bp.exp
@@ -30,6 +30,25 @@ if {[dap_launch $testfile] == ""} {
 }
 
 set line [gdb_get_line_number "STOP"]
+
+# Test some breakpoint-setting failure modes.
+set obj [dap_check_request_and_response "set invalid breakpoints" \
+	     setBreakpoints \
+	     [format {o source [o path [%s]] \
+			  breakpoints \
+			  [a [o line [i %d] condition [s "DEI@@++"]] \
+			       [o line [i %d] hitCondition [s "DEI@@++"]]]} \
+		  [list s $srcfile] $line $line]]
+
+set i 1
+foreach bp [dict get [lindex $obj 0] body breakpoints] {
+    gdb_assert {[dict get $bp verified] == "false"} \
+	"breakpoint $i invalid"
+    gdb_assert {[dict get $bp message] != ""} \
+	"breakpoint $i has message"
+    incr i
+}
+
 set obj [dap_check_request_and_response "set conditional breakpoint" \
 	     setBreakpoints \
 	     [format {o source [o path [%s]] \

-- 
2.40.1