[2/2] Add return value to DAP scope

Message ID 20240212-dap-finish-v1-2-8e30870e8b25@adacore.com
State New
Headers
Series Add "finish" return value to DAP |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom Tromey Feb. 12, 2024, 6:47 p.m. UTC
  A bug report in the DAP specification repository pointed out that it
is typical for DAP implementations to put a function's return value
into the outermost scope.

This patch changes gdb to follow this convention.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31341
---
 gdb/python/lib/gdb/dap/events.py   |  3 ++
 gdb/python/lib/gdb/dap/scopes.py   | 45 ++++++++++++++++++++-
 gdb/testsuite/gdb.dap/step-out.c   | 36 +++++++++++++++++
 gdb/testsuite/gdb.dap/step-out.exp | 82 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 164 insertions(+), 2 deletions(-)
  

Comments

Kévin Le Gouguec Feb. 13, 2024, 3:01 p.m. UTC | #1
Tom Tromey <tromey@adacore.com> writes:

> diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
> index ff553259ea9..f3bbe4fc614 100644
> --- a/gdb/python/lib/gdb/dap/scopes.py
> +++ b/gdb/python/lib/gdb/dap/scopes.py
> @@ -25,17 +25,32 @@ from .varref import BaseReference
>  frame_to_scope = {}
>  
>  
> +# If the most recent stop was due to a 'finish', and the return value
> +# could be computed, then this holds that value.  Otherwise it holds
> +# None.
> +_last_return_value = None
> +
> +
>  # When the inferior is re-started, we erase all scope references.  See
>  # the section "Lifetime of Objects References" in the spec.
>  @in_gdb_thread
>  def clear_scopes(event):
>      global frame_to_scope
>      frame_to_scope = {}
> +    global _last_return_value
> +    _last_return_value = None

[… some lines elided …]
> +    def fetch_one_child(self, idx):
> +        global _last_return_value
> +        if _last_return_value is not None:
> +            if idx == 0:
> +                return ("(return)", _last_return_value)
> +            idx = idx - 1
> +        return super().fetch_one_child(idx)

If scopes are cleared whenever the inferior is resumed (my paraphrasing
of § Lifetime of Objects References), can this method be called after
_last_return_value has been reset to None?

Wondering if the outer 'if' is necessary, since we only create a
_FinishScopeReference when _last_return_value is not None:

> @@ -120,10 +158,13 @@ def scopes(*, frameId: int, **extra):
>          args = tuple(frame.frame_args() or ())
>          if args:
>              scopes.append(_ScopeReference("Arguments", "arguments", frame, args))
> +        has_return_value = frameId == 0 and _last_return_value is not None
>          # Make sure to handle the None case as well as the empty
>          # iterator case.
>          locs = tuple(frame.frame_locals() or ())
> -        if locs:
> +        if has_return_value:
> +            scopes.append(_FinishScopeReference("Locals", "locals", frame, locs))
> +        elif locs:
>              scopes.append(_ScopeReference("Locals", "locals", frame, locs))
>          scopes.append(_RegisterReference("Registers", frame))
>          frame_to_scope[frameId] = scopes

Tangentially, wondering if capturing _last_return_value as a
_FinishScopeReference.__init__ parameter would make sense.

It may well be that I don't completely understand the lifetime of the
objects involved here.  At any rate:

Reviewed-By: Kévin Le Gouguec <legouguec@adacore.com>
  
Tom Tromey Feb. 13, 2024, 4:51 p.m. UTC | #2
>>>>> "Kévin" == Kévin Le Gouguec <legouguec@adacore.com> writes:

>> +    def fetch_one_child(self, idx):
>> +        global _last_return_value
>> +        if _last_return_value is not None:
>> +            if idx == 0:
>> +                return ("(return)", _last_return_value)
>> +            idx = idx - 1
>> +        return super().fetch_one_child(idx)

Kévin> If scopes are cleared whenever the inferior is resumed (my paraphrasing
Kévin> of § Lifetime of Objects References), can this method be called after
Kévin> _last_return_value has been reset to None?

Kévin> Wondering if the outer 'if' is necessary, since we only create a
Kévin> _FinishScopeReference when _last_return_value is not None:

Ah yeah -- that 'if' is a leftover from an earlier iteration.  Thanks.

Kévin> Tangentially, wondering if capturing _last_return_value as a
Kévin> _FinishScopeReference.__init__ parameter would make sense.

It doesn't really matter, because all these objects are only valid
during a single stop.

Tom
  

Patch

diff --git a/gdb/python/lib/gdb/dap/events.py b/gdb/python/lib/gdb/dap/events.py
index 41302229ee5..928f23fd3ff 100644
--- a/gdb/python/lib/gdb/dap/events.py
+++ b/gdb/python/lib/gdb/dap/events.py
@@ -15,6 +15,7 @@ 
 
 import gdb
 
+from .scopes import set_finish_value
 from .server import send_event
 from .startup import exec_and_log, in_gdb_thread, log
 from .modules import is_module, make_module
@@ -218,6 +219,8 @@  def _on_stop(event):
     }
     if isinstance(event, gdb.BreakpointEvent):
         obj["hitBreakpointIds"] = [x.number for x in event.breakpoints]
+    if hasattr(event, "details") and "finish-value" in event.details:
+        set_finish_value(event.details["finish-value"])
 
     global _expected_pause
     global _expected_stop_reason
diff --git a/gdb/python/lib/gdb/dap/scopes.py b/gdb/python/lib/gdb/dap/scopes.py
index ff553259ea9..f3bbe4fc614 100644
--- a/gdb/python/lib/gdb/dap/scopes.py
+++ b/gdb/python/lib/gdb/dap/scopes.py
@@ -25,17 +25,32 @@  from .varref import BaseReference
 frame_to_scope = {}
 
 
+# If the most recent stop was due to a 'finish', and the return value
+# could be computed, then this holds that value.  Otherwise it holds
+# None.
+_last_return_value = None
+
+
 # When the inferior is re-started, we erase all scope references.  See
 # the section "Lifetime of Objects References" in the spec.
 @in_gdb_thread
 def clear_scopes(event):
     global frame_to_scope
     frame_to_scope = {}
+    global _last_return_value
+    _last_return_value = None
 
 
 gdb.events.cont.connect(clear_scopes)
 
 
+@in_gdb_thread
+def set_finish_value(val):
+    """Set the current 'finish' value on a stop."""
+    global _last_return_value
+    _last_return_value = val
+
+
 # A helper function to compute the value of a symbol.  SYM is either a
 # gdb.Symbol, or an object implementing the SymValueWrapper interface.
 # FRAME is a frame wrapper, as produced by a frame filter.  Returns a
@@ -76,7 +91,7 @@  class _ScopeReference(BaseReference):
         result["presentationHint"] = self.hint
         # How would we know?
         result["expensive"] = False
-        result["namedVariables"] = len(self.var_list)
+        result["namedVariables"] = self.child_count()
         if self.line is not None:
             result["line"] = self.line
             # FIXME construct a Source object
@@ -93,6 +108,28 @@  class _ScopeReference(BaseReference):
         return symbol_value(self.var_list[idx], self.frame)
 
 
+# A _ScopeReference that prepends the most recent return value, if one
+# is available.
+class _FinishScopeReference(_ScopeReference):
+    def __init__(self, *args):
+        super().__init__(*args)
+
+    def child_count(self):
+        result = super().child_count()
+        global _last_return_value
+        if _last_return_value is not None:
+            result = result + 1
+        return result
+
+    def fetch_one_child(self, idx):
+        global _last_return_value
+        if _last_return_value is not None:
+            if idx == 0:
+                return ("(return)", _last_return_value)
+            idx = idx - 1
+        return super().fetch_one_child(idx)
+
+
 class _RegisterReference(_ScopeReference):
     def __init__(self, name, frame):
         super().__init__(
@@ -109,6 +146,7 @@  class _RegisterReference(_ScopeReference):
 
 @request("scopes")
 def scopes(*, frameId: int, **extra):
+    global _last_return_value
     global frame_to_scope
     if frameId in frame_to_scope:
         scopes = frame_to_scope[frameId]
@@ -120,10 +158,13 @@  def scopes(*, frameId: int, **extra):
         args = tuple(frame.frame_args() or ())
         if args:
             scopes.append(_ScopeReference("Arguments", "arguments", frame, args))
+        has_return_value = frameId == 0 and _last_return_value is not None
         # Make sure to handle the None case as well as the empty
         # iterator case.
         locs = tuple(frame.frame_locals() or ())
-        if locs:
+        if has_return_value:
+            scopes.append(_FinishScopeReference("Locals", "locals", frame, locs))
+        elif locs:
             scopes.append(_ScopeReference("Locals", "locals", frame, locs))
         scopes.append(_RegisterReference("Registers", frame))
         frame_to_scope[frameId] = scopes
diff --git a/gdb/testsuite/gdb.dap/step-out.c b/gdb/testsuite/gdb.dap/step-out.c
new file mode 100644
index 00000000000..8c7e6942342
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/step-out.c
@@ -0,0 +1,36 @@ 
+/* 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/>.  */
+
+struct result
+{
+  int x;
+};
+
+struct result
+function_breakpoint_here (int q)
+{
+  struct result val;
+  val.x = q;
+  return val;
+}
+
+int
+main ()
+{
+  int q = 23;
+  return function_breakpoint_here (q).x - q;
+}
diff --git a/gdb/testsuite/gdb.dap/step-out.exp b/gdb/testsuite/gdb.dap/step-out.exp
new file mode 100644
index 00000000000..757f4ebdaca
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/step-out.exp
@@ -0,0 +1,82 @@ 
+# 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 that stepOut puts the return value in scope.
+
+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 on function" \
+	     setFunctionBreakpoints \
+	     {o breakpoints [a [o name [s function_breakpoint_here]]]}]
+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 function breakpoint" stopped \
+    "body reason" breakpoint \
+    "body hitBreakpointIds" $fn_bpno
+
+dap_check_request_and_response "return from function" stepOut \
+    {o threadId [i 1]}
+dap_wait_for_event_and_check "stopped after return" stopped \
+    "body reason" step
+
+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]
+
+gdb_assert {[llength $scopes] == 2} "two scopes"
+
+lassign $scopes scope reg_scope
+gdb_assert {[dict get $scope name] == "Locals"} "scope is locals"
+gdb_assert {[dict get $scope presentationHint] == "locals"} \
+    "locals presentation hint"
+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 arguments" \
+		      "variables" \
+		      [format {o variablesReference [i %d]} $num]] \
+	      0]
+set varlist [lindex [dict get $refs body variables] 0]
+
+gdb_assert {[dict get $varlist variablesReference] > 0} \
+    "variable has children"
+gdb_assert {[dict get $varlist name] == "(return)"} \
+    "variable is return value"
+
+dap_shutdown