public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints
@ 2023-06-15 18:28 Simon Farre
  2023-06-16 18:14 ` Tom Tromey
  0 siblings, 1 reply; 4+ messages in thread
From: Simon Farre @ 2023-06-15 18:28 UTC (permalink / raw)
  To: gdb-patches; +Cc: 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


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-12 17:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 18:28 [PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints Simon Farre
2023-06-16 18:14 ` Tom Tromey
2023-06-17 22:47   ` Simon Farre
2023-09-12 17:08     ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).