[2/2] PowerPC, Fix-test-gdb.base-store.exp

  GDB maintainers:

This is the second patch which fixes the 128-bit floating point
register mappings.

             Carl 
--------------------------------------------------------
Fix test gdb.base/store.exp

The test currently fails for IEEE 128-bit floating point types.  PowerPC
supports the IBM double 128-bit floating point	format and IEEE 128-bit
format.  The IBM double 128-bit floating point format uses two 64-bit
floating point registers to store the 128-bit value.  The IEEE 128-bit
floating point format stores the value in a single 128-bit vector-scalar
register (vsr).

The various floating point values, 32-bit float, 64-bit double, IBM double
128-bit float and IEEE 128-bit floating point numbers are all mapped to the
DWARF fpr numbers.  The issue is the IEEE 128-bit floating point values are
actually stored in a vsr not the fprs.  This patch changes the register
mapping for the vsrs from the fpr to the vsr registers so the value is
properly accessed by GDB.  The functions rs6000_linux_register_to_value,
rs6000_linux_value_to_register, rs6000_linux_value_from_register check if
the value is an IEEE 128-bit floating point value and adjust the register
number as needed.  The test in function rs6000_convert_register_p is fixed
to so it is only true for floating point values.

This patch fixes three regression tests in gdb.base/store.exp.

The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE
with no regressions.

Change to inline function
---
 gdb/ppc-linux-tdep.c |  4 +++
 gdb/rs6000-tdep.c    | 66 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 1 deletion(-)
  

Carl Love wrote:

As in your previous patch, I only have some really trivial nits to point out.
So treat similarly, and thank you for the patch.

> The test currently fails for IEEE 128-bit floating point types.  PowerPC
> supports the IBM double 128-bit floating point	format and IEEE 128-bit

Looks like a tab got inserted above?

> format.  The IBM double 128-bit floating point format uses two 64-bit
> floating point registers to store the 128-bit value.  The IEEE 128-bit
> floating point format stores the value in a single 128-bit vector-scalar
> register (vsr).

> The various floating point values, 32-bit float, 64-bit double, IBM double
> 128-bit float and IEEE 128-bit floating point numbers are all mapped to the
> DWARF fpr numbers.  The issue is the IEEE 128-bit floating point values are
> actually stored in a vsr not the fprs.  This patch changes the register
> mapping for the vsrs from the fpr to the vsr registers so the value is
> properly accessed by GDB.  The functions rs6000_linux_register_to_value,
> rs6000_linux_value_to_register, rs6000_linux_value_from_register check if
> the value is an IEEE 128-bit floating point value and adjust the register
> number as needed.  The test in function rs6000_convert_register_p is fixed
> to so it is only true for floating point values.

"fixed [to] so"

> This patch fixes three regression tests in gdb.base/store.exp.
>
> The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE
> with no regressions.

Indeed!

Tested-by: Keith Seitz <keiths@redhat.com>

> Change to inline function

Stray line or was there something more to communicate?

Keith
---
 gdb/ppc-linux-tdep.c |  4 +++
 gdb/rs6000-tdep.c    | 66 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 7fb90799dff..ba646a7230f 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -63,6 +63,7 @@
 #include <ctype.h>
 #include "elf-bfd.h"
 #include "producer.h"
+#include "target-float.h"
 
 #include "features/rs6000/powerpc-32l.c"
 #include "features/rs6000/powerpc-altivec32l.c"
@@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num)
     /* FIXME: jimb/2004-05-05: What should we do when the debug info
        specifies registers the architecture doesn't have?  Our
        callers don't check the value we return.  */
+    /* Map dwarf register numbers for floating point, double, IBM double and
+       IEEE 128-bit floating point to the fpr range.  Will have to fix the
+       mapping for the IEEE 128-bit register numbers later.  */

I suggest rewording this last incomplete sentence to something like: "The mapping
for the IEEE 128-bit register numbers will have to be fixed later."

return tdep->ppc_fp0_regnum + (num - 32);
   else if (77 <= num && num < 77 + 32)
     return tdep->ppc_vr0_regnum + (num - 77);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 23397d037ae..ada9cea3353 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum,
 	  && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs
 	  && type->code () == TYPE_CODE_FLT
 	  && (type->length ()
-	      != builtin_type (gdbarch)->builtin_double->length ()));
+	      == builtin_type (gdbarch)->builtin_float->length ()));
+}
+
+static int
+ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type,
+			      int regnum)
+{
+  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
+
+  /* If we have the an IEEE 128-bit floating point value, need to map the
+   register number to the corresponding VSR.  */

"If we have the an" --> "If we have an". "need to" --> ""

+  if (tdep->ppc_vsr0_regnum != -1
+      && regnum >= tdep->ppc_fp0_regnum
+      && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs)
+      && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad)
+      && (type->length() == 16))
+    regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum;
+
+  return regnum;
 }
 
 static int
@@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame,
   
   gdb_assert (type->code () == TYPE_CODE_FLT);
 
+  /* We have an IEEE 128-bit float need to change regnum mapping from
+     fpr to vsr.  */
+  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
+

Add "--" before "need". Similarly below in a few spots.

if (!get_frame_register_bytes (frame, regnum, 0,
 				 gdb::make_array_view (from,
 						       register_size (gdbarch,
@@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr frame,
 
   gdb_assert (type->code () == TYPE_CODE_FLT);
 
+  /* We have an IEEE 128-bit float need to change regnum mapping from
+     fpr to vsr.  */
+  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
+
   target_float_convert (from, type,
 			to, builtin_type (gdbarch)->builtin_double);
   put_frame_register (frame, regnum, to);
 }
 
+static struct value *
+rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type,
+			    int regnum, struct frame_id frame_id)
+{
+  int len = type->length ();
+  struct value *value = value::allocate (type);
+  frame_info_ptr frame;
+
+  /* We have an IEEE 128-bit float need to change regnum mapping from
+     fpr to vsr.  */
+  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
+
+  value->set_lval (lval_register);
+  frame = frame_find_by_id (frame_id);
+
+  if (frame == NULL)
+    frame_id = null_frame_id;
+  else
+    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+
+  VALUE_NEXT_FRAME_ID (value) = frame_id;
+  VALUE_REGNUM (value) = regnum;
+
+  /* Any structure stored in more than one register will always be
+     an integral number of registers.  Otherwise, you need to do
+     some fiddling with the last register copied here for little
+     endian machines.  */
+  if (type_byte_order (type) == BFD_ENDIAN_BIG
+      && len < register_size (gdbarch, regnum))
+    /* Big-endian, and we want less than full size.  */
+    value->set_offset (register_size (gdbarch, regnum) - len);
+  else
+    value->set_offset (0);
+
+  return value;
+}
+
  /* The type of a function that moves the value of REG between CACHE
     or BUF --- in either direction.  */
 typedef enum register_status (*move_ev_register_func) (struct regcache *,
@@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p);
   set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value);
   set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register);
+  set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register);
 
   set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum);
   set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum);
  

Keith:

Thanks for catching the typos.  I have fixed them locally for now as
noted below.

                      Carl
--------------

On Fri, 2023-10-13 at 13:35 -0700, Keith Seitz wrote:
> Carl Love wrote:
> 
> As in your previous patch, I only have some really trivial nits to
> point out.
> So treat similarly, and thank you for the patch.
> 
> > The test currently fails for IEEE 128-bit floating point
> > types.  PowerPC
> > supports the IBM double 128-bit floating point	format and IEEE
> > 128-bit
> 
> Looks like a tab got inserted above?

Yea, Git gui doesn't show tabs in the window.  Fixed
> 
> > format.  The IBM double 128-bit floating point format uses two 64-
> > bit
> > floating point registers to store the 128-bit value.  The IEEE 128-
> > bit
> > floating point format stores the value in a single 128-bit vector-
> > scalar
> > register (vsr).
> > The various floating point values, 32-bit float, 64-bit double, IBM
> > double
> > 128-bit float and IEEE 128-bit floating point numbers are all
> > mapped to the
> > DWARF fpr numbers.  The issue is the IEEE 128-bit floating point
> > values are
> > actually stored in a vsr not the fprs.  This patch changes the
> > register
> > mapping for the vsrs from the fpr to the vsr registers so the value
> > is
> > properly accessed by GDB.  The functions
> > rs6000_linux_register_to_value,
> > rs6000_linux_value_to_register, rs6000_linux_value_from_register
> > check if
> > the value is an IEEE 128-bit floating point value and adjust the
> > register
> > number as needed.  The test in function rs6000_convert_register_p
> > is fixed
> > to so it is only true for floating point values.
> 
> "fixed [to] so"
> 
Looks like the "to" didn't get cleanup in the last re-write.  Changed
"to so it" to "so it".


> > This patch fixes three regression tests in gdb.base/store.exp.
> > 
> > The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power
> > 10 LE
> > with no regressions.
> 
> Indeed!
> 
> Tested-by: Keith Seitz <keiths@redhat.com>
> 
> > Change to inline function
> 
> Stray line or was there something more to communicate?

Stray line, got missed in the last edit.  Removed.  Thanks.

> 
> Keith
> ---
>  gdb/ppc-linux-tdep.c |  4 +++
>  gdb/rs6000-tdep.c    | 66
> +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 7fb90799dff..ba646a7230f 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -63,6 +63,7 @@
>  #include <ctype.h>
>  #include "elf-bfd.h"
>  #include "producer.h"
> +#include "target-float.h"
>  
>  #include "features/rs6000/powerpc-32l.c"
>  #include "features/rs6000/powerpc-altivec32l.c"
> @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct
> gdbarch *gdbarch, int num)
>      /* FIXME: jimb/2004-05-05: What should we do when the debug info
>         specifies registers the architecture doesn't have?  Our
>         callers don't check the value we return.  */
> +    /* Map dwarf register numbers for floating point, double, IBM
> double and
> +       IEEE 128-bit floating point to the fpr range.  Will have to
> fix the
> +       mapping for the IEEE 128-bit register numbers later.  */
> 
> I suggest rewording this last incomplete sentence to something like:
> "The mapping
> for the IEEE 128-bit register numbers will have to be fixed later."
> 
> return tdep->ppc_fp0_regnum + (num - 32);
>    else if (77 <= num && num < 77 + 32)
>      return tdep->ppc_vr0_regnum + (num - 77);
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 23397d037ae..ada9cea3353 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch
> *gdbarch, int regnum,
>  	  && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs
>  	  && type->code () == TYPE_CODE_FLT
>  	  && (type->length ()
> -	      != builtin_type (gdbarch)->builtin_double->length ()));
> +	      == builtin_type (gdbarch)->builtin_float->length ()));
> +}
> +
> +static int
> +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type
> *type,
> +			      int regnum)
> +{
> +  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
> +
> +  /* If we have the an IEEE 128-bit floating point value, need to
> map the
> +   register number to the corresponding VSR.  */
> 
> "If we have the an" --> "If we have an". "need to" --> ""
> 
> +  if (tdep->ppc_vsr0_regnum != -1
> +      && regnum >= tdep->ppc_fp0_regnum
> +      && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs)
> +      && (gdbarch_long_double_format (gdbarch) ==
> floatformats_ieee_quad)
> +      && (type->length() == 16))
> +    regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum;
> +
> +  return regnum;
>  }
>  
>  static int
> @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr
> frame,
>    
>    gdb_assert (type->code () == TYPE_CODE_FLT);
>  
> +  /* We have an IEEE 128-bit float need to change regnum mapping
> from
> +     fpr to vsr.  */
> +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> +
> 
> Add "--" before "need". Similarly below in a few spots.

Fixed.

> 
> if (!get_frame_register_bytes (frame, regnum, 0,
>  				 gdb::make_array_view (from,
>  						       register_size
> (gdbarch,
> @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr
> frame,
>  
>    gdb_assert (type->code () == TYPE_CODE_FLT);
>  
> +  /* We have an IEEE 128-bit float need to change regnum mapping
> from

Added "--"  before "need".
> +     fpr to vsr.  */
> +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> +
>    target_float_convert (from, type,
>  			to, builtin_type (gdbarch)->builtin_double);
>    put_frame_register (frame, regnum, to);
>  }
>  
> +static struct value *
> +rs6000_value_from_register (struct gdbarch *gdbarch, struct type
> *type,
> +			    int regnum, struct frame_id frame_id)
> +{
> +  int len = type->length ();
> +  struct value *value = value::allocate (type);
> +  frame_info_ptr frame;
> +
> +  /* We have an IEEE 128-bit float need to
> change regnum mapping from

Added "--"  before "need".

> +     fpr to vsr.  */
> +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> +
> +  value->set_lval (lval_register);
> +  frame = frame_find_by_id (frame_id);
> +
> +  if (frame == NULL)
> +    frame_id = null_frame_id;
> +  else
> +    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
> +
> +  VALUE_NEXT_FRAME_ID (value) = frame_id;
> +  VALUE_REGNUM (value) = regnum;
> +
> +  /* Any structure stored in more than one register will always be
> +     an integral number of registers.  Otherwise, you need to do
> +     some fiddling with the last register copied here for little
> +     endian machines.  */
> +  if (type_byte_order (type) == BFD_ENDIAN_BIG
> +      && len < register_size (gdbarch, regnum))
> +    /* Big-endian, and we want less than full size.  */
> +    value->set_offset (register_size (gdbarch, regnum) - len);
> +  else
> +    value->set_offset (0);
> +
> +  return value;
> +}
> +
>   /* The type of a function that moves the value of REG between CACHE
>      or BUF --- in either direction.  */
>  typedef enum register_status (*move_ev_register_func) (struct
> regcache *,
> @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info,
> struct gdbarch_list *arches)
>    set_gdbarch_convert_register_p (gdbarch,
> rs6000_convert_register_p);
>    set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value);
>    set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register);
> +  set_gdbarch_value_from_register (gdbarch,
> rs6000_value_from_register);
>  
>    set_gdbarch_stab_reg_to_regnum (gdbarch,
> rs6000_stab_reg_to_regnum);
>    set_gdbarch_dwarf2_reg_to_regnum (gdbarch,
> rs6000_dwarf2_reg_to_regnum);
  

Carl Love <cel@us.ibm.com> wrote:

>Thanks for catching the typos.  I have fixed them locally for now as
>noted below.

This patch (including fixed for the issues pointed out by
Keith) is OK.

Thanks,
Ulrich
  

Carl Love <cel@us.ibm.com> writes:

> GDB maintainers:
>
> This is the second patch which fixes the 128-bit floating point
> register mappings.
>
>              Carl 
> --------------------------------------------------------
> Fix test gdb.base/store.exp
>
> The test currently fails for IEEE 128-bit floating point types.  PowerPC
> supports the IBM double 128-bit floating point	format and IEEE 128-bit
> format.  The IBM double 128-bit floating point format uses two 64-bit
> floating point registers to store the 128-bit value.  The IEEE 128-bit
> floating point format stores the value in a single 128-bit vector-scalar
> register (vsr).
>
> The various floating point values, 32-bit float, 64-bit double, IBM double
> 128-bit float and IEEE 128-bit floating point numbers are all mapped to the
> DWARF fpr numbers.  The issue is the IEEE 128-bit floating point values are
> actually stored in a vsr not the fprs.  This patch changes the register
> mapping for the vsrs from the fpr to the vsr registers so the value is
> properly accessed by GDB.  The functions rs6000_linux_register_to_value,
> rs6000_linux_value_to_register, rs6000_linux_value_from_register check if
> the value is an IEEE 128-bit floating point value and adjust the register
> number as needed.  The test in function rs6000_convert_register_p is fixed
> to so it is only true for floating point values.
>
> This patch fixes three regression tests in gdb.base/store.exp.
>
> The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE
> with no regressions.
>
> Change to inline function
> ---
>  gdb/ppc-linux-tdep.c |  4 +++
>  gdb/rs6000-tdep.c    | 66 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 7fb90799dff..ba646a7230f 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -63,6 +63,7 @@
>  #include <ctype.h>
>  #include "elf-bfd.h"
>  #include "producer.h"
> +#include "target-float.h"
>  
>  #include "features/rs6000/powerpc-32l.c"
>  #include "features/rs6000/powerpc-altivec32l.c"
> @@ -2101,6 +2102,9 @@ rs6000_linux_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int num)
>      /* FIXME: jimb/2004-05-05: What should we do when the debug info
>         specifies registers the architecture doesn't have?  Our
>         callers don't check the value we return.  */
> +    /* Map dwarf register numbers for floating point, double, IBM double and
> +       IEEE 128-bit floating point to the fpr range.  Will have to fix the
> +       mapping for the IEEE 128-bit register numbers later.  */
>      return tdep->ppc_fp0_regnum + (num - 32);
>    else if (77 <= num && num < 77 + 32)
>      return tdep->ppc_vr0_regnum + (num - 77);
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 23397d037ae..ada9cea3353 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -2676,7 +2676,25 @@ rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum,
>  	  && regnum < tdep->ppc_fp0_regnum + ppc_num_fprs
>  	  && type->code () == TYPE_CODE_FLT
>  	  && (type->length ()
> -	      != builtin_type (gdbarch)->builtin_double->length ()));
> +	      == builtin_type (gdbarch)->builtin_float->length ()));
> +}
> +
> +static int
> +ieee_128_float_regnum_adjust (struct gdbarch *gdbarch, struct type *type,
> +			      int regnum)
> +{
> +  ppc_gdbarch_tdep *tdep = gdbarch_tdep<ppc_gdbarch_tdep> (gdbarch);
> +
> +  /* If we have the an IEEE 128-bit floating point value, need to map the
> +   register number to the corresponding VSR.  */
> +  if (tdep->ppc_vsr0_regnum != -1
> +      && regnum >= tdep->ppc_fp0_regnum
> +      && regnum < (tdep->ppc_fp0_regnum + ppc_num_fprs)
> +      && (gdbarch_long_double_format (gdbarch) == floatformats_ieee_quad)
> +      && (type->length() == 16))
> +    regnum = regnum - tdep->ppc_fp0_regnum + tdep->ppc_vsr0_regnum;
> +
> +  return regnum;
>  }
>  
>  static int
> @@ -2691,6 +2709,10 @@ rs6000_register_to_value (frame_info_ptr frame,
>    
>    gdb_assert (type->code () == TYPE_CODE_FLT);
>  
> +  /* We have an IEEE 128-bit float need to change regnum mapping from
> +     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,
> @@ -2715,11 +2737,52 @@ rs6000_value_to_register (frame_info_ptr frame,
>  
>    gdb_assert (type->code () == TYPE_CODE_FLT);
>  
> +  /* We have an IEEE 128-bit float need to change regnum mapping from
> +     fpr to vsr.  */
> +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> +
>    target_float_convert (from, type,
>  			to, builtin_type (gdbarch)->builtin_double);
>    put_frame_register (frame, regnum, to);
>  }
>  
> +static struct value *
> +rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type,
> +			    int regnum, struct frame_id frame_id)
> +{
> +  int len = type->length ();
> +  struct value *value = value::allocate (type);
> +  frame_info_ptr frame;
> +
> +  /* We have an IEEE 128-bit float need to change regnum mapping from
> +     fpr to vsr.  */
> +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> +
> +  value->set_lval (lval_register);
> +  frame = frame_find_by_id (frame_id);

You can move the declaration of frame to here:

  frame_info_ptr frame = frame_find_by_id (frame_id);

which is the preferred GDB style these days.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +
> +  if (frame == NULL)
> +    frame_id = null_frame_id;
> +  else
> +    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
> +
> +  VALUE_NEXT_FRAME_ID (value) = frame_id;
> +  VALUE_REGNUM (value) = regnum;
> +
> +  /* Any structure stored in more than one register will always be
> +     an integral number of registers.  Otherwise, you need to do
> +     some fiddling with the last register copied here for little
> +     endian machines.  */
> +  if (type_byte_order (type) == BFD_ENDIAN_BIG
> +      && len < register_size (gdbarch, regnum))
> +    /* Big-endian, and we want less than full size.  */
> +    value->set_offset (register_size (gdbarch, regnum) - len);
> +  else
> +    value->set_offset (0);
> +
> +  return value;
> +}
> +
>   /* The type of a function that moves the value of REG between CACHE
>      or BUF --- in either direction.  */
>  typedef enum register_status (*move_ev_register_func) (struct regcache *,
> @@ -8337,6 +8400,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    set_gdbarch_convert_register_p (gdbarch, rs6000_convert_register_p);
>    set_gdbarch_register_to_value (gdbarch, rs6000_register_to_value);
>    set_gdbarch_value_to_register (gdbarch, rs6000_value_to_register);
> +  set_gdbarch_value_from_register (gdbarch, rs6000_value_from_register);
>  
>    set_gdbarch_stab_reg_to_regnum (gdbarch, rs6000_stab_reg_to_regnum);
>    set_gdbarch_dwarf2_reg_to_regnum (gdbarch, rs6000_dwarf2_reg_to_regnum);
> -- 
> 2.37.2
  

Andrew:

On Mon, 2023-10-16 at 15:36 +0100, Andrew Burgess wrote:
> Carl Love <cel@us.ibm.com> writes:
> 
> > GDB maintainers:
> > 
> > This is the second patch which fixes the 128-bit floating point
> > register mappings.
> >  

<snip>
 
> > +static struct value *
> > +rs6000_value_from_register (struct gdbarch *gdbarch, struct type
> > *type,
> > +			    int regnum, struct frame_id frame_id)
> > +{
> > +  int len = type->length ();
> > +  struct value *value = value::allocate (type);
> > +  frame_info_ptr frame;
> > +
> > +  /* We have an IEEE 128-bit float need to change regnum mapping
> > from
> > +     fpr to vsr.  */
> > +  regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
> > +
> > +  value->set_lval (lval_register);
> > +  frame = frame_find_by_id (frame_id);
> 
> You can move the declaration of frame to here:
> 
>   frame_info_ptr frame = frame_find_by_id (frame_id);

OK, fixed.


> 
> which is the preferred GDB style these days.
> 
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew
>