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

[Andrew Burgess <aburgess@redhat.com>]

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