Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg

[Andrew Pinski <pinskia at gmail dot com>]

On Mon, Apr 14, 2014 at 10:00 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Hello,
>
> with recent compilers I'm seeing lots of failures in the GDB testsuite on SPU,
> because debug data now tends to use features more frequently that rely on the
> debugger using DWARF frame unwinding.  I had never enabled that on spu, and
> just used prologue parsing instead, which shows up as a problem now ...
>
> However, I cannot simply enable DWARF unwinding, since dwarf2-frame.c common
> code does not handle the situation well (that we have on SPU) where the stack
> and/or frame pointer are maintained in *vector* registers.  This is because
> read_addr_from_reg is hard-coded to assume that such pointers can be read
> from registers via a simple get_frame_register / unpack_pointer operation.
>
> Now, there *is* a routine address_from_register that calls into the
> appropriate tdep routines to handle pointer values in "weird" registers
> like on SPU, but it turns out I cannot simply change dwarf2-frame.c to
> use address_from_register.  This is because address_from_register uses
> value_from_register to create a (temporary) value, and that routine
> at some point calls get_frame_id in order to set up that value's
> VALUE_FRAME_ID entry.
>
> However, the dwarf2-frame.c read_addr_from_reg routine will be called
> during early unwinding (to unwind the frame's CFA), at which point the
> frame's ID is not actually known yet!  This would cause an assert.
>
> I not completely sure what the best fix is.
>
> One way might be to recognize that VALUE_FRAME_ID is only needed in the
> value returned by value_from_register if that value is later used as an
> lvalue.  But this is obviously never done to the temporary value used in
> address_from_register.  So, if we could change address_from_register to
> not call value_from_register but instead accept constructing a value
> that doesn't have VALUE_FRAME_ID set, things should be fine.
>
> To do that, we can change the value_from_register callback to accept
> a FRAME_ID instead of a FRAME; the only existing uses of the FRAME
> argument were either to extract its frame ID, or its gdbarch.  (To
> keep a way of getting at the latter, we also change the callback's
> type from "f" to "m".)  Together with the required follow-on changes
> in the existing value_from_register implementations (including the
> default one), this seems to fix the problem.
>
> As another minor interface cleanup, I've removed the explicit TYPE
> argument from address_from_register.  This routine really always
> uses a default pointer type, and in the new implementation it -to
> some extent- relies on that fact, in that it will now no longer
> handle types that require gdbarch_convert_register_p handling.
>
> Does this sound reasonable?  Comments or alternative suggestions
> would be appreciated!
>
> Tested on powerpc64-linux and spu-elf with no regressions.
>
> If there are no objections, I'm planning on committing this
> later this week.


I think this patch broke MIPS64 n32 big-endian support.  We assert here:
gdb_assert (!gdbarch_convert_register_p (gdbarch, regnum, type));

The convert_register_p code for MIPS does:
  return (register_size (gdbarch, regnum) == 8
          && regnum % num_regs > 0 && regnum % num_regs < 32
          && TYPE_LENGTH (type) < 8);


Since the register size is 8 byte wide (MIPS64) and the type length is
4 (pointer), we return true.  In MIPS64, the registers are stored
64bits but pointers are 32bits.

Here is the code that is used by mips_register_to_value:
      int len = TYPE_LENGTH (type);
      CORE_ADDR offset;

      offset = gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG ? 8 - len : 0;
      if (!get_frame_register_bytes (frame, regnum, offset, len, to,
    optimizedp, unavailablep))
return 0;

      *optimizedp = *unavailablep = 0;
      return 1;

Is there a way to fix this in a target neutral way?  (I might need a
way like this for AARCH64 ILP32 also).

Thanks,
Andrew Pinski



>
> Bye,
> Ulrich
>
>
> ChangeLog:
>
>         * gdbarch.sh (value_from_register): Make class "m" instead of "f".
>         Replace FRAME argument with FRAME_ID.
>         * gdbarch.c, gdbarch.h: Regenerate.
>         * findvar.c (default_value_from_register): Add GDBARCH argument;
>         replace FRAME by FRAME_ID.  No longer call get_frame_id.
>         (value_from_register): Update call to gdbarch_value_from_register.
>         * value.h (default_value_from_register): Update prototype.
>         * s390-linux-tdep.c (s390_value_from_register): Update interface
>         and call to default_value_from_register.
>         * spu-tdep.c (spu_value_from_register): Likewise.
>
>         * findvar.c (address_from_register): Remove TYPE argument.
>         Do not call value_from_register; use gdbarch_value_from_register
>         with null_frame_id instead.
>         * value.h (address_from_register): Update prototype.
>         * dwarf2-frame.c (read_addr_from_reg): Use address_from_register.
>         * dwarf2loc.c (dwarf_expr_read_addr_from_reg): Update for
>         address_from_register interface change.
>
>
> Index: binutils-gdb/gdb/dwarf2-frame.c
> ===================================================================
> --- binutils-gdb.orig/gdb/dwarf2-frame.c
> +++ binutils-gdb/gdb/dwarf2-frame.c
> @@ -291,15 +291,9 @@ read_addr_from_reg (void *baton, int reg
>  {
>    struct frame_info *this_frame = (struct frame_info *) baton;
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> -  int regnum;
> -  gdb_byte *buf;
> -
> -  regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg);
> -
> -  buf = alloca (register_size (gdbarch, regnum));
> -  get_frame_register (this_frame, regnum, buf);
> +  int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg);
>
> -  return unpack_pointer (register_type (gdbarch, regnum), buf);
> +  return address_from_register (regnum, this_frame);
>  }
>
>  /* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback.  */
> Index: binutils-gdb/gdb/dwarf2loc.c
> ===================================================================
> --- binutils-gdb.orig/gdb/dwarf2loc.c
> +++ binutils-gdb/gdb/dwarf2loc.c
> @@ -317,13 +317,9 @@ dwarf_expr_read_addr_from_reg (void *bat
>  {
>    struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
>    struct gdbarch *gdbarch = get_frame_arch (debaton->frame);
> -  CORE_ADDR result;
> -  int regnum;
> +  int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, dwarf_regnum);
>
> -  regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, dwarf_regnum);
> -  result = address_from_register (builtin_type (gdbarch)->builtin_data_ptr,
> -                                 regnum, debaton->frame);
> -  return result;
> +  return address_from_register (regnum, debaton->frame);
>  }
>
>  /* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback.  */
> Index: binutils-gdb/gdb/findvar.c
> ===================================================================
> --- binutils-gdb.orig/gdb/findvar.c
> +++ binutils-gdb/gdb/findvar.c
> @@ -625,15 +625,14 @@ read_var_value (struct symbol *var, stru
>  /* Install default attributes for register values.  */
>
>  struct value *
> -default_value_from_register (struct type *type, int regnum,
> -                            struct frame_info *frame)
> +default_value_from_register (struct gdbarch *gdbarch, struct type *type,
> +                             int regnum, struct frame_id frame_id)
>  {
> -  struct gdbarch *gdbarch = get_frame_arch (frame);
>    int len = TYPE_LENGTH (type);
>    struct value *value = allocate_value (type);
>
>    VALUE_LVAL (value) = lval_register;
> -  VALUE_FRAME_ID (value) = get_frame_id (frame);
> +  VALUE_FRAME_ID (value) = frame_id;
>    VALUE_REGNUM (value) = regnum;
>
>    /* Any structure stored in more than one register will always be
> @@ -741,7 +740,8 @@ value_from_register (struct type *type,
>    else
>      {
>        /* Construct the value.  */
> -      v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
> +      v = gdbarch_value_from_register (gdbarch, type,
> +                                      regnum, get_frame_id (frame));
>
>        /* Get the data.  */
>        read_frame_register_value (v, frame);
> @@ -750,18 +750,30 @@ value_from_register (struct type *type,
>    return v;
>  }
>
> -/* Return contents of register REGNUM in frame FRAME as address,
> -   interpreted as value of type TYPE.   Will abort if register
> -   value is not available.  */
> +/* Return contents of register REGNUM in frame FRAME as address.
> +   Will abort if register value is not available.  */
>
>  CORE_ADDR
> -address_from_register (struct type *type, int regnum, struct frame_info *frame)
> +address_from_register (int regnum, struct frame_info *frame)
>  {
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  struct type *type = builtin_type (gdbarch)->builtin_data_ptr;
>    struct value *value;
>    CORE_ADDR result;
>
> -  value = value_from_register (type, regnum, frame);
> -  gdb_assert (value);
> +  /* This routine may be called during early unwinding, at a time
> +     where the ID of FRAME is not yet known.  Calling value_from_register
> +     would therefore abort in get_frame_id.  However, since we only need
> +     a temporary value that is never used as lvalue, we actually do not
> +     really need to set its VALUE_FRAME_ID.  Therefore, we re-implement
> +     the core of value_from_register, but use the null_frame_id.
> +
> +     This works only if we do not require a special conversion routine,
> +     which is true for plain pointer types for all current targets.  */
> +  gdb_assert (!gdbarch_convert_register_p (gdbarch, regnum, type));
> +
> +  value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id);
> +  read_frame_register_value (value, frame);
>
>    if (value_optimized_out (value))
>      {
> Index: binutils-gdb/gdb/gdbarch.c
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbarch.c
> +++ binutils-gdb/gdb/gdbarch.c
> @@ -2381,13 +2381,13 @@ set_gdbarch_value_to_register (struct gd
>  }
>
>  struct value *
> -gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_info *frame)
> +gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->value_from_register != NULL);
>    if (gdbarch_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "gdbarch_value_from_register called\n");
> -  return gdbarch->value_from_register (type, regnum, frame);
> +  return gdbarch->value_from_register (gdbarch, type, regnum, frame_id);
>  }
>
>  void
> Index: binutils-gdb/gdb/gdbarch.h
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbarch.h
> +++ binutils-gdb/gdb/gdbarch.h
> @@ -422,12 +422,12 @@ extern void gdbarch_value_to_register (s
>  extern void set_gdbarch_value_to_register (struct gdbarch *gdbarch, gdbarch_value_to_register_ftype *value_to_register);
>
>  /* Construct a value representing the contents of register REGNUM in
> -   frame FRAME, interpreted as type TYPE.  The routine needs to
> +   frame FRAME_ID, interpreted as type TYPE.  The routine needs to
>     allocate and return a struct value with all value attributes
>     (but not the value contents) filled in. */
>
> -typedef struct value * (gdbarch_value_from_register_ftype) (struct type *type, int regnum, struct frame_info *frame);
> -extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_info *frame);
> +typedef struct value * (gdbarch_value_from_register_ftype) (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
> +extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
>  extern void set_gdbarch_value_from_register (struct gdbarch *gdbarch, gdbarch_value_from_register_ftype *value_from_register);
>
>  typedef CORE_ADDR (gdbarch_pointer_to_address_ftype) (struct gdbarch *gdbarch, struct type *type, const gdb_byte *buf);
> Index: binutils-gdb/gdb/gdbarch.sh
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbarch.sh
> +++ binutils-gdb/gdb/gdbarch.sh
> @@ -500,10 +500,10 @@ m:int:convert_register_p:int regnum, str
>  f:int:register_to_value:struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep:frame, regnum, type, buf, optimizedp, unavailablep:0
>  f:void:value_to_register:struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf:frame, regnum, type, buf:0
>  # Construct a value representing the contents of register REGNUM in
> -# frame FRAME, interpreted as type TYPE.  The routine needs to
> +# frame FRAME_ID, interpreted as type TYPE.  The routine needs to
>  # allocate and return a struct value with all value attributes
>  # (but not the value contents) filled in.
> -f:struct value *:value_from_register:struct type *type, int regnum, struct frame_info *frame:type, regnum, frame::default_value_from_register::0
> +m:struct value *:value_from_register:struct type *type, int regnum, struct frame_id frame_id:type, regnum, frame_id::default_value_from_register::0
>  #
>  m:CORE_ADDR:pointer_to_address:struct type *type, const gdb_byte *buf:type, buf::unsigned_pointer_to_address::0
>  m:void:address_to_pointer:struct type *type, gdb_byte *buf, CORE_ADDR addr:type, buf, addr::unsigned_address_to_pointer::0
> Index: binutils-gdb/gdb/s390-linux-tdep.c
> ===================================================================
> --- binutils-gdb.orig/gdb/s390-linux-tdep.c
> +++ binutils-gdb/gdb/s390-linux-tdep.c
> @@ -380,11 +380,11 @@ s390_pseudo_register_write (struct gdbar
>     registers, even though we are otherwise a big-endian platform.  */
>
>  static struct value *
> -s390_value_from_register (struct type *type, int regnum,
> -                         struct frame_info *frame)
> +s390_value_from_register (struct gdbarch *gdbarch, struct type *type,
> +                         int regnum, struct frame_id frame_id)
>  {
> -  struct value *value = default_value_from_register (type, regnum, frame);
> -
> +  struct value *value = default_value_from_register (gdbarch, type,
> +                                                    regnum, frame_id);
>    check_typedef (type);
>
>    if (regnum >= S390_F0_REGNUM && regnum <= S390_F15_REGNUM
> Index: binutils-gdb/gdb/spu-tdep.c
> ===================================================================
> --- binutils-gdb.orig/gdb/spu-tdep.c
> +++ binutils-gdb/gdb/spu-tdep.c
> @@ -314,10 +314,11 @@ spu_pseudo_register_write (struct gdbarc
>  /* Value conversion -- access scalar values at the preferred slot.  */
>
>  static struct value *
> -spu_value_from_register (struct type *type, int regnum,
> -                        struct frame_info *frame)
> +spu_value_from_register (struct gdbarch *gdbarch, struct type *type,
> +                        int regnum, struct frame_id frame_id)
>  {
> -  struct value *value = default_value_from_register (type, regnum, frame);
> +  struct value *value = default_value_from_register (gdbarch, type,
> +                                                    regnum, frame_id);
>    int len = TYPE_LENGTH (type);
>
>    if (regnum < SPU_NUM_GPRS && len < 16)
> Index: binutils-gdb/gdb/value.h
> ===================================================================
> --- binutils-gdb.orig/gdb/value.h
> +++ binutils-gdb/gdb/value.h
> @@ -579,9 +579,10 @@ extern struct value *value_from_contents
>                                                       CORE_ADDR);
>  extern struct value *value_from_contents (struct type *, const gdb_byte *);
>
> -extern struct value *default_value_from_register (struct type *type,
> +extern struct value *default_value_from_register (struct gdbarch *gdbarch,
> +                                                 struct type *type,
>                                                   int regnum,
> -                                                 struct frame_info *frame);
> +                                                 struct frame_id frame_id);
>
>  extern void read_frame_register_value (struct value *value,
>                                        struct frame_info *frame);
> @@ -589,7 +590,7 @@ extern void read_frame_register_value (s
>  extern struct value *value_from_register (struct type *type, int regnum,
>                                           struct frame_info *frame);
>
> -extern CORE_ADDR address_from_register (struct type *type, int regnum,
> +extern CORE_ADDR address_from_register (int regnum,
>                                         struct frame_info *frame);
>
>  extern struct value *value_of_variable (struct symbol *var,
> --
>   Dr. Ulrich Weigand
>   GNU/Linux compilers and toolchain
>   Ulrich.Weigand@de.ibm.com
>