[PATCH] Implement DAP setVariable request

[PATCH] Implement DAP setVariable request

[Tom Tromey]

This patch implements the DAP setVariable request.

setVariable is a bit odd in that it specifies the variable to modify
by passing in the variable's container and the name of the variable.
This approach can't handle variable shadowing (there are a couple of
open DAP bugs on this topic), so this patch renames duplicates to
avoid the problem.
---
 gdb/python/lib/gdb/dap/evaluate.py |  21 +++++
 gdb/python/lib/gdb/dap/scopes.py   |   3 +
 gdb/python/lib/gdb/dap/varref.py   |  97 ++++++++++++++++++-----
 gdb/testsuite/gdb.dap/assign.c     |  35 +++++++++
 gdb/testsuite/gdb.dap/assign.exp   | 122 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.dap/assign.py    |  54 +++++++++++++
 6 files changed, 311 insertions(+), 21 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dap/assign.c
 create mode 100644 gdb/testsuite/gdb.dap/assign.exp
 create mode 100644 gdb/testsuite/gdb.dap/assign.py

diff --git a/gdb/python/lib/gdb/dap/evaluate.py b/gdb/python/lib/gdb/dap/evaluate.py
index 405e6fc2410..ea5a1e61a08 100644
--- a/gdb/python/lib/gdb/dap/evaluate.py
+++ b/gdb/python/lib/gdb/dap/evaluate.py
@@ -141,3 +141,24 @@ def set_expression(
     return send_gdb_with_response(
         lambda: _set_expression(expression, value, frameId, format)
     )
+
+
+# Helper function to perform an assignment.
+@in_gdb_thread
+def _set_variable(ref, name, value, value_format):
+    with apply_format(value_format):
+        var = find_variable(ref)
+        lhs = var.find_child_by_name(name)
+        rhs = gdb.parse_and_eval(value)
+        lhs.assign(rhs)
+        return lhs.to_object()
+
+
+@capability("supportsSetVariable")
+@request("setVariable")
+def set_variable(
+    *, variablesReference: int, name: str, value: str, format=None, **args
+):
+    return send_gdb_with_response(
+        lambda: _set_variable(variablesReference, name, value, format)
+    )
diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
index 5dde060f44f..87f2ed7547f 100644
--- a/gdb/python/lib/gdb/dap/scopes.py
+++ b/gdb/python/lib/gdb/dap/scopes.py
@@ -59,6 +59,9 @@ class _ScopeReference(BaseReference):
             # FIXME construct a Source object
         return result
 
+    def has_children(self):
+        return True
+
     def child_count(self):
         return len(self.var_list)
 
diff --git a/gdb/python/lib/gdb/dap/varref.py b/gdb/python/lib/gdb/dap/varref.py
index 5dfe6e0edb9..8f0a070498c 100644
--- a/gdb/python/lib/gdb/dap/varref.py
+++ b/gdb/python/lib/gdb/dap/varref.py
@@ -16,7 +16,8 @@
 import gdb
 from .startup import in_gdb_thread
 from .server import client_bool_capability
-from abc import abstractmethod
+from abc import ABC, abstractmethod
+from collections import defaultdict
 from contextlib import contextmanager
 
 
@@ -52,7 +53,7 @@ def apply_format(value_format):
     return _null()
 
 
-class BaseReference:
+class BaseReference(ABC):
     """Represent a variable or a scope.
 
     This class is just a base class, some methods must be implemented in
@@ -72,7 +73,7 @@ class BaseReference:
         all_variables.append(self)
         self.ref = len(all_variables)
         self.name = name
-        self.children = None
+        self.reset_children()
 
     @in_gdb_thread
     def to_object(self):
@@ -80,16 +81,27 @@ class BaseReference:
 
         The resulting object is a starting point that can be filled in
         further.  See the Scope or Variable types in the spec"""
-        result = {
-            "variablesReference": self.ref,
-        }
+        result = {"variablesReference": self.ref if self.has_children() else 0}
         if self.name is not None:
             result["name"] = str(self.name)
         return result
 
-    def no_children(self):
-        """Call this to declare that this variable or scope has no children."""
-        self.ref = 0
+    @abstractmethod
+    def has_children(self):
+        """Return True if this object has children."""
+        return False
+
+    def reset_children(self):
+        """Reset any cached information about the children of this object."""
+        # A list of all the children.  Each child is a BaseReference
+        # of some kind.
+        self.children = None
+        # Map from the name of a child to a BaseReference.
+        self.by_name = {}
+        # Keep track of how many duplicates there are of a given name,
+        # so that unique names can be generated.  Map from base name
+        # to a count.
+        self.name_counts = defaultdict(lambda: 1)
 
     @abstractmethod
     def fetch_one_child(self, index):
@@ -105,23 +117,55 @@ class BaseReference:
         """Return the number of children of this variable."""
         return
 
+    # Helper method to compute the final name for a child whose base
+    # name is given.  Updates the name_counts map.  This is used to
+    # handle shadowing -- in DAP, the adapter is responsible for
+    # making sure that all the variables in a a given container have
+    # unique names.  See
+    # https://github.com/microsoft/debug-adapter-protocol/issues/141
+    # and
+    # https://github.com/microsoft/debug-adapter-protocol/issues/149
+    def _compute_name(self, name):
+        if name in self.by_name:
+            self.name_counts[name] += 1
+            # In theory there's no safe way to compute a name, because
+            # a pretty-printer might already be generating names of
+            # that form.  In practice I think we should not worry too
+            # much.
+            name = name + " #" + str(self.name_counts[name])
+        return name
+
     @in_gdb_thread
     def fetch_children(self, start, count):
         """Fetch children of this variable.
 
         START is the starting index.
-        COUNT is the number to return, with 0 meaning return all."""
+        COUNT is the number to return, with 0 meaning return all.
+        Returns an iterable of some kind."""
         if count == 0:
             count = self.child_count()
         if self.children is None:
             self.children = [None] * self.child_count()
-        result = []
         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)
-            result.append(self.children[idx])
-        return result
+                name = self._compute_name(name)
+                var = VariableReference(name, value)
+                self.children[idx] = var
+                self.by_name[name] = var
+            yield self.children[idx]
+
+    @in_gdb_thread
+    def find_child_by_name(self, name):
+        """Find a child of this variable, given its name.
+
+        Returns the value of the child, or throws if not found."""
+        # A lookup by name can only be done using names previously
+        # provided to the client, so we can simply rely on the by-name
+        # map here.
+        if name in self.by_name:
+            return self.by_name[name]
+        raise Exception("no variable named '" + name + "'")
 
 
 class VariableReference(BaseReference):
@@ -135,16 +179,27 @@ class VariableReference(BaseReference):
         RESULT_NAME can be used to change how the simple string result
         is emitted in the result dictionary."""
         super().__init__(name)
-        self.value = value
-        self.printer = gdb.printing.make_visualizer(value)
         self.result_name = result_name
-        # We cache all the children we create.
+        self.value = value
+        self._update_value()
+
+    # Internal method to update local data when the value changes.
+    def _update_value(self):
+        self.reset_children()
+        self.printer = gdb.printing.make_visualizer(self.value)
         self.child_cache = None
-        if not hasattr(self.printer, "children"):
-            self.no_children()
-            self.count = None
-        else:
+        if self.has_children():
             self.count = -1
+        else:
+            self.count = None
+
+    def assign(self, value):
+        """Assign VALUE to this object and update."""
+        self.value.assign(value)
+        self._update_value()
+
+    def has_children(self):
+        return hasattr(self.printer, "children")
 
     def cache_children(self):
         if self.child_cache is None:
diff --git a/gdb/testsuite/gdb.dap/assign.c b/gdb/testsuite/gdb.dap/assign.c
new file mode 100644
index 00000000000..c3bc3d3f107
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/assign.c
@@ -0,0 +1,35 @@
+/* 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 special_type
+{
+  /* Discriminator used by the pretty-printer.  When zero, no fields
+     are shown; when non-zero, shows the datum.  */
+  int disc;
+  int datum;
+};
+
+struct special_type empty = { 0, 23 };
+struct special_type full = { 1, 23 };
+
+int
+main ()
+{
+  struct special_type value = full;
+
+  return 0;			/* STOP */
+}
diff --git a/gdb/testsuite/gdb.dap/assign.exp b/gdb/testsuite/gdb.dap/assign.exp
new file mode 100644
index 00000000000..e1c412eca5f
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/assign.exp
@@ -0,0 +1,122 @@
+# Copyright 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/>.
+
+# Test the setVariable request.
+
+require allow_dap_tests
+
+load_lib dap-support.exp
+
+standard_testfile
+
+if {[build_executable ${testfile}.exp $testfile] == -1} {
+    return
+}
+
+set remote_python_file [gdb_remote_download host \
+			    ${srcdir}/${subdir}/${testfile}.py]
+
+save_vars GDBFLAGS {
+    append GDBFLAGS " -iex \"source $remote_python_file\""
+
+    if {[dap_launch $testfile] == ""} {
+	return
+    }
+}
+
+set line [gdb_get_line_number "STOP"]
+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 "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 scope reg_scope
+gdb_assert {[dict get $scope name] == "Locals"} "scope is locals"
+gdb_assert {[dict get $scope namedVariables] == 1} "one var in scope"
+
+set num [dict get $scope variablesReference]
+set refs [lindex [dap_check_request_and_response "fetch variable" \
+		      "variables" \
+		      [format {o variablesReference [i %d] count [i 1]} \
+			   $num]] \
+	      0]
+
+set desc [dict get $refs body variables]
+gdb_assert {[llength $desc] == 1} "only one variable fetched"
+set desc [lindex $desc 0]
+
+set saved_ref [dict get $desc variablesReference]
+
+proc check_vars {prefix var varref summary} {
+    with_test_prefix $prefix {
+	gdb_assert {[dict get $var name] == "value"} "variable name"
+	gdb_assert {[dict get $var variablesReference] == $varref} \
+	    "variable reference"
+	gdb_assert {[dict get $var value] == $summary} \
+	    "variable value"
+    }
+}
+
+check_vars initial $desc $saved_ref full
+
+set refs [lindex [dap_check_request_and_response "assign empty to variable" \
+		      "setVariable" \
+		      [format {o variablesReference [i %d] name [s value] \
+				   value [s empty]} \
+			   $num]] \
+	      0]
+check_vars "assign empty" [dict get $refs body] 0 empty
+
+set refs [lindex [dap_check_request_and_response "assign full to variable" \
+		      "setVariable" \
+		      [format {o variablesReference [i %d] name [s value] \
+				   value [s full]} \
+			   $num]] \
+	      0]
+check_vars "assign full" [dict get $refs body] $saved_ref full
+
+# Now fetch the children of the variable, to check that the shadowing
+# workaround works.
+gdb_assert {[dict get $refs body namedVariables] == 2} \
+    "two children of variable"
+
+set num [dict get $refs body variablesReference]
+set refs [lindex [dap_check_request_and_response \
+		      "fetch children of variable" "variables" \
+		      [format {o variablesReference [i %d] count [i 2]} \
+			   $num]] \
+	      0]
+
+lassign [dict get $refs body variables] one two
+gdb_assert {[dict get $one name] != [dict get $two name]} \
+    "children have different names"
+
+dap_shutdown
diff --git a/gdb/testsuite/gdb.dap/assign.py b/gdb/testsuite/gdb.dap/assign.py
new file mode 100644
index 00000000000..d7b4868455d
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/assign.py
@@ -0,0 +1,54 @@
+# Copyright (C) 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/>.
+
+
+import gdb
+
+
+class EmptyPrinter(gdb.ValuePrinter):
+    """Pretty print a structure"""
+
+    def __init__(self, val):
+        self._val = val
+
+    def to_string(self):
+        return "empty"
+
+
+class FullPrinter(gdb.ValuePrinter):
+    """Pretty print a structure"""
+
+    def __init__(self, val):
+        self._val = val
+
+    def to_string(self):
+        return "full"
+
+    def children(self):
+        # This is used to test the renaming code.
+        yield "datum", self._val["datum"]
+        yield "datum", self._val["datum"]
+
+
+def lookup_function(val):
+    if val.type.tag == "special_type":
+        if val["disc"] == 0:
+            return EmptyPrinter(val)
+        else:
+            return FullPrinter(val)
+    return None
+
+
+gdb.pretty_printers.append(lookup_function)
-- 
2.40.1

Re: [PATCH] Implement DAP setVariable request

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This patch implements the DAP setVariable request.
Tom> setVariable is a bit odd in that it specifies the variable to modify
Tom> by passing in the variable's container and the name of the variable.
Tom> This approach can't handle variable shadowing (there are a couple of
Tom> open DAP bugs on this topic), so this patch renames duplicates to
Tom> avoid the problem.

I'm checking this in now, and since it is DAP-specific, I am also going
to apply it to the GDB 14 branch.

Tom