diff mbox series

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

Message ID 20231201162751.741751-13-simon.marchi@efficios.com
State New
Headers
Series Fix reading and writing pseudo registers in non-current frames |

Checks

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

Commit Message

Simon Marchi Dec. 1, 2023, 4:27 p.m. UTC 
  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(-)

Comments

Kévin Le Gouguec Dec. 22, 2023, 1:33 p.m. UTC | #1 
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,
Simon Marchi Dec. 22, 2023, 3:31 p.m. UTC | #2 
Hi Kevin,

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

Yes, you're right.

> 
> 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!

Your patch LGTM, thanks for finding and fixing it.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Kévin Le Gouguec Dec. 22, 2023, 4:22 p.m. UTC | #3 
Simon Marchi <simon.marchi@efficios.com> writes:

>> 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!
>
> Your patch LGTM, thanks for finding and fixing it.

Thank you for confirming!  Pushed.
diff mbox series

Patch

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 7fc7b3abf5ca..c71bbb594131 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -204,8 +204,9 @@  rw_pieced_value (value *v, value *from, bool check_optimized)
 	{
 	case DWARF_VALUE_REGISTER:
 	  {
-	    frame_info_ptr frame = frame_find_by_id (c->frame_id);
-	    gdbarch *arch = get_frame_arch (frame);
+	    frame_info_ptr next_frame
+	      = get_next_frame_sentinel_okay (frame_find_by_id (c->frame_id));
+	    gdbarch *arch = frame_unwind_arch (next_frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
 	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 	    int optim, unavail;
@@ -225,9 +226,9 @@  rw_pieced_value (value *v, value *from, bool check_optimized)
 	    if (from == nullptr)
 	      {
 		/* Read mode.  */
-		if (!get_frame_register_bytes (frame, gdb_regnum,
-					       bits_to_skip / 8,
-					       buffer, &optim, &unavail))
+		if (!get_frame_register_bytes (next_frame, gdb_regnum,
+					       bits_to_skip / 8, buffer,
+					       &optim, &unavail))
 		  {
 		    if (optim)
 		      {
@@ -254,9 +255,9 @@  rw_pieced_value (value *v, value *from, bool check_optimized)
 		  {
 		    /* Data is copied non-byte-aligned into the register.
 		       Need some bits from original register value.  */
-		    get_frame_register_bytes (frame, gdb_regnum,
-					      bits_to_skip / 8,
-					      buffer, &optim, &unavail);
+		    get_frame_register_bytes (next_frame, gdb_regnum,
+					      bits_to_skip / 8, buffer, &optim,
+					      &unavail);
 		    if (optim)
 		      throw_error (OPTIMIZED_OUT_ERROR,
 				   _("Can't do read-modify-write to "
@@ -272,9 +273,8 @@  rw_pieced_value (value *v, value *from, bool check_optimized)
 		copy_bitwise (buffer.data (), bits_to_skip % 8,
 			      from_contents, offset,
 			      this_size_bits, bits_big_endian);
-		put_frame_register_bytes
-		  (get_next_frame_sentinel_okay (frame), gdb_regnum,
-		   bits_to_skip / 8, buffer);
+		put_frame_register_bytes (next_frame, gdb_regnum,
+					  bits_to_skip / 8, buffer);
 	      }
 	  }
 	  break;
diff --git a/gdb/frame.c b/gdb/frame.c
index 982aaa6d6753..d260e8c28f3f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1456,12 +1456,11 @@  deprecated_frame_register_read (frame_info_ptr frame, int regnum,
 }
 
 bool
-get_frame_register_bytes (frame_info_ptr frame, int regnum,
-			  CORE_ADDR offset,
-			  gdb::array_view<gdb_byte> buffer,
+get_frame_register_bytes (frame_info_ptr next_frame, int regnum,
+			  CORE_ADDR offset, gdb::array_view<gdb_byte> buffer,
 			  int *optimizedp, int *unavailablep)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
+  gdbarch *gdbarch = frame_unwind_arch (next_frame);
 
   /* Skip registers wholly inside of OFFSET.  */
   while (offset >= register_size (gdbarch, regnum))
@@ -1499,17 +1498,14 @@  get_frame_register_bytes (frame_info_ptr frame, int regnum,
 	  CORE_ADDR addr;
 	  int realnum;
 
-	  frame_register_unwind (get_next_frame_sentinel_okay (frame), regnum,
-				 optimizedp, unavailablep, &lval, &addr,
-				 &realnum, buffer.data ());
+	  frame_register_unwind (next_frame, regnum, optimizedp, unavailablep,
+				 &lval, &addr, &realnum, buffer.data ());
 	  if (*optimizedp || *unavailablep)
 	    return false;
 	}
       else
 	{
-	  struct value *value
-	    = frame_unwind_register_value (frame_info_ptr (frame->next),
-					   regnum);
+	  value *value = frame_unwind_register_value (next_frame, regnum);
 	  gdb_assert (value != NULL);
 	  *optimizedp = value->optimized_out ();
 	  *unavailablep = !value->entirely_available ();
diff --git a/gdb/frame.h b/gdb/frame.h
index 5ce3a1b285aa..62f53ece9eaa 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -733,11 +733,11 @@  extern bool read_frame_register_unsigned (frame_info_ptr frame,
 extern void put_frame_register (frame_info_ptr next_frame, int regnum,
 				gdb::array_view<const gdb_byte> buf);
 
-/* Read LEN bytes from one or multiple registers starting with REGNUM
-   in frame FRAME, starting at OFFSET, into BUF.  If the register
-   contents are optimized out or unavailable, set *OPTIMIZEDP,
-   *UNAVAILABLEP accordingly.  */
-extern bool get_frame_register_bytes (frame_info_ptr frame, int regnum,
+/* Read LEN bytes from one or multiple registers starting with REGNUM in
+   NEXT_FRAME's previous frame, starting at OFFSET, into BUF.  If the register
+   contents are optimized out or unavailable, set *OPTIMIZEDP, *UNAVAILABLEP
+   accordingly.  */
+extern bool get_frame_register_bytes (frame_info_ptr next_frame, int regnum,
 				      CORE_ADDR offset,
 				      gdb::array_view<gdb_byte> buffer,
 				      int *optimizedp, int *unavailablep);
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index f188f83de6f3..59d77d1d574d 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3877,10 +3877,10 @@  i386_register_to_value (frame_info_ptr frame, int regnum,
       gdb_assert (regnum != -1);
       gdb_assert (register_size (gdbarch, regnum) == 4);
 
-      if (!get_frame_register_bytes (frame, regnum, 0,
-				     gdb::make_array_view (to,
-							register_size (gdbarch,
-								       regnum)),
+      auto to_view
+	= gdb::make_array_view (to, register_size (gdbarch, regnum));
+      frame_info_ptr next_frame = get_next_frame_sentinel_okay (frame);
+      if (!get_frame_register_bytes (next_frame, regnum, 0, to_view,
 				     optimizedp, unavailablep))
 	return 0;
 
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 77dc5a008d5f..ffd8303746e5 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -364,11 +364,11 @@  i387_register_to_value (frame_info_ptr frame, int regnum,
     }
 
   /* Convert to TYPE.  */
-  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 (next_frame, regnum, 0, from_view, optimizedp,
+				 unavailablep))
     return 0;
 
   target_float_convert (from, i387_ext_type (gdbarch), to, type);
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 3bede2644c03..b009b46ef6df 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -1227,11 +1227,10 @@  ia64_register_to_value (frame_info_ptr frame, int regnum,
   gdb_byte in[IA64_FP_REGISTER_SIZE];
 
   /* Convert to TYPE.  */
-  if (!get_frame_register_bytes (frame, regnum, 0,
-				 gdb::make_array_view (in,
-						       register_size (gdbarch,
-								      regnum)),
-				 optimizedp, unavailablep))
+  auto in_view = gdb::make_array_view (in, register_size (gdbarch, regnum));
+  frame_info_ptr next_frame = get_next_frame_sentinel_okay (frame);
+  if (!get_frame_register_bytes (next_frame, regnum, 0, in_view, optimizedp,
+				 unavailablep))
     return 0;
 
   target_float_convert (in, ia64_ext_type (gdbarch), out, valtype);
diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 8bbfa4bd0b10..f9c734cf3808 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -216,11 +216,11 @@  m68k_register_to_value (frame_info_ptr frame, int regnum,
   gdb_assert (type->code () == TYPE_CODE_FLT);
 
   /* Convert to TYPE.  */
-  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 (next_frame, regnum, 0, from_view, optimizedp,
+				 unavailablep))
     return 0;
 
   target_float_convert (from, fpreg_type, to, type);
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 9c0cfede492a..fc3193ea27a5 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -947,17 +947,18 @@  mips_register_to_value (frame_info_ptr frame, int regnum,
 			int *optimizedp, int *unavailablep)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
+  frame_info_ptr next_frame = get_next_frame_sentinel_okay (frame);
 
   if (mips_convert_register_float_case_p (gdbarch, regnum, type))
     {
       get_frame_register (frame, regnum + 0, to + 4);
       get_frame_register (frame, regnum + 1, to + 0);
 
-      if (!get_frame_register_bytes (frame, regnum + 0, 0, {to + 4, 4},
+      if (!get_frame_register_bytes (next_frame, regnum + 0, 0, { to + 4, 4 },
 				     optimizedp, unavailablep))
 	return 0;
 
-      if (!get_frame_register_bytes (frame, regnum + 1, 0, {to + 0, 4},
+      if (!get_frame_register_bytes (next_frame, regnum + 1, 0, { to + 0, 4 },
 				     optimizedp, unavailablep))
 	return 0;
       *optimizedp = *unavailablep = 0;
@@ -969,7 +970,7 @@  mips_register_to_value (frame_info_ptr frame, int regnum,
       CORE_ADDR offset;
 
       offset = gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG ? 8 - len : 0;
-      if (!get_frame_register_bytes (frame, regnum, offset, {to, len},
+      if (!get_frame_register_bytes (next_frame, regnum, offset, { to, len },
 				     optimizedp, unavailablep))
 	return 0;
 
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,
+				 unavailablep))
     return 0;
 
   target_float_convert (from, builtin_type (gdbarch)->builtin_double,
diff --git a/gdb/valops.c b/gdb/valops.c
index 8e1849837cf1..dbf7a7552b10 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1225,8 +1225,7 @@  value_assign (struct value *toval, struct value *fromval)
 		       "don't fit in a %d bit word."),
 		     (int) sizeof (LONGEST) * HOST_CHAR_BIT);
 
-	    if (!get_frame_register_bytes (get_prev_frame_always (next_frame),
-					   value_reg, offset,
+	    if (!get_frame_register_bytes (next_frame, value_reg, offset,
 					   { buffer, changed_len }, &optim,
 					   &unavail))
 	      {