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

[Keith Seitz <keiths@redhat.com>]

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);
-- 
2.37.2