[PATCH 0/3] Return a global scope from DAP scopes request

[PATCH 0/3] Return a global scope from DAP scopes request

[Tom Tromey]

A co-worker asked that some global variables (in his case,
specifically package-scoped variables in Ada) be visible in the
response to the DAP "scopes" request.

This series implements this.  I wasn't completely sure what to call
the resulting scope.  I chose "globals" but really the scope here is
"globals local to the frame's CU".

---
Tom Tromey (3):
      Memoize gdb.Block and make them hashable
      Convert DAP disassemble code to use Block hashing
      Return global scope from DAP scopes request

 gdb/NEWS                              |   3 +
 gdb/data-directory/Makefile.in        |   1 +
 gdb/python/lib/gdb/dap/disassemble.py |   7 +-
 gdb/python/lib/gdb/dap/globalvars.py  |  97 +++++++++++++++++++++++
 gdb/python/lib/gdb/dap/scopes.py      |   4 +
 gdb/python/py-block.c                 | 143 +++++++++++++++++++---------------
 gdb/testsuite/gdb.dap/ptrref.exp      |   8 +-
 gdb/testsuite/gdb.dap/rust-slices.exp |   4 +-
 gdb/testsuite/gdb.python/py-block.exp |   4 +
 9 files changed, 199 insertions(+), 72 deletions(-)
---
base-commit: 89457440e4a4b71b1fe60ed3e034c814bf8eb6b7
change-id: 20240516-dap-global-scope-697eb53a080a

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/3] Memoize gdb.Block and make them hashable

[Tom Tromey]

In subsequent patches, it's handy if gdb.Block is hashable, so it can
be stored in a set or a dictionary.  However, doing this in a
straightforward way is not really possible, because a block isn't
truly immutable -- it can be invalidated.  And, while this isn't a
real problem for my use case (in DAP the maps are only used during a
single stop), it seemed error-prone.

This patch instead takes the approach of using the gdb.Block's own
object identity to allow hashing.  This seems fine because the
contents don't affect the hashing.  In order for this to work, though,
the blocks have to be memoized -- two requests for the same block must
return the same object.

This also allows (actually, requires) the simplification of the
rich-compare method for blocks.
---
 gdb/python/py-block.c                 | 143 +++++++++++++++++++---------------
 gdb/testsuite/gdb.python/py-block.exp |   4 +
 2 files changed, 83 insertions(+), 64 deletions(-)

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 3de6200e7c2..62e93d55072 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -31,10 +31,6 @@ struct block_object {
      between a block and an object file.  When a block is created also
      store a pointer to the object file for later use.  */
   struct objfile *objfile;
-  /* Keep track of all blocks with a doubly-linked list.  Needed for
-     block invalidation if the source object file has been freed.  */
-  block_object *prev;
-  block_object *next;
 };
 
 struct block_syms_iterator_object {
@@ -76,32 +72,9 @@ struct block_syms_iterator_object {
       }									\
   } while (0)
 
-/* This is called when an objfile is about to be freed.
-   Invalidate the block as further actions on the block would result
-   in bad data.  All access to obj->symbol should be gated by
-   BLPY_REQUIRE_VALID which will raise an exception on invalid
-   blocks.  */
-struct blpy_deleter
-{
-  void operator() (block_object *obj)
-  {
-    while (obj)
-      {
-	block_object *next = obj->next;
-
-	obj->block = NULL;
-	obj->objfile = NULL;
-	obj->next = NULL;
-	obj->prev = NULL;
-
-	obj = next;
-      }
-  }
-};
-
 extern PyTypeObject block_syms_iterator_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("block_syms_iterator_object");
-static const registry<objfile>::key<block_object, blpy_deleter>
+static const registry<objfile>::key<htab, htab_deleter>
      blpy_objfile_data_key;
 
 static PyObject *
@@ -278,42 +251,54 @@ blpy_getitem (PyObject *self, PyObject *key)
   return nullptr;
 }
 
+/* Deleter function for the hash table.  */
+
 static void
-blpy_dealloc (PyObject *obj)
+block_object_del (void *obj)
 {
   block_object *block = (block_object *) obj;
+  block->block = nullptr;
+  block->objfile = nullptr;
+}
 
-  if (block->prev)
-    block->prev->next = block->next;
-  else if (block->objfile)
-    blpy_objfile_data_key.set (block->objfile, block->next);
-  if (block->next)
-    block->next->prev = block->prev;
-  block->block = NULL;
-  Py_TYPE (obj)->tp_free (obj);
+/* Hash function for the hash table.  */
+
+static hashval_t
+block_object_hash (const void *obj)
+{
+  const block_object *block = (const block_object *) obj;
+  return htab_hash_pointer (block->block);
+}
+
+/* Equality function for the hash table.  Note that searches must be
+   done with a plain block.  */
+
+static int
+block_object_eq (const void *a, const void *b)
+{
+  const block_object *blocka = (const block_object *) a;
+  const block *blockb = (const block *) b;
+  return blocka->block == blockb;
 }
 
-/* Given a block, and a block_object that has previously been
-   allocated and initialized, populate the block_object with the
-   struct block data.  Also, register the block_object life-cycle
-   with the life-cycle of the object file associated with this
-   block, if needed.  */
+/* Called when a gdb.Block is destroyed.  This removes it from the
+   hash.  */
+
 static void
-set_block (block_object *obj, const struct block *block,
-	   struct objfile *objfile)
+blpy_dealloc (PyObject *obj)
 {
-  obj->block = block;
-  obj->prev = NULL;
-  if (objfile)
+  block_object *block = (block_object *) obj;
+
+  if (block->objfile != nullptr)
     {
-      obj->objfile = objfile;
-      obj->next = blpy_objfile_data_key.get (objfile);
-      if (obj->next)
-	obj->next->prev = obj;
-      blpy_objfile_data_key.set (objfile, obj);
+      htab_t table = blpy_objfile_data_key.get (block->objfile);
+      hashval_t hash = block_object_hash (block);
+      /* This will clear the contents of the block as a side
+	 effect.  */
+      htab_remove_elt_with_hash (table, block->block, hash);
     }
-  else
-    obj->next = NULL;
+
+  Py_TYPE (obj)->tp_free (obj);
 }
 
 /* Create a new block object (gdb.Block) that encapsulates the struct
@@ -321,13 +306,32 @@ set_block (block_object *obj, const struct block *block,
 PyObject *
 block_to_block_object (const struct block *block, struct objfile *objfile)
 {
-  block_object *block_obj;
+  htab_t table = blpy_objfile_data_key.get (objfile);
+  if (table == nullptr)
+    {
+      table = htab_create_alloc (10, block_object_hash, block_object_eq,
+				 block_object_del, xcalloc, xfree);
+      blpy_objfile_data_key.set (objfile, table);
+    }
+
+  hashval_t hash = htab_hash_pointer (block);
+  block_object *result = (block_object *) htab_find_with_hash (table, block,
+							       hash);
+  if (result != nullptr)
+    {
+      PyObject *py_result = (PyObject *) result;
+      Py_INCREF (py_result);
+      return py_result;
+    }
+
+  result = PyObject_New (block_object, &block_object_type);
+  result->block = block;
+  result->objfile = objfile;
 
-  block_obj = PyObject_New (block_object, &block_object_type);
-  if (block_obj)
-    set_block (block_obj, block, objfile);
+  void **slot = htab_find_slot_with_hash (table, block, hash, INSERT);
+  *slot = result;
 
-  return (PyObject *) block_obj;
+  return (PyObject *) result;
 }
 
 /* Return struct block reference that is wrapped by this object.  */
@@ -452,6 +456,20 @@ blpy_repr (PyObject *self)
 			       name, str.c_str ());
 }
 
+/* Hash function for block objects.  */
+
+static Py_hash_t
+blpy_hash (PyObject *self)
+{
+  /* Python doesn't really expose its pointer hash function, so we use
+     our own.  */
+  Py_hash_t result = (Py_hash_t) htab_hash_pointer (self);
+  /* -1 has a special meaning for Python.  */
+  if (result == -1)
+    result = -2;
+  return result;
+}
+
 /* Implements the equality comparison for Block objects.  All other
    comparison operators will throw NotImplemented, as they aren't
    valid for blocks.  */
@@ -466,10 +484,7 @@ blpy_richcompare (PyObject *self, PyObject *other, int op)
       return Py_NotImplemented;
     }
 
-  block_object *self_block = (block_object *) self;
-  block_object *other_block = (block_object *) other;
-
-  bool expected = self_block->block == other_block->block;
+  bool expected = self == other;
   bool equal = op == Py_EQ;
   return PyBool_FromLong (equal == expected);
 }
@@ -542,7 +557,7 @@ PyTypeObject block_object_type = {
   0,				  /*tp_as_number*/
   0,				  /*tp_as_sequence*/
   &block_object_as_mapping,	  /*tp_as_mapping*/
-  0,				  /*tp_hash */
+  blpy_hash,			  /*tp_hash */
   0,				  /*tp_call*/
   0,				  /*tp_str*/
   0,				  /*tp_getattro*/
diff --git a/gdb/testsuite/gdb.python/py-block.exp b/gdb/testsuite/gdb.python/py-block.exp
index 99642a546a7..0e6851ddf8b 100644
--- a/gdb/testsuite/gdb.python/py-block.exp
+++ b/gdb/testsuite/gdb.python/py-block.exp
@@ -119,8 +119,12 @@ gdb_test "python print (block.is_valid())" "True" \
          "Check block validity"
 gdb_test "python print (block_iter.is_valid())" "True" \
          "Check block_iter validity"
+gdb_test_no_output "python x = hash(block)" "block is hashable"
+gdb_test "python print (type (x))" "<class 'int'>" "block hash is integer"
 gdb_unload
 gdb_test "python print (block.is_valid())" "False" \
          "Check block validity after unload"
 gdb_test "python print (block_iter.is_valid())" "False" \
          "Check block_iter validity after unload"
+gdb_test "python print (hash (block) == x)" "True" \
+    "block hash did not change"

-- 
2.44.0

[PATCH 2/3] Convert DAP disassemble code to use Block hashing

[Tom Tromey]

This changes the DAP disassemble code to use the new Block hashing,
storing the already-visited blocks in a set rather than a list.
---
 gdb/python/lib/gdb/dap/disassemble.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index d65790a40b0..a2e27e54a64 100644
--- a/gdb/python/lib/gdb/dap/disassemble.py
+++ b/gdb/python/lib/gdb/dap/disassemble.py
@@ -27,9 +27,8 @@ class _BlockTracker:
         # just one label -- DAP wouldn't let us return multiple labels
         # anyway.
         self.labels = {}
-        # List of blocks that have already been handled.  Note that
-        # blocks aren't hashable so a set is not used.
-        self.blocks = []
+        # Blocks that have already been handled.
+        self.blocks = set()
 
     # Add a gdb.Block and its superblocks, ignoring the static and
     # global block.  BLOCK can also be None, which is ignored.
@@ -37,7 +36,7 @@ class _BlockTracker:
         while block is not None:
             if block.is_static or block.is_global or block in self.blocks:
                 return
-            self.blocks.append(block)
+            self.blocks.add(block)
             if block.function is not None:
                 self.labels[block.start] = block.function.name
             for sym in block:

-- 
2.44.0

[PATCH 3/3] Return global scope from DAP scopes request

[Tom Tromey]

A co-worker requested that the DAP code emit a scope for global
variables.  It's not really practical to do this for all globals, but
it seemed reasonable to do this for globals coming from the frame's
compilation unit.  For Ada in particular, this is convenient as it
exposes package-scoped variables.
---
 gdb/NEWS                              |  3 ++
 gdb/data-directory/Makefile.in        |  1 +
 gdb/python/lib/gdb/dap/globalvars.py  | 97 +++++++++++++++++++++++++++++++++++
 gdb/python/lib/gdb/dap/scopes.py      |  4 ++
 gdb/testsuite/gdb.dap/ptrref.exp      |  8 +--
 gdb/testsuite/gdb.dap/rust-slices.exp |  4 +-
 6 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 691abff420b..ab252c81ab6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -179,6 +179,9 @@ show unwind-on-signal
   ** The "set debug dap-log-file" command is now documented.  This
      command was available in GDB 14 but not documented.
 
+  ** The "scopes" request will now return a scope holding global
+     variables from the stack frame's compilation unit.
+
 * Guile API
 
   ** New constants SYMBOL_TYPE_DOMAIN, SYMBOL_FUNCTION_DOMAIN, and
diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index 98a43529b07..f529656ca05 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -98,6 +98,7 @@ PYTHON_FILE_LIST = \
 	gdb/dap/evaluate.py \
 	gdb/dap/events.py \
 	gdb/dap/frames.py \
+	gdb/dap/globalvars.py \
 	gdb/dap/__init__.py \
 	gdb/dap/io.py \
 	gdb/dap/launch.py \
diff --git a/gdb/python/lib/gdb/dap/globalvars.py b/gdb/python/lib/gdb/dap/globalvars.py
new file mode 100644
index 00000000000..149c9a8f22f
--- /dev/null
+++ b/gdb/python/lib/gdb/dap/globalvars.py
@@ -0,0 +1,97 @@
+# Copyright 2024 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
+
+from .sources import make_source
+from .startup import in_gdb_thread
+from .varref import BaseReference
+
+# Map a block identifier to a scope object.
+_id_to_scope = {}
+
+
+# Arrange to clear the scope references when the inferior runs.
+@in_gdb_thread
+def clear(event):
+    global _id_to_scope
+    _id_to_scope = {}
+
+
+gdb.events.cont.connect(clear)
+
+
+# A scope that holds static and/or global variables.
+class _Globals(BaseReference):
+    def __init__(self, filename, var_list):
+        super().__init__("Globals")
+        self.filename = filename
+        self.var_list = var_list
+
+    def to_object(self):
+        result = super().to_object()
+        result["presentationHint"] = "globals"
+        # How would we know?
+        result["expensive"] = False
+        result["namedVariables"] = self.child_count()
+        if self.filename is not None:
+            result["source"] = make_source(self.filename)
+        return result
+
+    def has_children(self):
+        # This object won't even be created if there are no variables
+        # to return.
+        return True
+
+    def child_count(self):
+        return len(self.var_list)
+
+    @in_gdb_thread
+    def fetch_one_child(self, idx):
+        return self.var_list[idx].value()
+
+
+@in_gdb_thread
+def get_global_scope(frame):
+    """Given a frame decorator, return the corresponding global scope
+    object.
+
+    If the frame does not have a block, or if the CU does not have
+    globals (that is, empty static and global blocks), return None."""
+    inf_frame = frame.inferior_frame()
+    # It's unfortunate that this API throws instead of returning None.
+    try:
+        block = inf_frame.block()
+    except RuntimeError:
+        return None
+
+    global _id_to_scope
+    block = block.static_block
+    if block in _id_to_scope:
+        return _id_to_scope[block]
+
+    syms = []
+    block_iter = block
+    while block_iter is not None:
+        syms += [sym for sym in block_iter if sym.is_variable]
+        block_iter = block_iter.superblock
+
+    if len(syms) == 0:
+        return None
+
+    result = _Globals(frame.filename(), syms)
+    _id_to_scope[block] = result
+
+    return result
diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
index 8cd860141d6..ad2f10f4057 100644
--- a/gdb/python/lib/gdb/dap/scopes.py
+++ b/gdb/python/lib/gdb/dap/scopes.py
@@ -16,6 +16,7 @@
 import gdb
 
 from .frames import frame_for_id
+from .globalvars import get_global_scope
 from .server import request
 from .startup import in_gdb_thread
 from .varref import BaseReference
@@ -161,4 +162,7 @@ def scopes(*, frameId: int, **extra):
             scopes.append(_ScopeReference("Locals", "locals", frame, locs))
         scopes.append(_RegisterReference("Registers", frame))
         frame_to_scope[frameId] = scopes
+        global_scope = get_global_scope(frame)
+        if global_scope is not None:
+            scopes.append(global_scope)
     return {"scopes": [x.to_object() for x in scopes]}
diff --git a/gdb/testsuite/gdb.dap/ptrref.exp b/gdb/testsuite/gdb.dap/ptrref.exp
index 0552c3b9815..236ffae12d5 100644
--- a/gdb/testsuite/gdb.dap/ptrref.exp
+++ b/gdb/testsuite/gdb.dap/ptrref.exp
@@ -54,12 +54,14 @@ 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]
 
-gdb_assert {[llength $scopes] == 2} "two scopes"
+gdb_assert {[llength $scopes] == 3} "three scopes"
 
-lassign $scopes scope reg_scope
+lassign $scopes scope reg_scope global_scope
 gdb_assert {[dict get $scope name] == "Locals"} "scope is locals"
+gdb_assert {[dict get $global_scope name] == "Globals"} "scope is globals"
 
-gdb_assert {[dict get $scope namedVariables] == 4} "three vars in scope"
+gdb_assert {[dict get $scope namedVariables] == 4} "four vars in locals"
+gdb_assert {[dict get $global_scope namedVariables] == 1} "one var in globals"
 
 set num [dict get $scope variablesReference]
 set refs [lindex [dap_check_request_and_response "fetch variables" \
diff --git a/gdb/testsuite/gdb.dap/rust-slices.exp b/gdb/testsuite/gdb.dap/rust-slices.exp
index c85568d69ea..d3bd3050c16 100644
--- a/gdb/testsuite/gdb.dap/rust-slices.exp
+++ b/gdb/testsuite/gdb.dap/rust-slices.exp
@@ -59,7 +59,9 @@ 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]
 
-gdb_assert {[llength $scopes] == 2} "two scopes"
+# There are three scopes because an artificial symbol ends up in the
+# DWARF.  See https://github.com/rust-lang/rust/issues/125126.
+gdb_assert {[llength $scopes] == 3} "three scopes"
 
 lassign $scopes scope ignore
 gdb_assert {[dict get $scope name] == "Locals"} "scope is locals"

-- 
2.44.0

Re: [PATCH 3/3] Return global scope from DAP scopes request

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Date: Thu, 16 May 2024 12:01:13 -0600
> 
> A co-worker requested that the DAP code emit a scope for global
> variables.  It's not really practical to do this for all globals, but
> it seemed reasonable to do this for globals coming from the frame's
> compilation unit.  For Ada in particular, this is convenient as it
> exposes package-scoped variables.
> ---
>  gdb/NEWS                              |  3 ++
>  gdb/data-directory/Makefile.in        |  1 +
>  gdb/python/lib/gdb/dap/globalvars.py  | 97 +++++++++++++++++++++++++++++++++++
>  gdb/python/lib/gdb/dap/scopes.py      |  4 ++
>  gdb/testsuite/gdb.dap/ptrref.exp      |  8 +--
>  gdb/testsuite/gdb.dap/rust-slices.exp |  4 +-
>  6 files changed, 113 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 691abff420b..ab252c81ab6 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -179,6 +179,9 @@ show unwind-on-signal
>    ** The "set debug dap-log-file" command is now documented.  This
>       command was available in GDB 14 but not documented.
>  
> +  ** The "scopes" request will now return a scope holding global
> +     variables from the stack frame's compilation unit.
> +
>  * Guile API

The NEWS part is okay, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 1/3] Memoize gdb.Block and make them hashable

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Thu, May 16, 2024 at 8:01 PM Tom Tromey <tromey@adacore.com> wrote:

> In subsequent patches, it's handy if gdb.Block is hashable, so it can
> be stored in a set or a dictionary.  However, doing this in a
> straightforward way is not really possible, because a block isn't
> truly immutable -- it can be invalidated.  And, while this isn't a
> real problem for my use case (in DAP the maps are only used during a
> single stop), it seemed error-prone.
>
> This patch instead takes the approach of using the gdb.Block's own
> object identity to allow hashing.  This seems fine because the
> contents don't affect the hashing.  In order for this to work, though,
> the blocks have to be memoized -- two requests for the same block must
> return the same object.
>
> This also allows (actually, requires) the simplification of the
> rich-compare method for blocks.
> ---
>
>
This change looks reasonable to me. Also it introduces no regressions on
ppc64le with Fedora Rawhide.

Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>

Re: [PATCH 0/3] Return a global scope from DAP scopes request

[Tom Tromey]

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

Tom> A co-worker asked that some global variables (in his case,
Tom> specifically package-scoped variables in Ada) be visible in the
Tom> response to the DAP "scopes" request.

Tom> This series implements this.  I wasn't completely sure what to call
Tom> the resulting scope.  I chose "globals" but really the scope here is
Tom> "globals local to the frame's CU".

I minorly updated the NEWS change to reflect the gdb 15 release (moved
the text up to the new section).

I'm checking this in now.

Tom