Re: [PATCH 12/24] gdb: make get_frame_register_bytes take the next frame

["Kévin Le Gouguec" <legouguec@adacore.com>]

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

Hey Simon!

Simon Marchi <simon.marchi@efficios.com> writes:

> Similar to the previous patches, change get_frame_register_bytes to take
> the "next frame" instead of "this frame".
>
> Change-Id: Ie8f35042bfa6e93565fcefaee71b6b3903f0fe9f
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>
> ---
>  gdb/dwarf2/expr.c | 22 +++++++++++-----------
>  gdb/frame.c       | 16 ++++++----------
>  gdb/frame.h       | 10 +++++-----
>  gdb/i386-tdep.c   |  8 ++++----
>  gdb/i387-tdep.c   | 10 +++++-----
>  gdb/ia64-tdep.c   |  9 ++++-----
>  gdb/m68k-tdep.c   | 10 +++++-----
>  gdb/mips-tdep.c   |  7 ++++---
>  gdb/rs6000-tdep.c | 10 +++++-----
>  gdb/valops.c      |  3 +--
>  10 files changed, 50 insertions(+), 55 deletions(-)
>
[…]
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index c43039302bc3..643997e5bd7d 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -2713,11 +2713,11 @@ rs6000_register_to_value (frame_info_ptr frame,
>       fpr to vsr.  */
>    regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
>  
> -  if (!get_frame_register_bytes (frame, regnum, 0,
> -				 gdb::make_array_view (from,
> -						       register_size (gdbarch,
> -								      regnum)),
> -				 optimizedp, unavailablep))
> +  auto from_view
> +    = gdb::make_array_view (from, register_size (gdbarch, regnum));
> +  frame_info_ptr next_frame = get_next_frame_sentinel_okay (frame);
> +  if (!get_frame_register_bytes (frame, regnum, 0, from_view, optimizedp,
                                    ^^^^^

Should this have been changed to 'next_frame', as the commit message
suggests, and as was done in the other *-tdep.c?

Asking because I'm observing a regression on ppc-linux in our internal
testsuite.  I've reproduced that regression with the O2_float_param
testcase:

  cd gdb/testsuite/gdb.ada/O2_float_param
  powerpc-linux-gnu-gnatmake foo.adb \
      -cargs -g -O2 -bargs -static -largs -static
  scp foo $TARGET:
  […gdb…] foo                                   \
      -batch                                    \
      -ex 'tbreak callee.increment'             \
      -ex 'target remote | ssh gdbserver - foo' \
      -ex continue

Expected:

  > Temporary breakpoint 1, callee.increment (val=val@entry=99.0, msg=...) at callee.adb:19

Currently, since 2023-12-14 "gdb: make get_frame_register_bytes take the
next frame" (9fc79b42369):

  > Temporary breakpoint 1, callee.increment (val=<optimized out>, val@entry=0.0, msg=...) at callee.adb:19

Applying the attached patch gets us the expected output back, and does
not introduce further regressions that I could find.  I've tried it on
top of both master and your recently submitted "register value cleanups"
series, <20231221191716.257256-1-simon.marchi@efficios.com>.

Let me know if I've missed something!

> +				 unavailablep))
>      return 0;
>  
>    target_float_convert (from, builtin_type (gdbarch)->builtin_double,


[-- Attachment #2: 0001-gdb-fix-refactoring-hiccup-in-rs6000_register_to_val.patch --]
[-- Type: text/x-diff, Size: 1584 bytes --]

From 0fae84368f22250cb672c467e56652103fbb8c49 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <legouguec@adacore.com>
Date: Fri, 22 Dec 2023 14:06:15 +0100
Subject: [PATCH] gdb: fix refactoring hiccup in rs6000_register_to_value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In 2023-12-14 "gdb: make get_frame_register_bytes take the next frame"
(9fc79b42369), *_register_to_value functions were made to (a) call
get_next_frame_sentinel_okay (frame) (b) pass that next frame to
get_frame_register_bytes.

Step (b) was omitted for rs6000-tdep.c; this manifests as a regression on
PPC platforms for e.g. O2_float_param: instead of seeing…

  Temporary breakpoint 1, callee.increment (val=val@entry=99.0, msg=...) at callee.adb:19

… we get "optimized_out" for val.  Passing next_frame to
get_frame_register_bytes fixes the issue.
---
 gdb/rs6000-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 1c9bb3e5f04..f043be08355 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2716,7 +2716,7 @@ rs6000_register_to_value (frame_info_ptr frame,
   auto from_view
     = gdb::make_array_view (from, register_size (gdbarch, regnum));
   frame_info_ptr next_frame = get_next_frame_sentinel_okay (frame);
-  if (!get_frame_register_bytes (frame, regnum, 0, from_view, optimizedp,
+  if (!get_frame_register_bytes (next_frame, regnum, 0, from_view, optimizedp,
 				 unavailablep))
     return 0;
 
-- 
2.34.1