[PATCH v2 0/3] Add more info to DAP disassemble response

[PATCH v2 0/3] Add more info to DAP disassemble response

[Tom Tromey]

A user requested some extra information be added to the DAP
disassemble response.  This series is the result.

This version of the series makes the block-tracking code (see patch 3)
more robust.  A user noticed a case where this code would throw an
exception when trying to access a member of None.  I was unable to
reproduce the bug, however, so there's no test case.

Tom

[PATCH v2 1/3] Simplify DAP make_source callers

[Tom Tromey]

A couple callers of make_source call basename by hand.  Rather than
add another caller like this, I thought it would be better to put this
ability into make_source itself.
---
 gdb/python/lib/gdb/dap/breakpoint.py | 3 +--
 gdb/python/lib/gdb/dap/bt.py         | 4 +---
 gdb/python/lib/gdb/dap/sources.py    | 8 ++++++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/breakpoint.py b/gdb/python/lib/gdb/dap/breakpoint.py
index b2e74277ebd..1380f7decd4 100644
--- a/gdb/python/lib/gdb/dap/breakpoint.py
+++ b/gdb/python/lib/gdb/dap/breakpoint.py
@@ -13,7 +13,6 @@
 # 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 os
 import re
 from contextlib import contextmanager
 
@@ -116,7 +115,7 @@ def _breakpoint_descriptor(bp):
 
             result.update(
                 {
-                    "source": make_source(filename, os.path.basename(filename)),
+                    "source": make_source(filename),
                     "line": line,
                 }
             )
diff --git a/gdb/python/lib/gdb/dap/bt.py b/gdb/python/lib/gdb/dap/bt.py
index e0c2e2a1751..668bcc7ce23 100644
--- a/gdb/python/lib/gdb/dap/bt.py
+++ b/gdb/python/lib/gdb/dap/bt.py
@@ -13,8 +13,6 @@
 # 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 os
-
 # This is deprecated in 3.9, but required in older versions.
 from typing import Optional
 
@@ -98,7 +96,7 @@ def _backtrace(thread_id, levels, startFrame, stack_format):
                     name += ", module " + objfile.username
             filename = current_frame.filename()
             if filename is not None:
-                newframe["source"] = make_source(filename, os.path.basename(filename))
+                newframe["source"] = make_source(filename)
             newframe["name"] = name
             frames.append(newframe)
         # Note that we do not calculate totalFrames here.  Its absence
diff --git a/gdb/python/lib/gdb/dap/sources.py b/gdb/python/lib/gdb/dap/sources.py
index ee3464db679..ad0c913c8c1 100644
--- a/gdb/python/lib/gdb/dap/sources.py
+++ b/gdb/python/lib/gdb/dap/sources.py
@@ -32,16 +32,20 @@ _id_map = {}
 
 
 @in_gdb_thread
-def make_source(fullname, filename):
+def make_source(fullname, filename=None):
     """Return the Source for a given file name.
 
     FULLNAME is the full name.  This is used as the key.
-    FILENAME is the base name.
+    FILENAME is the base name; if None (the default), then it is
+    computed from FULLNAME.
     """
     global _source_map
     if fullname in _source_map:
         result = _source_map[fullname]
     else:
+        if filename is None:
+            filename = os.path.basename(fullname)
+
         result = {
             "name": filename,
             "path": fullname,
-- 
2.44.0

[PATCH v2 2/3] Implement tp_richcompare for gdb.Block

[Tom Tromey]

I noticed that two gdb.Block objects will never compare as equal with
'=='.  This patch fixes the problem by implementing tp_richcompare, as
was done for gdb.Frame.
---
 gdb/python/py-block.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
index 3e30faf0856..3de6200e7c2 100644
--- a/gdb/python/py-block.c
+++ b/gdb/python/py-block.c
@@ -452,6 +452,28 @@ blpy_repr (PyObject *self)
 			       name, str.c_str ());
 }
 
+/* Implements the equality comparison for Block objects.  All other
+   comparison operators will throw NotImplemented, as they aren't
+   valid for blocks.  */
+
+static PyObject *
+blpy_richcompare (PyObject *self, PyObject *other, int op)
+{
+  if (!PyObject_TypeCheck (other, &block_object_type)
+      || (op != Py_EQ && op != Py_NE))
+    {
+      Py_INCREF (Py_NotImplemented);
+      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 equal = op == Py_EQ;
+  return PyBool_FromLong (equal == expected);
+}
+
 static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION
 gdbpy_initialize_blocks (void)
 {
@@ -530,7 +552,7 @@ PyTypeObject block_object_type = {
   "GDB block object",		  /* tp_doc */
   0,				  /* tp_traverse */
   0,				  /* tp_clear */
-  0,				  /* tp_richcompare */
+  blpy_richcompare,		  /* tp_richcompare */
   0,				  /* tp_weaklistoffset */
   blpy_iter,			  /* tp_iter */
   0,				  /* tp_iternext */
-- 
2.44.0

[PATCH v2 3/3] Add symbol, line, and location to DAP disassemble result

[Tom Tromey]

The DAP spec allows a number of attributes on the resulting
instructions that gdb currently does not emit.  A user requested some
of these, so this patch adds the 'symbol', 'line', and 'location'
attributes.  While the spec lets the implementation omit 'location' in
some cases, it was simpler in the code to just always emit it, as then
no extra tracking was needed.
---
 gdb/python/lib/gdb/dap/disassemble.py |  59 +++++++++++++--
 gdb/testsuite/gdb.dap/disassem.c      |  52 +++++++++++++
 gdb/testsuite/gdb.dap/disassem.exp    | 105 ++++++++++++++++++++++++++
 3 files changed, 209 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dap/disassem.c
 create mode 100644 gdb/testsuite/gdb.dap/disassem.exp

diff --git a/gdb/python/lib/gdb/dap/disassemble.py b/gdb/python/lib/gdb/dap/disassemble.py
index 65bf3d4457b..b99e4f83f84 100644
--- a/gdb/python/lib/gdb/dap/disassemble.py
+++ b/gdb/python/lib/gdb/dap/disassemble.py
@@ -16,6 +16,50 @@
 import gdb
 
 from .server import capability, request
+from .sources import make_source
+
+
+# This tracks labels associated with a disassembly request and helps
+# with updating individual instructions.
+class _BlockTracker:
+    def __init__(self):
+        # Map from PC to symbol names.  A given PC is assumed to have
+        # 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 = []
+
+    # Add a gdb.Block and its superblocks, ignoring the static and
+    # global block.  BLOCK can also be None, which is ignored.
+    def add_block(self, block):
+        while block is not None:
+            if block.is_static or block.is_global or block in self.blocks:
+                return
+            self.blocks.append(block)
+            if block.function is not None:
+                self.labels[block.start] = block.function.name
+            for sym in block:
+                if sym.addr_class == gdb.SYMBOL_LOC_LABEL:
+                    self.labels[int(sym.value())] = sym.name
+            block = block.superblock
+
+    # Add PC to this tracker.  Update RESULT as appropriate with
+    # information about the source and any label.
+    def add_pc(self, pc, result):
+        self.add_block(gdb.block_for_pc(pc))
+        if pc in self.labels:
+            result["symbol"] = self.labels[pc]
+        sal = gdb.find_pc_line(pc)
+        if sal.symtab is not None:
+            if sal.line != 0:
+                result["line"] = str(sal.line)
+            if sal.symtab.filename is not None:
+                # The spec says this can be omitted in some
+                # situations, but it's a little simpler to just always
+                # supply it.
+                result["location"] = make_source(sal.symtab.filename)
 
 
 @request("disassemble")
@@ -35,17 +79,18 @@ def disassemble(
     except gdb.error:
         # Maybe there was no frame.
         arch = inf.architecture()
+    tracker = _BlockTracker()
     result = []
     total_count = instructionOffset + instructionCount
     for elt in arch.disassemble(pc, count=total_count)[instructionOffset:]:
         mem = inf.read_memory(elt["addr"], elt["length"])
-        result.append(
-            {
-                "address": hex(elt["addr"]),
-                "instruction": elt["asm"],
-                "instructionBytes": mem.hex(),
-            }
-        )
+        insn = {
+            "address": hex(elt["addr"]),
+            "instruction": elt["asm"],
+            "instructionBytes": mem.hex(),
+        }
+        tracker.add_pc(elt["addr"], insn)
+        result.append(insn)
     return {
         "instructions": result,
     }
diff --git a/gdb/testsuite/gdb.dap/disassem.c b/gdb/testsuite/gdb.dap/disassem.c
new file mode 100644
index 00000000000..c0f5128c97d
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/disassem.c
@@ -0,0 +1,52 @@
+/* Copyright 2024 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/>.  */
+
+__attribute__((__noinline__)) static int
+callee (int x)
+{
+  return x * 2;
+}
+
+static inline int __attribute__((__always_inline__))
+compute (int x)
+{
+  return callee (x);
+}
+
+static int
+return_value (int x)
+{
+  int accum = 0;
+
+  for (int i = 0; i < x; ++i)
+    {
+      int value = compute (i);
+      if (value < 0)
+	goto out_label;
+    }
+
+ out_label:
+
+  return accum;
+}
+
+int
+main ()
+{
+  int value = return_value (23);
+  return value > 0 ? 0 : 1;
+}
diff --git a/gdb/testsuite/gdb.dap/disassem.exp b/gdb/testsuite/gdb.dap/disassem.exp
new file mode 100644
index 00000000000..87fb516931a
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/disassem.exp
@@ -0,0 +1,105 @@
+# 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/>.
+
+# Test DAP disassembly.
+
+require allow_dap_tests
+
+load_lib dap-support.exp
+
+standard_testfile
+
+if {[build_executable ${testfile}.exp $testfile] == -1} {
+    return
+}
+
+if {[dap_initialize] == ""} {
+    return
+}
+
+set obj [dap_check_request_and_response "set breakpoint" \
+	     setFunctionBreakpoints \
+	     {o breakpoints [a [o name [s main]]]}]
+set fn_bpno [dap_get_breakpoint_number $obj]
+
+dap_check_request_and_response "configurationDone" configurationDone
+
+if {[dap_launch $testfile] == ""} {
+    return
+}
+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" $fn_bpno
+
+# Find out how many lines of disassembly we should request.  This is
+# kind of lame but DAP doesn't really provide tools to do this, and
+# gdb's DAP implementation doesn't try to figure out what memory might
+# not really be part of a function.
+set obj [dap_check_request_and_response "disassemble using CLI" \
+	     evaluate {o expression [s {disassemble &return_value}] \
+			   context [s repl]}]
+set output [dict get [lindex $obj 0] body result]
+# The result will have literal "\" "n" sequences, turn these into
+# newlines.
+set with_nl [string map [list "\\n" "\n"] $output]
+# The value we want is the number of lines starting with an address.
+set insn_count 0
+foreach line [split $with_nl "\n"] {
+    if {[regexp "^ *0x" $line]} {
+	incr insn_count
+    }
+}
+
+set obj [dap_check_request_and_response "find function address" \
+	     evaluate {o expression [s "&return_value"]}]
+set pc [dict get [lindex $obj 0] body memoryReference]
+
+set obj [dap_check_request_and_response "disassemble the function" \
+	     disassemble \
+	     [format {o memoryReference [s %s] instructionCount [i %d]} \
+		  $pc $insn_count]]
+set response [lindex $obj 0]
+
+set seen_labels(_) _
+set insn_no 1
+foreach insn [dict get $response body instructions] {
+    with_test_prefix $insn_no {
+	gdb_assert {[dict exists $insn line]} \
+	    "line in disassemble output"
+	gdb_assert {[dict exists $insn location]} \
+	    "location in disassemble output"
+	if {[dict exists $insn symbol]} {
+	    set seen_labels([dict get $insn symbol]) 1
+	}
+    }
+    incr insn_no
+}
+
+proc require_label {name} {
+    global seen_labels
+    if {[info exists seen_labels($name)]} {
+	pass "saw label $name"
+    } else {
+	fail "saw label $name"
+    }
+}
+
+require_label return_value
+require_label compute
+require_label out_label
+
+dap_shutdown
-- 
2.44.0

Re: [PATCH v2 3/3] Add symbol, line, and location to DAP disassemble result

[Tom Tromey]

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

Tom> The DAP spec allows a number of attributes on the resulting
Tom> instructions that gdb currently does not emit.  A user requested some
Tom> of these, so this patch adds the 'symbol', 'line', and 'location'
Tom> attributes.  While the spec lets the implementation omit 'location' in
Tom> some cases, it was simpler in the code to just always emit it, as then
Tom> no extra tracking was needed.

Tom> +            if sal.line != 0:
Tom> +                result["line"] = str(sal.line)

A co-worker noticed that this is emitting a string instead of a number,
contrary to the spec.  I've fixed this locally.

Tom

Re: [PATCH v2 0/3] Add more info to DAP disassemble response

[Tom Tromey]

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

Tom> A user requested some extra information be added to the DAP
Tom> disassemble response.  This series is the result.

Tom> This version of the series makes the block-tracking code (see patch 3)
Tom> more robust.  A user noticed a case where this code would throw an
Tom> exception when trying to access a member of None.  I was unable to
Tom> reproduce the bug, however, so there's no test case.

I'm checking this in now.

Tom