[PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints

[Simon Farre]

v2.

Added tests.

Decided to not encode frameId forwarding to setDataBreakpoint from
dataBreakpointInfo. I think this is fine though.

The requests resolve a name + variableReference, so that GDB can
build an expression from it, (for instance bar, in variableReference N,
which is foo, becomes foo.bar).

If frameId is passed in, we do not look for global/static global
expressions. If neither variableReference or frameId is passed in,
NAME must be the "full expression" for gdb to understand it. This will
always be the case, passing bar in above example, without a varRef to
construct/resolve the full expression, would be nonsensical (unless
there's an actual expression bar somewhere).

Tests test the dataBreakpointInfo request in different variations as
well as the setDatabreakpoints.

v1.

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      | 179 ++++++++++++++++++++-
 gdb/python/lib/gdb/dap/events.py          |   7 +-
 gdb/python/lib/gdb/dap/frames.py          |  21 ++-
 gdb/python/lib/gdb/dap/varref.py          |  17 +-
 gdb/testsuite/gdb.dap/data-breakpoint.cc  |  38 +++++
 gdb/testsuite/gdb.dap/data-breakpoint.exp | 187 ++++++++++++++++++++++
 6 files changed, 440 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dap/data-breakpoint.cc
 create mode 100644 gdb/testsuite/gdb.dap/data-breakpoint.exp

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index 20e65aa0e61..3eda9eeba56 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -17,11 +17,52 @@ 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, ScopedFrameOperation
+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, expr, type, condition=None, hit_condition=None, frameId = None):
+        """DATA_PATH is the expression of a watchpoint, e.g. foo.bar.baz
+           TYPE describes watchpoint type
+           CONDITION for watchpoint
+           HIT_CONDITION is an expression evaluated to decide how many times
+           the watchpoint should be ignored."""
+        self.expr = expr
+        self.type = type
+        self.condition = condition
+        self.hitCondition = hit_condition
+        # gdb.BP_HARDWARE_WATCHPOINT seem to consistently fail
+        self.gdb_wp = gdb.Breakpoint(spec = expr,
+                                     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 = hit_condition
+
+    def to_object(self):
+        return {
+            "id": self.gdb_wp.number,
+            "verified": True,
+        }
 
+    def is_same(self, expr, type, condition = None, hitCondition = None):
+        return (self.expr == expr
+                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 +71,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 and len(bp.locations) > 0:
         # 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 its 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 = {
@@ -56,6 +101,11 @@ def breakpoint_descriptor(bp):
         if path is not None:
             result["source"]["path"] = path
         return result
+    elif bp.expression is not None:
+        return {
+            "id": bp.number,
+            "verified": True,
+        }
     else:
         return {
             "id": bp.number,
@@ -248,3 +298,126 @@ def set_exception_breakpoints(
     return {
         "breakpoints": result,
     }
+
+@in_gdb_thread
+def _databreakpoint_info(
+    name: str,
+    variablesReference: Optional[int] = None,
+    frameId: Optional[int] = None
+):
+    path = []
+    path.append(name)
+    if variablesReference is not None:
+        # Resolve variable expression 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))
+
+    # Verify that symbol is in that frame
+    if frameId is not None:
+        symbol_name = path[-1]
+        with ScopedFrameOperation(frameId):
+            (symbol, implicit_this) = gdb.lookup_symbol(symbol_name)
+            if symbol is None:
+                dataId = None
+                description = f"Symbol {name} not found in this frame"
+
+    description = dataId
+
+    try:
+        v = gdb.parse_and_eval(dataId)
+    except:
+        description = f"Symbol {dataId} ({name}) was not found"
+        dataId = None
+
+    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 = []
+    # We can't remove during iteration, store to-be removed here
+    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.expr)
+            new_wps_to_add.append(req_wp)
+
+    # Delete wps that are not found in BREAKPOINTS
+    for wp in watchpoint_map.values():
+        if wp.expr not in set([x["dataId"] for x in breakpoints]):
+            # tells gdb to remove wp
+            remove_wps.append(wp.expr)
+
+    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(expr=req_wp["dataId"], type=req_wp["accessType"],
+                            condition=req_wp["condition"],
+                            hit_condition=req_wp["hitCondition"])
+        watchpoint_map[wp.expr] = 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 wp.get("condition") is None:
+            wp["condition"] = None
+        if wp.get("hitCondition") is None:
+            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/events.py b/gdb/python/lib/gdb/dap/events.py
index d9ae603dfa4..f09b7b9df29 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -101,10 +101,12 @@ class StopKinds(enum.Enum):
     # The values here are chosen to follow the DAP spec.
     STEP = "step"
     BREAKPOINT = "breakpoint"
+    DATABREAKPOINT = "data breakpoint"
     PAUSE = "pause"
     EXCEPTION = "exception"
 
 
+
 _expected_stop = None
 
 
@@ -144,7 +146,10 @@ def _on_stop(event):
     }
     if isinstance(event, gdb.BreakpointEvent):
         # Ignore the expected stop, we hit a breakpoint instead.
-        _expected_stop = StopKinds.BREAKPOINT
+        if event.breakpoints[0].expression:
+            _expected_stop = StopKinds.DATABREAKPOINT
+        else:
+            _expected_stop = StopKinds.BREAKPOINT
         obj["hitBreakpointIds"] = [x.number for x in event.breakpoints]
     elif _expected_stop is None:
         # FIXME what is even correct here
diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
index 08209d0b361..eb6e2f1a1a3 100644
--- a/gdb/python/lib/gdb/dap/frames.py
+++ b/gdb/python/lib/gdb/dap/frames.py
@@ -48,7 +48,26 @@ 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]
+
+class ScopedFrameOperation:
+    """Takes FRAME_ID and does a lookup to find matching gdb.Frame
+       and makes this the selected frame, saving the currently selected
+       frame. Upon exit of 'with scope' that frame is again selected."""
+    def __init__(self, frame_id):
+        self.frame_id = frame_id
+        self.restore_frame = gdb.selected_frame()
+
+    def __enter__(self):
+        frame = frame_for_id(self.frame_id)
+        frame.select()
+        return frame
+
+    def __exit__(self, *args):
+        try:
+            self.restore_frame.select()
+        except Exception:
+            gdb.newest_frame().select()
diff --git a/gdb/python/lib/gdb/dap/varref.py b/gdb/python/lib/gdb/dap/varref.py
index 23f18d647c3..23fddb8f192 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,25 +100,33 @@ 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)
+                par = self.ref if isinstance(self, VariableReference) else -1
+                vr = VariableReference(name=name, value=value, p_ref=par)
+                self.children[idx] = vr
             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, p_ref=-1, result_name="value"):
         """Initializer.
 
         NAME is the name of this reference, see superclass.
         VALUE is a gdb.Value that holds the value.
         RESULT_NAME can be used to change how the simple string result
-        is emitted in the result dictionary."""
+        is emitted in the result dictionary.
+        P_REF parent variableReference. Is set to -1 if parent is not
+        of VariableReference type"""
         super().__init__(name)
         self.value = value
         self.printer = gdb.printing.make_visualizer(value)
         self.result_name = result_name
+        self.parent_var_ref = p_ref
         # We cache all the children we create.
         self.child_cache = None
         if not hasattr(self.printer, "children"):
@@ -174,6 +181,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):
diff --git a/gdb/testsuite/gdb.dap/data-breakpoint.cc b/gdb/testsuite/gdb.dap/data-breakpoint.cc
new file mode 100644
index 00000000000..56492e7fb10
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/data-breakpoint.cc
@@ -0,0 +1,38 @@
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+;
+
+struct Var {
+  int val;
+  struct Member {
+    int val;
+  } member;
+};
+
+static Var global{.val = 10, .member = Var::Member{ .val = 20 }};
+
+int
+main (int argc, const char** argv)
+{
+  Var var{.val = 10, .member = Var::Member{ .val = 20 }};
+  int new_val = 30;
+  int member_new_val = 40; /* DATA_BREAKPOINT_TEST */
+  var.val = new_val;
+  var.member.val = member_new_val;
+  return 0; /* AFTER_WRITE */
+}
diff --git a/gdb/testsuite/gdb.dap/data-breakpoint.exp b/gdb/testsuite/gdb.dap/data-breakpoint.exp
new file mode 100644
index 00000000000..fb698e3ce59
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/data-breakpoint.exp
@@ -0,0 +1,187 @@
+# Copyright 2022-2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Basic DAP test.
+
+require allow_dap_tests
+
+load_lib dap-support.exp
+
+standard_testfile .cc
+set exefile $testfile
+
+if {[build_executable "failed to prepare" $exefile $srcfile {debug c++}]} {
+    return
+}
+
+if {[dap_launch $testfile] == ""} {
+    return
+}
+
+set line [gdb_get_line_number "DATA_BREAKPOINT_TEST"]
+set obj [dap_check_request_and_response "set breakpoint by line number" \
+	     setBreakpoints \
+	     [format {o source [o path [%s]] breakpoints [a [o line [i %d]]]} \
+		  [list s $srcfile] $line]]
+set line_bpno [dap_get_breakpoint_number $obj]
+
+dap_check_request_and_response "start inferior" configurationDone
+dap_wait_for_event_and_check "inferior started" thread "body reason" started
+
+dap_wait_for_event_and_check "stopped at line breakpoint" stopped \
+    "body reason" breakpoint \
+    "body hitBreakpointIds" $line_bpno
+
+set bt [lindex [dap_check_request_and_response "backtrace" stackTrace \
+		    {o threadId [i 1]}] \
+	    0]
+set frame_id [dict get [lindex [dict get $bt body stackFrames] 0] id]
+
+set scopes [dap_check_request_and_response "get scopes" scopes \
+		[format {o frameId [i %d]} $frame_id]]
+set scopes [dict get [lindex $scopes 0] body scopes]
+
+lassign $scopes args scope reg_scope
+set num [dict get $scope variablesReference]
+set refs [lindex [dap_check_request_and_response "fetch variables" \
+		      "variables" \
+		      [format {o variablesReference [i %d] count [i 3]} \
+			   $num]] \
+	      0]
+
+foreach var [dict get $refs body variables] {
+    set name [dict get $var name]
+
+    switch $name {
+	"var" {
+	    set var_ref [dict get $var variablesReference]
+	}
+	"new_val" {}
+	"member_new_val" {}
+	default {
+	    fail "unknown variable $name"
+	}
+    }
+}
+
+set refs [lindex [dap_check_request_and_response "fetch contents of var" \
+		      "variables" \
+		      [format {o variablesReference [i %d]} $var_ref]] \
+	      0]
+set var_members [dict get $refs body variables]
+
+set dbi_var_val \
+	[lindex [dap_check_request_and_response \
+		 "dataBreakpointInfo request var ref + name" \
+		 dataBreakpointInfo \
+		 {o variablesReference [i $var_ref] name [s "val"] }] 0]
+
+set dataId [ dict get $dbi_var_val body dataId ]
+gdb_assert { $dataId == "var.val" } "resolved val with varRef $var_ref to var.val"
+
+set refs [lindex [dap_check_request_and_response "fetch contents of var.member" \
+		      "variables" \
+		      [format {o variablesReference [i %d]} $var_ref]] \
+	      0]
+
+foreach var [dict get $refs body variables] {
+    set name [dict get $var name]
+    switch $name {
+	"member" {
+	    set var_member_var_ref [dict get $var variablesReference]
+	}
+	"val" {}
+	default {
+	    fail "unknown variable $name"
+	}
+    }
+}
+
+set dbi_var_member_val [lindex [dap_check_request_and_response "varRef name" \
+				 dataBreakpointInfo {o variablesReference \
+				 [i $var_member_var_ref] name [s "val"] } \
+				] 0 ]
+
+set dataId [ dict get $dbi_var_member_val body dataId ]
+gdb_assert { $dataId == "var.member.val" } \
+	"resolved val with varRef $var_member_var_ref to var.val"
+
+# Same as above but without variablesReference and the full resolve symbol path
+set dbi_var_member_val [lindex [dap_check_request_and_response "name only" \
+		     dataBreakpointInfo {o name [s "var.member.val"] }] 0]
+
+set dataId [ dict get $dbi_var_member_val body dataId ]
+gdb_assert { $dataId == "var.member.val" } \
+	"dataBreakpointInfo without variablesReference for $dataId ok"
+
+set rw "readWrite"
+set wp_response [lindex [dap_check_request_and_response "setDataBreakpoints" \
+		 	setDataBreakpoints \
+			{o breakpoints \
+			[a [o dataId [s $dataId] accessType [s "$rw" ] ] ] \
+			}] \
+		0 ]
+
+set wp_success [ dict get $wp_response success ]
+set bp [lindex [ dict get $wp_response body breakpoints ] 0]
+set bp_verified [ dict get $bp verified ]
+gdb_assert { $wp_success == "true" } "setDataBreakpoints request success"
+gdb_assert { $bp_verified == "true" } "dataBreakpoint verified"
+
+# dataBreakpointInfo for global.member.val => dataId == None
+set dbi_global_member_val \
+	[lindex [dap_check_request_and_response \
+		 "dataBreakpointInfo dataId == None" \
+		 dataBreakpointInfo \
+		 {o frameId [i $frame_id ] name [s "global.member.val"] } \
+		 ] \
+	0 ]
+
+set null_dataId [ dict get $dbi_global_member_val body dataId ]
+gdb_assert { $null_dataId == "null" } \
+	"global.member.val with frameId should return dataId == None"
+
+# dataBreakpointInfo for global.member.val without varRef|frameId
+set dbi_global_member_val \
+	[lindex [dap_check_request_and_response \
+			"dataBreakpointInfo request" \
+			dataBreakpointInfo \
+			{o name [s "global.member.val"] } \
+		] \
+	0]
+
+set dataId [ dict get $dbi_global_member_val body dataId ]
+gdb_assert { $dataId == "global.member.val" } \
+	"dataBreakpointInfo without var ref and frame id should return global"
+
+set after_write [gdb_get_line_number "AFTER_WRITE"]
+
+dap_check_request_and_response "continue until watchpoint triggers" continue \
+    {o threadId [i 1]}
+
+dap_wait_for_event_and_check "stopped at watchpoint" stopped \
+    "body reason" "data breakpoint" \
+    "body hitBreakpointIds" 2
+
+set bt [lindex [dap_check_request_and_response "stopped at line" stackTrace \
+		    {o threadId [i 1]}] \
+	    0]
+
+set stopped_line [dict get [lindex [dict get $bt body stackFrames] 0] line]
+
+gdb_assert { $stopped_line == $after_write } \
+	"Watchpoint stopped at line $stopped_line"
+
+dap_shutdown
-- 
2.40.1

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

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> v2.
Simon> Added tests.

Thanks for the patch.

You probably should rebase this one on top of

https://sourceware.org/pipermail/gdb-patches/2023-June/200252.html

since that changes the breakpoint creation & reuse logic.  I suspect it
would simplify your patch.

Simon> +from .startup import send_gdb_with_response, in_gdb_thread, log
Simon> +from .varref import find_variable
Simon> +from .frames import frame_for_id, ScopedFrameOperation
Simon> +class DataBreakpoint:

Make sure to run 'black', I suspect it wants blank lines in here.
I just do:

murgatroyd. black gdb/python/lib/gdb/


Simon> +    def __init__(self, expr, type, condition=None, hit_condition=None, frameId = None):

With the aforementioned series, this would probably be replaced by one
of the transformer functions, which would then be type-checked.

Simon> +watchpoint_map: Mapping[str, DataBreakpoint] = {}

This can/should be unified with the existing breakpoint_map somehow.

Simon> +        # FIXME this does not matter; GDB can translate its understanding
Simon> +        # of breakpoint locations into individual breakpoints and
Simon> +        # even though GDB can't delete locs, it can disable them.

I think it would be great to discuss this but it seems unrelated to your
patch.  Maybe we should either discuss on the github issue or open a new
gdb 'dap' bug for it?

Simon> +    # We've resolved the "path" for the variable using varrefs.  Construct it
Simon> +    # for example: baz being a member variable of bar, which is a member of
Simon> +    # foo: foo.bar.baz
Simon> +    dataId = ".".join(reversed(path))

Yeah, I'm not totally sure how this part should work.

The issue I see is that variable references use python pretty-printers
under the hood, but these work in a funny/limited way -- they were
intended just for printing -- and so maybe won't interact well with
watchpoints.  For one thing, nothing requires that the names returned by
a printer be meaningful.  That is, they don't necessarily correspond to
expressions that can actually access the member, and in practice do not
(i.e., many C++ printers use readable labels but not usable ones, if
that makes sense).

I had two ideas for how to handle this situation.

One is to keep using pretty printers, and just iterate over the children
looking for one that matches the given name.  Then, if that printer
returns a gdb.Value for the child, watch that value.  This would end up
working like "watch -location".  If 'children' returns a string or
whatever, it would just be unwatchable.

The other idea is to invert the pretty-printer relationship.  Right now
DAP will use a no-op printer for printer-less values.  Instead, DAP
could make its own objects for printer-less values and have a wrapper
for pretty-printers.

Then watching ordinary values could work by remembering the expression
and block, and watching pretty-printed values could simply fail (or
could fall back to the gdb.Value idea above).

The reason to consider this second plan is that watching a Value might
require extra work or will probably just do the wrong thing for local
variables.  (Though I am not sure watchpoints work well with DWARF
location lists anyway.)

Simon> +    # Verify that symbol is in that frame
Simon> +    if frameId is not None:
Simon> +        symbol_name = path[-1]
Simon> +        with ScopedFrameOperation(frameId):
Simon> +            (symbol, implicit_this) = gdb.lookup_symbol(symbol_name)
Simon> +            if symbol is None:
Simon> +                dataId = None
Simon> +                description = f"Symbol {name} not found in this frame"

My understanding of the spec is that there are two cases:

1. variable reference supplied -> frame ID is ignored

2. no variable reference supplied -> the type of evaluation depends on
   the presence of the frame ID

So there's no need for this verification, I think.

However, gdb doesn't currently provide a way to implement #2, as
watchpoint expression evaluation doesn't have a way to pass
PARSER_LEAVE_BLOCK_ALONE to the parser.  This could be added, it's
probably a bit of a pain but mostly mechanical work.

Tom

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

[Simon Farre]

> You probably should rebase this one on top of
>
> https://sourceware.org/pipermail/gdb-patches/2023-June/200252.html
>
> since that changes the breakpoint creation & reuse logic.  I suspect it
> would simplify your patch.
>
> Simon> +from .startup import send_gdb_with_response, in_gdb_thread, log
> Simon> +from .varref import find_variable
> Simon> +from .frames import frame_for_id, ScopedFrameOperation
> Simon> +class DataBreakpoint:
>
> Make sure to run 'black', I suspect it wants blank lines in here.
> I just do:
>
> murgatroyd. black gdb/python/lib/gdb/
I'll get it done in v3, I saw some minor documentation errors i had left 
behind too.
> Simon> +    def __init__(self, expr, type, condition=None, hit_condition=None, frameId = None):
>
> With the aforementioned series, this would probably be replaced by one
> of the transformer functions, which would then be type-checked.
I'll kill the comments about breakpoints being translated -  I wanted to 
be able to enable/disable individual breakpoint locations created by 
GDB, but that perhaps might not be the goal/scope of the 
DAP-interpreters initial feature? Besides, speaking strictly from a 
VSCode perspective here; one could always disable individual breakpoint 
locations using their "debug console"/evaluateRequest (which would be 
the equivalent of typing `disable break 1.1`), even if they don't show 
up in the UI. After having fiddled around with the breakpoint code, it'd 
require some potential hackery and I wasn't at all pleased with my 
intial attempt, so I'll remove the comments.

I'll re-write the watchpoint creation so that it can fit in the current 
design.
> Simon> +    # We've resolved the "path" for the variable using varrefs.  Construct it
> Simon> +    # for example: baz being a member variable of bar, which is a member of
> Simon> +    # foo: foo.bar.baz
> Simon> +    dataId = ".".join(reversed(path))
>
> Yeah, I'm not totally sure how this part should work.
>
> The issue I see is that variable references use python pretty-printers
> under the hood, but these work in a funny/limited way -- they were
> intended just for printing -- and so maybe won't interact well with
> watchpoints.  For one thing, nothing requires that the names returned by
> a printer be meaningful.  That is, they don't necessarily correspond to
> expressions that can actually access the member, and in practice do not
> (i.e., many C++ printers use readable labels but not usable ones, if
> that makes sense).
>
> I had two ideas for how to handle this situation.
>
> One is to keep using pretty printers, and just iterate over the children
> looking for one that matches the given name.  Then, if that printer
> returns a gdb.Value for the child, watch that value.  This would end up
> working like "watch -location".  If 'children' returns a string or
> whatever, it would just be unwatchable.
After having read this the first time I had no idea what you meant, but 
I (finally) figured out what you meant and where this approach would be 
greatly useful; for instance, if we had a `std::vector<Foo> foo` , we 
can now set `watch -location foo[10]` because we would be able to 
resolve element 10 and it's address, right?; using my approach it would 
attempt to watch `foo.[10]` (or maybe foo.10) which is nonsensical. So 
great idea, I'll try get something that'll work with pretty printers 
that return `gdb.Value`. I'll also add new tests for this as well.

I'll address the remainder of your review after I've addressed these 
points first.

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

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Sorry I didn't reply to this one earlier.

Simon> I'll kill the comments about breakpoints being translated -  I wanted
Simon> to be able to enable/disable individual breakpoint locations created
Simon> by GDB, but that perhaps might not be the goal/scope of the
Simon> DAP-interpreters initial feature?

Unfortunately DAP doesn't really admit the possibility that a breakpoint
has multiple locations.  This is here:

https://github.com/microsoft/debug-adapter-protocol/issues/114

... but it hasn't seen any action in a while.

Tom