[PATCH v2 0/7] Reimplement DAP backtrace using frame filters

[PATCH v2 0/7] Reimplement DAP backtrace using frame filters

[Tom Tromey]

This series reimplements DAP backtraces using frame filters.  This
seemed nice to have, because it would present users with a friendlier
view into the stack.  It also turned out to simplify the code a bit.

---
Changes in v2:
- Removed is_constant check from FrameVars
- Added patch to strip typedefs
- Added Ada scopes test case
- Link to v1: https://inbox.sourceware.org/gdb-patches/20230614-dap-frame-decor-v1-0-af87db6467b2@adacore.com

---
Tom Tromey (7):
      Fix execute_frame_filters doc string
      Add new interface to frame filter iteration
      Fix oversights in frame decorator code
      Simplify FrameVars
      Reimplement DAP stack traces using frame filters
      Handle typedefs in no-op pretty printers
      Add Ada scope test for DAP

 gdb/python/lib/gdb/FrameDecorator.py      | 44 ++++++----------
 gdb/python/lib/gdb/dap/bt.py              | 81 +++++++++++++----------------
 gdb/python/lib/gdb/dap/evaluate.py        | 11 ++--
 gdb/python/lib/gdb/dap/frames.py          |  7 +++
 gdb/python/lib/gdb/dap/scopes.py          | 83 ++++++++++++------------------
 gdb/python/lib/gdb/frames.py              | 83 +++++++++++++++++++++---------
 gdb/python/lib/gdb/printing.py            | 23 +++++----
 gdb/testsuite/gdb.dap/ada-scopes.exp      | 84 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.dap/ada-scopes/pack.adb | 23 +++++++++
 gdb/testsuite/gdb.dap/ada-scopes/pack.ads | 21 ++++++++
 gdb/testsuite/gdb.dap/ada-scopes/prog.adb | 26 ++++++++++
 gdb/testsuite/gdb.dap/scopes.c            |  6 +--
 12 files changed, 321 insertions(+), 171 deletions(-)
---
base-commit: d8a001f57016eff05977e9699c7aabdf4302c71b
change-id: 20230614-dap-frame-decor-5cccd7f34dd3

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

[PATCH v2 1/7] Fix execute_frame_filters doc string

[Tom Tromey]

When reading the doc string for execute_frame_filters, I wasn't sure
if the ranges were inclusive or exclusive.  This patch updates the doc
string to reflect my findings, and also fixes an existing typo.
---
 gdb/python/lib/gdb/frames.py | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/python/lib/gdb/frames.py b/gdb/python/lib/gdb/frames.py
index 5f8119c22a0..52f4191a8e8 100644
--- a/gdb/python/lib/gdb/frames.py
+++ b/gdb/python/lib/gdb/frames.py
@@ -165,18 +165,20 @@ def execute_frame_filters(frame, frame_low, frame_high):
     Arguments:
         frame: The initial frame.
 
-        frame_low: The low range of the slice.  If this is a negative
-        integer then it indicates a backward slice (ie bt -4) which
-        counts backward from the last frame in the backtrace.
+        frame_low: The low range of the slice, counting from 0.  If
+        this is a negative integer then it indicates a backward slice
+        (ie bt -4) which counts backward from the last frame in the
+        backtrace.
 
-        frame_high: The high range of the slice.  If this is -1 then
-        it indicates all frames until the end of the stack from
-        frame_low.
+        frame_high: The high range of the slice, inclusive.  If this
+        is -1 then it indicates all frames until the end of the stack
+        from frame_low.
 
     Returns:
         frame_iterator: The sliced iterator after all frame
-        filters have had a change to execute, or None if no frame
+        filters have had a chance to execute, or None if no frame
         filters are registered.
+
     """
 
     # Get a sorted list of frame filters.

-- 
2.40.1

[PATCH v2 2/7] Add new interface to frame filter iteration

[Tom Tromey]

This patch adds a new function, frame_iterator, that wraps the
existing code to find and execute the frame filters.  However, unlike
execute_frame_filters, it will always return an iterator -- whereas
execute_frame_filters will return None if no frame filters apply.

Nothing uses this new function yet, but it will used by a subsequent
DAP patch.
---
 gdb/python/lib/gdb/frames.py | 85 ++++++++++++++++++++++++++++++--------------
 1 file changed, 59 insertions(+), 26 deletions(-)

diff --git a/gdb/python/lib/gdb/frames.py b/gdb/python/lib/gdb/frames.py
index 52f4191a8e8..01e7cff6c70 100644
--- a/gdb/python/lib/gdb/frames.py
+++ b/gdb/python/lib/gdb/frames.py
@@ -156,37 +156,16 @@ def _sort_list():
     return sorted_frame_filters
 
 
-def execute_frame_filters(frame, frame_low, frame_high):
-    """Internal function called from GDB that will execute the chain
-    of frame filters.  Each filter is executed in priority order.
-    After the execution completes, slice the iterator to frame_low -
-    frame_high range.
-
-    Arguments:
-        frame: The initial frame.
-
-        frame_low: The low range of the slice, counting from 0.  If
-        this is a negative integer then it indicates a backward slice
-        (ie bt -4) which counts backward from the last frame in the
-        backtrace.
-
-        frame_high: The high range of the slice, inclusive.  If this
-        is -1 then it indicates all frames until the end of the stack
-        from frame_low.
-
-    Returns:
-        frame_iterator: The sliced iterator after all frame
-        filters have had a chance to execute, or None if no frame
-        filters are registered.
-
-    """
-
+# Internal function that implements frame_iterator and
+# execute_frame_filters.  If ALWAYS is True, then this will always
+# return an iterator.
+def _frame_iterator(frame, frame_low, frame_high, always):
     # Get a sorted list of frame filters.
     sorted_list = list(_sort_list())
 
     # Check to see if there are any frame-filters.  If not, just
     # return None and let default backtrace printing occur.
-    if len(sorted_list) == 0:
+    if not always and len(sorted_list) == 0:
         return None
 
     frame_iterator = FrameIterator(frame)
@@ -233,3 +212,57 @@ def execute_frame_filters(frame, frame_low, frame_high):
     sliced = itertools.islice(frame_iterator, frame_low, frame_high)
 
     return sliced
+
+
+def frame_iterator(frame, frame_low, frame_high):
+    """Helper function that will execute the chain of frame filters.
+    Each filter is executed in priority order.  After the execution
+    completes, slice the iterator to frame_low - frame_high range.  An
+    iterator is always returned.
+
+    Arguments:
+        frame: The initial frame.
+
+        frame_low: The low range of the slice, counting from 0.  If
+        this is a negative integer then it indicates a backward slice
+        (ie bt -4) which counts backward from the last frame in the
+        backtrace.
+
+        frame_high: The high range of the slice, inclusive.  If this
+        is -1 then it indicates all frames until the end of the stack
+        from frame_low.
+
+    Returns:
+        frame_iterator: The sliced iterator after all frame
+        filters have had a chance to execute.
+    """
+
+    return _frame_iterator(frame, frame_low, frame_high, True)
+
+
+def execute_frame_filters(frame, frame_low, frame_high):
+    """Internal function called from GDB that will execute the chain
+    of frame filters.  Each filter is executed in priority order.
+    After the execution completes, slice the iterator to frame_low -
+    frame_high range.
+
+    Arguments:
+        frame: The initial frame.
+
+        frame_low: The low range of the slice, counting from 0.  If
+        this is a negative integer then it indicates a backward slice
+        (ie bt -4) which counts backward from the last frame in the
+        backtrace.
+
+        frame_high: The high range of the slice, inclusive.  If this
+        is -1 then it indicates all frames until the end of the stack
+        from frame_low.
+
+    Returns:
+        frame_iterator: The sliced iterator after all frame
+        filters have had a chance to execute, or None if no frame
+        filters are registered.
+
+    """
+
+    return _frame_iterator(frame, frame_low, frame_high, False)

-- 
2.40.1

[PATCH v2 3/7] Fix oversights in frame decorator code

[Tom Tromey]

The frame decorator "FrameVars" code misses a couple of cases,
discovered when working on related DAP changes.

First, fetch_frame_locals does not stop when reaching a function
boundary.  This means it would return locals from any enclosing
functions.

Second, fetch_frame_args assumes that all arguments are at the
outermost scope, but this doesn't seem to be required by gdb.
---
 gdb/python/lib/gdb/FrameDecorator.py | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py
index 6773780735b..7293be86185 100644
--- a/gdb/python/lib/gdb/FrameDecorator.py
+++ b/gdb/python/lib/gdb/FrameDecorator.py
@@ -269,6 +269,11 @@ class FrameVars(object):
                 if self.fetch_b(sym):
                     lvars.append(SymValueWrapper(sym, None))
 
+            # Stop when the function itself is seen, to avoid showing
+            # variables from outer functions in a nested function.
+            if block.function is not None:
+                break
+
             block = block.superblock
 
         return lvars
@@ -286,14 +291,18 @@ class FrameVars(object):
             block = None
 
         while block is not None:
-            if block.function is not None:
+            if block.is_global or block.is_static:
                 break
-            block = block.superblock
-
-        if block is not None:
             for sym in block:
                 if not sym.is_argument:
                     continue
                 args.append(SymValueWrapper(sym, None))
 
+            # Stop when the function itself is seen, to avoid showing
+            # variables from outer functions in a nested function.
+            if block.function is not None:
+                break
+
+            block = block.superblock
+
         return args

-- 
2.40.1

[PATCH v2 4/7] Simplify FrameVars

[Tom Tromey]

FrameVars implements its own variant of Symbol.is_variable and/or
Symbol.is_control.  This patch replaces this code.
---
 gdb/python/lib/gdb/FrameDecorator.py | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/gdb/python/lib/gdb/FrameDecorator.py b/gdb/python/lib/gdb/FrameDecorator.py
index 7293be86185..050cb934b7c 100644
--- a/gdb/python/lib/gdb/FrameDecorator.py
+++ b/gdb/python/lib/gdb/FrameDecorator.py
@@ -223,31 +223,6 @@ class FrameVars(object):
 
     def __init__(self, frame):
         self.frame = frame
-        self.symbol_class = {
-            gdb.SYMBOL_LOC_STATIC: True,
-            gdb.SYMBOL_LOC_REGISTER: True,
-            gdb.SYMBOL_LOC_ARG: True,
-            gdb.SYMBOL_LOC_REF_ARG: True,
-            gdb.SYMBOL_LOC_LOCAL: True,
-            gdb.SYMBOL_LOC_REGPARM_ADDR: True,
-            gdb.SYMBOL_LOC_COMPUTED: True,
-        }
-
-    def fetch_b(self, sym):
-        """Local utility method to determine if according to Symbol
-        type whether it should be included in the iterator.  Not all
-        symbols are fetched, and only symbols that return
-        True from this method should be fetched."""
-
-        # SYM may be a string instead of a symbol in the case of
-        # synthetic local arguments or locals.  If that is the case,
-        # always fetch.
-        if isinstance(sym, str):
-            return True
-
-        sym_type = sym.addr_class
-
-        return self.symbol_class.get(sym_type, False)
 
     def fetch_frame_locals(self):
         """Public utility method to fetch frame local variables for
@@ -266,7 +241,7 @@ class FrameVars(object):
             for sym in block:
                 if sym.is_argument:
                     continue
-                if self.fetch_b(sym):
+                if sym.is_variable:
                     lvars.append(SymValueWrapper(sym, None))
 
             # Stop when the function itself is seen, to avoid showing

-- 
2.40.1

[PATCH v2 5/7] Reimplement DAP stack traces using frame filters

[Tom Tromey]

This reimplements DAP stack traces using frame filters.  This slightly
simplifies the code, because frame filters and DAP were already doing
some similar work.  This also renames RegisterReference and
ScopeReference to make it clear that these are private (and so changes
don't have to worry about other files).

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30468
---
 gdb/python/lib/gdb/dap/bt.py       | 81 ++++++++++++++++---------------------
 gdb/python/lib/gdb/dap/evaluate.py | 11 ++---
 gdb/python/lib/gdb/dap/frames.py   |  7 ++++
 gdb/python/lib/gdb/dap/scopes.py   | 83 +++++++++++++++-----------------------
 4 files changed, 79 insertions(+), 103 deletions(-)

diff --git a/gdb/python/lib/gdb/dap/bt.py b/gdb/python/lib/gdb/dap/bt.py
index 4439b428926..0350a3bb6d5 100644
--- a/gdb/python/lib/gdb/dap/bt.py
+++ b/gdb/python/lib/gdb/dap/bt.py
@@ -15,67 +15,56 @@
 
 import gdb
 import os
+import itertools
 
+from gdb.frames import frame_iterator
+from gdb.FrameIterator import FrameIterator
+from gdb.FrameDecorator import FrameDecorator
 from .frames import frame_id
 from .server import request, capability
 from .startup import send_gdb_with_response, in_gdb_thread
 from .state import set_thread
 
 
-# Helper function to safely get the name of a frame as a string.
-@in_gdb_thread
-def _frame_name(frame):
-    name = frame.name()
-    if name is None:
-        name = "???"
-    return name
-
-
-# Helper function to get a frame's SAL without an error.
-@in_gdb_thread
-def _safe_sal(frame):
-    try:
-        return frame.find_sal()
-    except gdb.error:
-        return None
-
-
 # Helper function to compute a stack trace.
 @in_gdb_thread
 def _backtrace(thread_id, levels, startFrame):
     set_thread(thread_id)
     frames = []
-    current_number = 0
+    if levels == 0:
+        # Zero means all remaining frames.
+        high = -1
+    else:
+        # frame_iterator uses an inclusive range, so subtract one.
+        high = startFrame + levels - 1
     try:
-        current_frame = gdb.newest_frame()
+        frame_iter = frame_iterator(gdb.newest_frame(), startFrame, high)
     except gdb.error:
-        current_frame = None
-    while current_frame is not None and (levels == 0 or len(frames) < levels):
-        # This condition handles the startFrame==0 case as well.
-        if current_number >= startFrame:
-            newframe = {
-                "id": frame_id(current_frame),
-                "name": _frame_name(current_frame),
-                # This must always be supplied, but we will set it
-                # correctly later if that is possible.
-                "line": 0,
-                # GDB doesn't support columns.
-                "column": 0,
-                "instructionPointerReference": hex(current_frame.pc()),
+        frame_iter = ()
+    for current_frame in frame_iter:
+        newframe = {
+            "id": frame_id(current_frame),
+            "name": current_frame.function(),
+            # This must always be supplied, but we will set it
+            # correctly later if that is possible.
+            "line": 0,
+            # GDB doesn't support columns.
+            "column": 0,
+            "instructionPointerReference": hex(current_frame.address()),
+        }
+        line = current_frame.line()
+        if line is not None:
+            newframe["line"] = line
+        filename = current_frame.filename()
+        if filename is not None:
+            newframe["source"] = {
+                "name": os.path.basename(filename),
+                "path": filename,
+                # We probably don't need this but it doesn't hurt
+                # to be explicit.
+                "sourceReference": 0,
             }
-            sal = _safe_sal(current_frame)
-            if sal is not None and sal.symtab is not None:
-                newframe["source"] = {
-                    "name": os.path.basename(sal.symtab.filename),
-                    "path": sal.symtab.filename,
-                    # We probably don't need this but it doesn't hurt
-                    # to be explicit.
-                    "sourceReference": 0,
-                }
-                newframe["line"] = sal.line
-            frames.append(newframe)
-        current_number = current_number + 1
-        current_frame = current_frame.older()
+        frames.append(newframe)
     # Note that we do not calculate totalFrames here.  Its absence
     # tells the client that it may simply ask for frames until a
     # response yields fewer frames than requested.
diff --git a/gdb/python/lib/gdb/dap/evaluate.py b/gdb/python/lib/gdb/dap/evaluate.py
index 651a4046a34..456b03e39cc 100644
--- a/gdb/python/lib/gdb/dap/evaluate.py
+++ b/gdb/python/lib/gdb/dap/evaluate.py
@@ -19,7 +19,7 @@ import gdb.printing
 # This is deprecated in 3.9, but required in older versions.
 from typing import Optional
 
-from .frames import frame_for_id
+from .frames import select_frame
 from .server import capability, request, client_bool_capability
 from .startup import send_gdb_with_response, in_gdb_thread
 from .varref import find_variable, VariableReference
@@ -35,8 +35,7 @@ class EvaluateResult(VariableReference):
 def _evaluate(expr, frame_id):
     global_context = True
     if frame_id is not None:
-        frame = frame_for_id(frame_id)
-        frame.select()
+        select_frame(frame_id)
         global_context = False
     val = gdb.parse_and_eval(expr, global_context=global_context)
     ref = EvaluateResult(val)
@@ -58,8 +57,7 @@ def _eval_for_hover(expr, frame_id):
 def _set_expression(expression, value, frame_id):
     global_context = True
     if frame_id is not None:
-        frame = frame_for_id(frame_id)
-        frame.select()
+        select_frame(frame_id)
         global_context = False
     lhs = gdb.parse_and_eval(expression, global_context=global_context)
     rhs = gdb.parse_and_eval(value, global_context=global_context)
@@ -71,8 +69,7 @@ def _set_expression(expression, value, frame_id):
 @in_gdb_thread
 def _repl(command, frame_id):
     if frame_id is not None:
-        frame = frame_for_id(frame_id)
-        frame.select()
+        select_frame(frame_id)
     val = gdb.execute(command, from_tty=True, to_string=True)
     return {
         "result": val,
diff --git a/gdb/python/lib/gdb/dap/frames.py b/gdb/python/lib/gdb/dap/frames.py
index 08209d0b361..1d2d1371354 100644
--- a/gdb/python/lib/gdb/dap/frames.py
+++ b/gdb/python/lib/gdb/dap/frames.py
@@ -52,3 +52,10 @@ def frame_for_id(id):
     """Given a frame identifier ID, return the corresponding frame."""
     global _all_frames
     return _all_frames[id]
+
+
+@in_gdb_thread
+def select_frame(id):
+    """Given a frame identifier ID, select the corresponding frame."""
+    frame = frame_for_id(id)
+    frame.inferior_frame().select()
diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
index 9b80dd9ce80..1687094c4ce 100644
--- a/gdb/python/lib/gdb/dap/scopes.py
+++ b/gdb/python/lib/gdb/dap/scopes.py
@@ -21,41 +21,17 @@ from .server import request
 from .varref import BaseReference
 
 
-# Helper function to return a frame's block without error.
-@in_gdb_thread
-def _safe_block(frame):
-    try:
-        return frame.block()
-    except gdb.error:
-        return None
-
-
-# Helper function to return two lists of variables of block, up to the
-# enclosing function.  The result is of the form (ARGS, LOCALS), where
-# each element is itself a list.
-@in_gdb_thread
-def _block_vars(block):
-    args = []
-    locs = []
-    while True:
-        for var in block:
-            if var.is_argument:
-                args.append(var)
-            elif var.is_variable or var.is_constant:
-                locs.append(var)
-        if block.function is not None:
-            break
-        block = block.superblock
-    return (args, locs)
-
-
-class ScopeReference(BaseReference):
+class _ScopeReference(BaseReference):
     def __init__(self, name, hint, frame, var_list):
         super().__init__(name)
         self.hint = hint
         self.frame = frame
+        self.inf_frame = frame.inferior_frame()
         self.func = frame.function()
-        self.var_list = var_list
+        self.line = frame.line()
+        # VAR_LIST might be any kind of iterator, but it's convenient
+        # here if it is just a collection.
+        self.var_list = tuple(var_list)
 
     def to_object(self):
         result = super().to_object()
@@ -63,8 +39,8 @@ class ScopeReference(BaseReference):
         # How would we know?
         result["expensive"] = False
         result["namedVariables"] = len(self.var_list)
-        if self.func is not None:
-            result["line"] = self.func.line
+        if self.line is not None:
+            result["line"] = self.line
             # FIXME construct a Source object
         return result
 
@@ -73,38 +49,45 @@ class ScopeReference(BaseReference):
 
     @in_gdb_thread
     def fetch_one_child(self, idx):
+        # Here SYM will conform to the SymValueWrapper interface.
         sym = self.var_list[idx]
-        if sym.needs_frame:
-            val = sym.value(self.frame)
-        else:
-            val = sym.value()
-        return (sym.print_name, val)
-
-
-class RegisterReference(ScopeReference):
+        name = str(sym.symbol())
+        val = sym.value()
+        if val is None:
+            # No synthetic value, so must read the symbol value
+            # ourselves.
+            val = sym.symbol().value(self.inf_frame)
+        elif not isinstance(val, gdb.Value):
+            val = gdb.Value(val)
+        return (name, val)
+
+
+class _RegisterReference(_ScopeReference):
     def __init__(self, name, frame):
         super().__init__(
-            name, "registers", frame, list(frame.architecture().registers())
+            name, "registers", frame, frame.inferior_frame().architecture().registers()
         )
 
     @in_gdb_thread
     def fetch_one_child(self, idx):
-        return (self.var_list[idx].name, self.frame.read_register(self.var_list[idx]))
+        return (
+            self.var_list[idx].name,
+            self.inf_frame.read_register(self.var_list[idx]),
+        )
 
 
 # Helper function to create a DAP scopes for a given frame ID.
 @in_gdb_thread
 def _get_scope(id):
     frame = frame_for_id(id)
-    block = _safe_block(frame)
     scopes = []
-    if block is not None:
-        (args, locs) = _block_vars(block)
-        if args:
-            scopes.append(ScopeReference("Arguments", "arguments", frame, args))
-        if locs:
-            scopes.append(ScopeReference("Locals", "locals", frame, locs))
-    scopes.append(RegisterReference("Registers", frame))
+    args = frame.frame_args()
+    if args:
+        scopes.append(_ScopeReference("Arguments", "arguments", frame, args))
+    locs = frame.frame_locals()
+    if locs:
+        scopes.append(_ScopeReference("Locals", "locals", frame, locs))
+    scopes.append(_RegisterReference("Registers", frame))
     return [x.to_object() for x in scopes]
 
 

-- 
2.40.1

[PATCH v2 6/7] Handle typedefs in no-op pretty printers

[Tom Tromey]

The no-ops pretty-printers that were introduced for DAP have a classic
gdb bug: they neglect to call check_typedef.  This will cause some
strange behavior; for example not showing the children of a variable
whose type is a typedef of a structure type.  This patch fixes the
oversight.
---
 gdb/python/lib/gdb/printing.py | 23 ++++++++++++-----------
 gdb/testsuite/gdb.dap/scopes.c |  6 +++---
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/gdb/python/lib/gdb/printing.py b/gdb/python/lib/gdb/printing.py
index 353427d000a..a668bd0e3fc 100644
--- a/gdb/python/lib/gdb/printing.py
+++ b/gdb/python/lib/gdb/printing.py
@@ -282,9 +282,9 @@ class NoOpScalarPrinter:
 class NoOpArrayPrinter:
     """A no-op pretty printer that wraps an array value."""
 
-    def __init__(self, value):
+    def __init__(self, ty, value):
         self.value = value
-        (low, high) = self.value.type.range()
+        (low, high) = ty.range()
         self.low = low
         self.high = high
         # This is a convenience to the DAP code and perhaps other
@@ -305,14 +305,15 @@ class NoOpArrayPrinter:
 class NoOpStructPrinter:
     """A no-op pretty printer that wraps a struct or union value."""
 
-    def __init__(self, value):
+    def __init__(self, ty, value):
+        self.ty = ty
         self.value = value
 
     def to_string(self):
         return ""
 
     def children(self):
-        for field in self.value.type.fields():
+        for field in self.ty.fields():
             if field.name is not None:
                 yield (field.name, self.value[field])
 
@@ -327,14 +328,14 @@ def make_visualizer(value):
     if result is not None:
         # Found a pretty-printer.
         pass
-    elif value.type.code == gdb.TYPE_CODE_ARRAY:
-        result = gdb.printing.NoOpArrayPrinter(value)
-        (low, high) = value.type.range()
-        result.n_children = high - low + 1
-    elif value.type.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION):
-        result = gdb.printing.NoOpStructPrinter(value)
     else:
-        result = gdb.printing.NoOpScalarPrinter(value)
+        ty = value.type.strip_typedefs()
+        if ty.code == gdb.TYPE_CODE_ARRAY:
+            result = gdb.printing.NoOpArrayPrinter(ty, value)
+        elif ty.code in (gdb.TYPE_CODE_STRUCT, gdb.TYPE_CODE_UNION):
+            result = gdb.printing.NoOpStructPrinter(ty, value)
+        else:
+            result = gdb.printing.NoOpScalarPrinter(value)
     return result
 
 
diff --git a/gdb/testsuite/gdb.dap/scopes.c b/gdb/testsuite/gdb.dap/scopes.c
index ce87db1f13d..8f17c0d9039 100644
--- a/gdb/testsuite/gdb.dap/scopes.c
+++ b/gdb/testsuite/gdb.dap/scopes.c
@@ -17,13 +17,13 @@
 
 int main ()
 {
-  struct dei_struct
+  typedef struct dei_struct
   {
     int x;
     int more[5];
-  };
+  } dei_type;
 
-  struct dei_struct dei = { 2, { 3, 5, 7, 11, 13 } };
+  dei_type dei = { 2, { 3, 5, 7, 11, 13 } };
 
   static int scalar = 23;
 

-- 
2.40.1

[PATCH v2 7/7] Add Ada scope test for DAP

[Tom Tromey]

This adds a DAP test for fetching scopes and variables with an Ada
program.  This test is the reason that the FrameVars code does not
check is_constant on the symbols it returns.

Note that this test also shows that string-printing is incorrect in
Ada.  This is a known bug but I'm still considering how to fix it.
---
 gdb/testsuite/gdb.dap/ada-scopes.exp      | 84 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.dap/ada-scopes/pack.adb | 23 +++++++++
 gdb/testsuite/gdb.dap/ada-scopes/pack.ads | 21 ++++++++
 gdb/testsuite/gdb.dap/ada-scopes/prog.adb | 26 ++++++++++
 4 files changed, 154 insertions(+)

diff --git a/gdb/testsuite/gdb.dap/ada-scopes.exp b/gdb/testsuite/gdb.dap/ada-scopes.exp
new file mode 100644
index 00000000000..75d51f96ab4
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/ada-scopes.exp
@@ -0,0 +1,84 @@
+# 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/>.
+
+load_lib ada.exp
+load_lib dap-support.exp
+
+require allow_ada_tests allow_dap_tests
+
+standard_ada_testfile prog
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable \
+	 {debug additional_flags=-gnata}] != ""} {
+    return -1
+}
+
+if {[dap_launch $binfile] == ""} {
+    return
+}
+
+set line [gdb_get_line_number "STOP"]
+set obj [dap_check_request_and_response "set breakpoint" \
+	     setBreakpoints \
+	     [format {o source [o path [%s]] \
+			  breakpoints [a [o line [i %d]]]} \
+		  [list s $srcfile] $line]]
+set fn_bpno [dap_get_breakpoint_number $obj]
+
+dap_check_request_and_response "start inferior" configurationDone
+
+dap_wait_for_event_and_check "stopped at breakpoint" stopped \
+    "body reason" breakpoint \
+    "body hitBreakpointIds" $fn_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]
+
+# This is what the implementation does, so we can assume it, but check
+# just in case something changes.
+lassign $scopes scope _ignore
+gdb_assert {[dict get $scope name] == "Locals"} "scope is locals"
+
+gdb_assert {[dict get $scope namedVariables] == 2} "two vars in 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 2]} \
+			   $num]] \
+	      0]
+
+foreach var [dict get $refs body variables] {
+    set name [dict get $var name]
+
+    switch $name {
+	"value" {
+	    gdb_assert {[dict get $var value] == "three"} "check value of value"
+	}
+	"my_string" {
+	}
+	default {
+	    fail "unknown variable $name"
+	}
+    }
+}
+
+dap_shutdown
diff --git a/gdb/testsuite/gdb.dap/ada-scopes/pack.adb b/gdb/testsuite/gdb.dap/ada-scopes/pack.adb
new file mode 100644
index 00000000000..a97a8293b0e
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/ada-scopes/pack.adb
@@ -0,0 +1,23 @@
+--  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/>.
+
+package body Pack is
+
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pack;
diff --git a/gdb/testsuite/gdb.dap/ada-scopes/pack.ads b/gdb/testsuite/gdb.dap/ada-scopes/pack.ads
new file mode 100644
index 00000000000..3a6721d62d2
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/ada-scopes/pack.ads
@@ -0,0 +1,21 @@
+--  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/>.
+
+with System;
+package Pack is
+
+   procedure Do_Nothing (A : System.Address);
+
+end Pack;
diff --git a/gdb/testsuite/gdb.dap/ada-scopes/prog.adb b/gdb/testsuite/gdb.dap/ada-scopes/prog.adb
new file mode 100644
index 00000000000..5f6ccd5ee66
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/ada-scopes/prog.adb
@@ -0,0 +1,26 @@
+--  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/>.
+
+with Pack; use Pack;
+
+procedure Foo is
+   type Enum_Type is (one, two, three);
+   Value : Enum_Type := three;
+
+   My_String : constant String := "Hello World";
+begin
+   Do_Nothing (Value'address);
+   Do_Nothing (My_String'address);  -- STOP
+end Foo;

-- 
2.40.1

Re: [PATCH v2 4/7] Simplify FrameVars

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> FrameVars implements its own variant of Symbol.is_variable and/or
Tom> Symbol.is_control.  This patch replaces this code.

Tom> -        # SYM may be a string instead of a symbol in the case of
Tom> -        # synthetic local arguments or locals.  If that is the case,
Tom> -        # always fetch.
Tom> -        if isinstance(sym, str):
Tom> -            return True

I noticed that I removed this check.

However, looking into it, I think there's no way for this to happen,
because FrameVars is only ever instantiated with a real gdb.Frame, and
these won't return synthetic locals.

Tom

Re: [PATCH v2 0/7] Reimplement DAP backtrace using frame filters

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> - Removed is_constant check from FrameVars

This particular change caused a regression in the relatively new DAP
"scopes" test, because that test has:

  static int scalar = 23;

... which is not used at all.  This causes GCC to emit it as a constant.


Now, I tend to think this change is a good one, because I think
including enumeration constants in the locals output is not very useful,
and because the above scenario isn't one that is likely to happen in
real code.

Another solution might be to find a way to eliminate enumeration
constants from the output without affecting other local constants.

Meanwhile, locally I've updated the scopes test to use the variable in
question.

But, do let me know what you think.  It would be convenient if I could
land my remaining DAP patches.

Tom

Re: [PATCH v2 0/7] Reimplement DAP backtrace using frame filters

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> This series reimplements DAP backtraces using frame filters.  This
Tom> seemed nice to have, because it would present users with a friendlier
Tom> view into the stack.  It also turned out to simplify the code a bit.

I'm checking this in.

Tom