[PATCH v1] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[PATCH v1] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Simon Farre]

This is v1 of the implementation of these two requests.

I've tested the implementation in my debug adapter for VSCode (Midas)
and it works fully.

However, I believe these two requests are flawed from a spec-perspective
and has filed an issue for the DAP spec here: https://github.com/microsoft/debug-adapter-protocol/issues/404
As such, it implements *most* of the functionality of these two
requests and as the github issues suggests, I believe implementing all
of it is not possible with the current spec design (also, VSCode does not
use the frameId field, in dataBreakpointInfo request).
---
 gdb/python/lib/gdb/dap/breakpoint.py | 160 ++++++++++++++++++++++++++-
 gdb/python/lib/gdb/dap/frames.py     |   2 +-
 gdb/python/lib/gdb/dap/varref.py     |  12 +-
 3 files changed, 166 insertions(+), 8 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 20e65aa0e61..2d5bf7d0ee4 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -17,11 +17,42 @@ import gdb
 import os
 
 # These are deprecated in 3.9, but required in older versions.
-from typing import Optional, Sequence
+from typing import Optional, Sequence, Mapping
 
 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
+from .varref import find_variable
+from .frames import frame_for_id
+class DataBreakpoint:
+    def wp_type(type: str):
+        if type == "read":
+            return gdb.WP_READ
+        if type == "write":
+            return gdb.WP_WRITE
+        if type == "readWrite":
+            return gdb.WP_ACCESS
+        raise gdb.GdbError("Erroneous watchpoint type")
+
+    def __init__(self, dataId, type, condition=None, hitCondition=None):
+        self.dataId = dataId
+        self.type = type
+        self.condition = condition
+        self.hitCondition = hitCondition
+        # gdb.BP_HARDWARE_WATCHPOINT seem to consistently fail
+        self.gdb_wp = gdb.Breakpoint(spec=dataId, type=gdb.BP_WATCHPOINT, wp_class=DataBreakpoint.wp_type(type))
+        if self.condition is not None:
+            self.gdb_wp.condition = condition
+        if self.hitCondition is not None:
+            self.gdb_wp.ignore_count = hitCondition
+
+    def to_object(self):
+        return {
+            "id": self.gdb_wp.number,
+            "verified": True,
+        }
 
+    def is_same(self, dataId, type, condition=None, hitCondition=None):
+        return self.dataId == dataId and self.type == type and self.condition == condition and self.hitCondition == hitCondition
 
 # Map from the breakpoint "kind" (like "function") to a second map, of
 # breakpoints of that type.  The second map uses the breakpoint spec
@@ -30,14 +61,18 @@ from .startup import send_gdb_with_response, in_gdb_thread
 # allowing for reuse when a breakpoint can be kept.
 breakpoint_map = {}
 
+watchpoint_map: Mapping[str, DataBreakpoint] = {}
 
 @in_gdb_thread
 def breakpoint_descriptor(bp):
     "Return the Breakpoint object descriptor given a gdb Breakpoint."
-    if bp.locations:
+    if bp.location is not None:
         # Just choose the first location, because DAP doesn't allow
         # multiple locations.  See
         # https://github.com/microsoft/debug-adapter-protocol/issues/13
+        # FIXME this does not matter; GDB can translate it's understanding
+        # of breakpoint locations into individual breakpoints and
+        # even though GDB can't delete locs, it can disable them.
         loc = bp.locations[0]
         (basename, line) = loc.source
         result = {
@@ -59,7 +94,7 @@ def breakpoint_descriptor(bp):
     else:
         return {
             "id": bp.number,
-            "verified": False,
+            "verified": True,
         }
 
 
@@ -248,3 +283,120 @@ def set_exception_breakpoints(
     return {
         "breakpoints": result,
     }
+
+@in_gdb_thread
+def _databreakpoint_info(
+    name: str,
+    variablesReference: Optional[int] = None,
+    frameId: Optional[int] = None
+):
+    # frameId does not have an effect when var ref is not none,
+    # however, frameId really never has an effect. An issue has been filed:
+    # https://github.com/microsoft/debug-adapter-protocol/issues/404
+    path = []
+    path.append(name)
+    if variablesReference is not None:
+        # append "path" to name until find_variable throws
+        try:
+            variable = find_variable(variablesReference)
+            while variable is not None and variable.is_variable():
+                path.append(variable.name)
+                parent_ref = variable.parent_var_ref
+                variable = find_variable(parent_ref)
+        except:
+            pass
+
+    # We've resolved the "path" for the variable using varrefs. Construct it
+    # for example: baz being a member variable of bar, which is a member of
+    # foo: foo.bar.baz
+    dataId = ".".join(reversed(path))
+    description = dataId
+
+    try:
+        # since frameId can't be used, always parse global (see issue)
+        v = gdb.parse_and_eval(dataId)
+    except:
+        dataId = None
+        description = "Symbol was not found"
+
+    return {
+        "dataId": dataId,
+        "description": description,
+        "accessTypes": ["read", "write", "readWrite"],
+    }
+
+@capability("supportsDataBreakpoints")
+@request("dataBreakpointInfo")
+def data_breakpoint_info(
+    *,
+    name: str,
+    variablesReference: Optional[int] = None,
+    frameId: Optional[int] = None, **args
+):
+    result = send_gdb_with_response(
+        lambda: _databreakpoint_info(name, variablesReference, frameId))
+    return result
+
+
+@in_gdb_thread
+def _set_watchpoint(breakpoints: Sequence[dict]):
+    """Set (or keep) watchpoints passed in breakpoints and remove set
+        watchpoints not found in breakpoints"""
+    global watchpoint_map
+    if len(breakpoints) == 0:
+        for wp in watchpoint_map:
+            wp.gdb_wp.delete()
+        watchpoint_map.clear()
+        return []
+
+    new_wps_to_add = []
+    # Because we can't remove during iteration.
+    remove_wps = []
+    for req_wp in breakpoints:
+        wp = watchpoint_map.get(req_wp["dataId"])
+        if wp is None:
+            new_wps_to_add.append(req_wp)
+        # If wp with dataId exist; but something about it has changed
+        # (another condition for instance), delete it, so it can be set
+        # new with the new spec.
+        elif not wp.is_same(req_wp["dataId"], req_wp["accessType"],
+                            req_wp["condition"], req_wp["hitCondition"]):
+            remove_wps.append(wp.dataId)
+            new_wps_to_add.append(req_wp)
+    
+    # Delete wps that are not found in `breakpoints` request
+    for wp in watchpoint_map.values():
+        if wp.dataId not in set([x["dataId"] for x in breakpoints]):
+            # tells gdb to remove wp
+            remove_wps.append(wp.dataId)
+
+    for remove_wp in remove_wps:
+        watchpoint_map[remove_wp].gdb_wp.delete()
+        del watchpoint_map[remove_wp]
+
+    # Set the new watchpoints
+    for req_wp in new_wps_to_add:
+        wp = DataBreakpoint(dataId=req_wp["dataId"], type=req_wp["accessType"],
+                            condition=req_wp["condition"],
+                            hitCondition=req_wp["hitCondition"])
+        watchpoint_map[wp.dataId] = wp
+
+    return [x.to_object() for x in watchpoint_map.values()]
+
+def _sanitize_wp_input(breakpoints: Sequence) -> Sequence:
+    """Force the request arg DataBreakpoints to contain these attributes"""
+    res = []
+    for wp in breakpoints:
+        if not hasattr(wp, "condition"):
+            wp["condition"] = None
+        if not hasattr(wp, "hitCondition"):
+            wp["hitCondition"] = None
+        res.append(wp)
+    return res
+
+@request("setDataBreakpoints")
+@capability("supportsDataBreakpoints")
+def watchpoints(*, breakpoints: Sequence, **args):
+    bps = _sanitize_wp_input(breakpoints=breakpoints)
+    result = send_gdb_with_response(lambda: _set_watchpoint(bps))
+    return { "breakpoints": result }
diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
index 08209d0b361..6102352e2e5 100644
--- a/gdb/python/lib/gdb/dap/frames.py
+++ b/gdb/python/lib/gdb/dap/frames.py
@@ -48,7 +48,7 @@ def frame_id(frame):
 
 
 @in_gdb_thread
-def frame_for_id(id):
+def frame_for_id(id) -> gdb.Frame:
     """Given a frame identifier ID, return the corresponding frame."""
     global _all_frames
     return _all_frames[id]
diff --git a/gdb/python/lib/gdb/dap/varref.py b/gdb/python/lib/gdb/dap/varref.py
index 23f18d647c3..611d30b6a1d 100644
--- a/gdb/python/lib/gdb/dap/varref.py
+++ b/gdb/python/lib/gdb/dap/varref.py
@@ -22,7 +22,6 @@ from abc import abstractmethod
 # A list of all the variable references created during this pause.
 all_variables = []
 
-
 # When the inferior is re-started, we erase all variable references.
 # See the section "Lifetime of Objects References" in the spec.
 @in_gdb_thread
@@ -101,15 +100,19 @@ class BaseReference:
         for idx in range(start, start + count):
             if self.children[idx] is None:
                 (name, value) = self.fetch_one_child(idx)
-                self.children[idx] = VariableReference(name, value)
+                parent_ref = self.ref if isinstance(self, VariableReference) else -1
+                self.children[idx] = VariableReference(name=name, value=value, parent_var_ref=parent_ref)
             result.append(self.children[idx])
         return result
 
+    @abstractmethod
+    def is_variable(self):
+        return False
 
 class VariableReference(BaseReference):
     """Concrete subclass of BaseReference that handles gdb.Value."""
 
-    def __init__(self, name, value, result_name="value"):
+    def __init__(self, name, value, parent_var_ref=-1, result_name="value"):
         """Initializer.
 
         NAME is the name of this reference, see superclass.
@@ -120,6 +123,7 @@ class VariableReference(BaseReference):
         self.value = value
         self.printer = gdb.printing.make_visualizer(value)
         self.result_name = result_name
+        self.parent_var_ref = parent_var_ref
         # We cache all the children we create.
         self.child_cache = None
         if not hasattr(self.printer, "children"):
@@ -174,6 +178,8 @@ class VariableReference(BaseReference):
     def fetch_one_child(self, idx):
         return self.cache_children()[idx]
 
+    def is_variable(self):
+        return True
 
 @in_gdb_thread
 def find_variable(ref):
-- 
2.40.1

Re: [PATCH v1] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Eli Zaretskii]

> Cc: tom@tromey.com,
> 	Simon Farre <simon.farre.cx@gmail.com>
> Date: Wed, 14 Jun 2023 12:33:27 +0200
> From: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> 
> This is v1 of the implementation of these two requests.

I didn't (and cannot) review this in a useful way, but my eye caught a
few nits:

> -    if bp.locations:
> +    if bp.location is not None:
>          # Just choose the first location, because DAP doesn't allow
>          # multiple locations.  See
>          # https://github.com/microsoft/debug-adapter-protocol/issues/13
> +        # FIXME this does not matter; GDB can translate it's understanding
                                                           ^^^^
"its"

> +    # frameId does not have an effect when var ref is not none,
                                                             ^^^^
"None", capitalized?

> +    # however, frameId really never has an effect. An issue has been filed:
                                                    ^^
Two spaces there.

> +    # We've resolved the "path" for the variable using varrefs. Construct it
                                                                 ^^
And there.

> +    # Delete wps that are not found in `breakpoints` request
                                          ^^^^^^^^^^^^^
Are we okay with MD-style markup?

Thanks.

Re: [PATCH v1] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Simon Farre]

> Are we okay with MD-style markup?
> Thanks.

I'll address the nitpicks. Misspelling of "its" every time, never fails :P.

About the MD-style markup, that's a oopsie daisy from me, I don't think 
GDB actually does that, its just something I'm used to doing. It will be 
removed, I think GDB does ALL_CAPS instead, right?

Re: [PATCH v1] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Eli Zaretskii]

> Date: Wed, 14 Jun 2023 15:40:18 +0200
> Cc: gdb-patches@sourceware.org
> From: Simon Farre <simon.farre.cx@gmail.com>
> 
> > Are we okay with MD-style markup?
> > Thanks.
> 
> I'll address the nitpicks. Misspelling of "its" every time, never fails :P.
> 
> About the MD-style markup, that's a oopsie daisy from me, I don't think 
> GDB actually does that, its just something I'm used to doing. It will be 
> removed, I think GDB does ALL_CAPS instead, right?

Yes, I think so.