public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
@ 2014-04-14 17:00 Ulrich Weigand
  2014-04-16 15:29 ` Joel Brobecker
  2014-08-27  4:01 ` Andrew Pinski
  0 siblings, 2 replies; 12+ messages in thread
From: Ulrich Weigand @ 2014-04-14 17:00 UTC (permalink / raw)
  To: gdb-patches

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.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-04-14 17:00 [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg Ulrich Weigand
@ 2014-04-16 15:29 ` Joel Brobecker
  2014-04-17 12:18   ` Ulrich Weigand
  2014-08-27  4:01 ` Andrew Pinski
  1 sibling, 1 reply; 12+ messages in thread
From: Joel Brobecker @ 2014-04-16 15:29 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: gdb-patches

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

FWIW, I am not a specialist here, but it seemed reasonable to me.

It took me a minute or two to figure out that you needed to make
value_from_register a class "m" because you don't have access to
the gdbarch via the frame anymore, so you have to have it passed
directly. Perhaps it's worth a sentence in the revision log when
you commit the patch?

Thanks for the detailed comment explaining why we use a null_frame_id
in address_from_register, and why it's OK to do so. Very helpful!

-- 
Joel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-04-16 15:29 ` Joel Brobecker
@ 2014-04-17 12:18   ` Ulrich Weigand
  0 siblings, 0 replies; 12+ messages in thread
From: Ulrich Weigand @ 2014-04-17 12:18 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

Joel Brobecker wrote:
> > 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.
> 
> FWIW, I am not a specialist here, but it seemed reasonable to me.
> 
> It took me a minute or two to figure out that you needed to make
> value_from_register a class "m" because you don't have access to
> the gdbarch via the frame anymore, so you have to have it passed
> directly. Perhaps it's worth a sentence in the revision log when
> you commit the patch?
> 
> Thanks for the detailed comment explaining why we use a null_frame_id
> in address_from_register, and why it's OK to do so. Very helpful!

Thanks for the review!   I've checked the patch in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-04-14 17:00 [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg Ulrich Weigand
  2014-04-16 15:29 ` Joel Brobecker
@ 2014-08-27  4:01 ` Andrew Pinski
  2014-08-27 12:21   ` Ulrich Weigand
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Pinski @ 2014-08-27  4:01 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: gdb-patches

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-27  4:01 ` Andrew Pinski
@ 2014-08-27 12:21   ` Ulrich Weigand
  2014-08-27 18:15     ` Maciej W. Rozycki
  2014-09-06 22:34     ` Andrew Pinski
  0 siblings, 2 replies; 12+ messages in thread
From: Ulrich Weigand @ 2014-08-27 12:21 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gdb-patches

Andrew Pinski wrote:

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

Huh, I wasn't aware of that conversion.  Note that for the register_to_value
case, I don't actually see any difference to the default behavior; it's the
value_to_register routine that's really special (because of the sign-extension
in performs).

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

I guess it isn't too hard to support gdbarch_convert_register_p in that
routine as well; I just didn't have any target to test on.

Can you try whether something along the following lines works for you?

Bye,
Ulrich

ChangeLog:

	* findvar.c (address_from_register): Handle targets requiring
	a special conversion routine even for plain pointer types.

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 41887de..ba3dd4d 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -764,11 +764,28 @@ address_from_register (int regnum, struct frame_info *frame)
      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.
+     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));
+  /* Some targets require a special conversion routine even for plain
+     pointer types.  Avoid constructing a value object in those cases.  */
+  if (gdbarch_convert_register_p (gdbarch, regnum, type))
+    {
+      gdb_byte *buf = alloca (TYPE_LENGTH (type));
+      int optim, unavail, ok;
+
+      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
+				      buf, &optim, &unavail);
+      if (!ok)
+	{
+	  /* This function is used while computing a location expression.
+	     Complain about the value being optimized out, rather than
+	     letting value_as_address complain about some random register
+	     the expression depends on not being saved.  */
+	  error_value_optimized_out ();
+	}
+
+      return unpack_long (type, buf);
+    }
 
   value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id);
   read_frame_register_value (value, frame);

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-27 12:21   ` Ulrich Weigand
@ 2014-08-27 18:15     ` Maciej W. Rozycki
  2014-08-28 11:51       ` Ulrich Weigand
  2014-09-14 14:35       ` Maciej W. Rozycki
  2014-09-06 22:34     ` Andrew Pinski
  1 sibling, 2 replies; 12+ messages in thread
From: Maciej W. Rozycki @ 2014-08-27 18:15 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: Andrew Pinski, gdb-patches

On Wed, 27 Aug 2014, Ulrich Weigand wrote:

> Andrew Pinski wrote:
> 
> > 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;
> 
> Huh, I wasn't aware of that conversion.  Note that for the register_to_value
> case, I don't actually see any difference to the default behavior; it's the
> value_to_register routine that's really special (because of the sign-extension
> in performs).
> 
> > Is there a way to fix this in a target neutral way?  (I might need a
> > way like this for AARCH64 ILP32 also).
> 
> I guess it isn't too hard to support gdbarch_convert_register_p in that
> routine as well; I just didn't have any target to test on.
> 
> Can you try whether something along the following lines works for you?

 I'll see if I can push it through testing, though it may take a few days 
as some of MIPS hardware I use (and especially 64-bit one) is slooow (and 
I already have a test run under way).

 I'd expect the issue to be consistent across all ILP32 64-bit ABIs BTW, 
that is e.g. x32 x86-64 as well.

  Maciej

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-27 18:15     ` Maciej W. Rozycki
@ 2014-08-28 11:51       ` Ulrich Weigand
  2014-08-28 12:37         ` pinskia
  2014-09-14 14:35       ` Maciej W. Rozycki
  1 sibling, 1 reply; 12+ messages in thread
From: Ulrich Weigand @ 2014-08-28 11:51 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Andrew Pinski, gdb-patches

Maciej W. Rozycki wrote:
> On Wed, 27 Aug 2014, Ulrich Weigand wrote:
> > I guess it isn't too hard to support gdbarch_convert_register_p in that
> > routine as well; I just didn't have any target to test on.
> > 
> > Can you try whether something along the following lines works for you?
> 
>  I'll see if I can push it through testing, though it may take a few days 
> as some of MIPS hardware I use (and especially 64-bit one) is slooow (and 
> I already have a test run under way).

Thanks!
 
>  I'd expect the issue to be consistent across all ILP32 64-bit ABIs BTW, 
> that is e.g. x32 x86-64 as well.

Well, I guess that depends on whether the hardware expects 32-bit pointer
values to be sign-extended when loaded in 64-bit registers or not.  I don't
think x32 x86-64 requires this sign-extension (but I'm certainly not an
expert here) ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-28 11:51       ` Ulrich Weigand
@ 2014-08-28 12:37         ` pinskia
  0 siblings, 0 replies; 12+ messages in thread
From: pinskia @ 2014-08-28 12:37 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: Maciej W. Rozycki, gdb-patches



> On Aug 28, 2014, at 4:51 AM, "Ulrich Weigand" <uweigand@de.ibm.com> wrote:
> 
> Maciej W. Rozycki wrote:
>>> On Wed, 27 Aug 2014, Ulrich Weigand wrote:
>>> I guess it isn't too hard to support gdbarch_convert_register_p in that
>>> routine as well; I just didn't have any target to test on.
>>> 
>>> Can you try whether something along the following lines works for you?
>> 
>> I'll see if I can push it through testing, though it may take a few days 
>> as some of MIPS hardware I use (and especially 64-bit one) is slooow (and 
>> I already have a test run under way).
> 
> Thanks!
> 
>> I'd expect the issue to be consistent across all ILP32 64-bit ABIs BTW, 
>> that is e.g. x32 x86-64 as well.
> 
> Well, I guess that depends on whether the hardware expects 32-bit pointer
> values to be sign-extended when loaded in 64-bit registers or not.  I don't
> think x32 x86-64 requires this sign-extension (but I'm certainly not an
> expert here) ...

Both x32 and ilpl32 on arrch64 have zero-extended pointers so it should not be an issue with those two abis, only mips64's n32. 

Thanks,
Andrew

> 
> Bye,
> Ulrich
> 
> -- 
>  Dr. Ulrich Weigand
>  GNU/Linux compilers and toolchain
>  Ulrich.Weigand@de.ibm.com
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-27 12:21   ` Ulrich Weigand
  2014-08-27 18:15     ` Maciej W. Rozycki
@ 2014-09-06 22:34     ` Andrew Pinski
  2014-09-10 17:06       ` [COMMITTED] " Ulrich Weigand
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Pinski @ 2014-09-06 22:34 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: gdb-patches

On Wed, Aug 27, 2014 at 5:21 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Andrew Pinski wrote:
>
>> 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;
>
> Huh, I wasn't aware of that conversion.  Note that for the register_to_value
> case, I don't actually see any difference to the default behavior; it's the
> value_to_register routine that's really special (because of the sign-extension
> in performs).
>
>> Is there a way to fix this in a target neutral way?  (I might need a
>> way like this for AARCH64 ILP32 also).
>
> I guess it isn't too hard to support gdbarch_convert_register_p in that
> routine as well; I just didn't have any target to test on.
>
> Can you try whether something along the following lines works for you?

Yes this works for me.

Thanks,
Andrew Pinski


>
> Bye,
> Ulrich
>
> ChangeLog:
>
>         * findvar.c (address_from_register): Handle targets requiring
>         a special conversion routine even for plain pointer types.
>
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index 41887de..ba3dd4d 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -764,11 +764,28 @@ address_from_register (int regnum, struct frame_info *frame)
>       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.
> +     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));
> +  /* Some targets require a special conversion routine even for plain
> +     pointer types.  Avoid constructing a value object in those cases.  */
> +  if (gdbarch_convert_register_p (gdbarch, regnum, type))
> +    {
> +      gdb_byte *buf = alloca (TYPE_LENGTH (type));
> +      int optim, unavail, ok;
> +
> +      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
> +                                     buf, &optim, &unavail);
> +      if (!ok)
> +       {
> +         /* This function is used while computing a location expression.
> +            Complain about the value being optimized out, rather than
> +            letting value_as_address complain about some random register
> +            the expression depends on not being saved.  */
> +         error_value_optimized_out ();
> +       }
> +
> +      return unpack_long (type, buf);
> +    }
>
>    value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id);
>    read_frame_register_value (value, frame);
>
> --
>   Dr. Ulrich Weigand
>   GNU/Linux compilers and toolchain
>   Ulrich.Weigand@de.ibm.com
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [COMMITTED] Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-09-06 22:34     ` Andrew Pinski
@ 2014-09-10 17:06       ` Ulrich Weigand
  0 siblings, 0 replies; 12+ messages in thread
From: Ulrich Weigand @ 2014-09-10 17:06 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gdb-patches

Andrew Pinski wrote:
> On Wed, Aug 27, 2014 at 5:21 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > I guess it isn't too hard to support gdbarch_convert_register_p in that
> > routine as well; I just didn't have any target to test on.
> >
> > Can you try whether something along the following lines works for you?
> 
> Yes this works for me.

OK, I've checked this in now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-08-27 18:15     ` Maciej W. Rozycki
  2014-08-28 11:51       ` Ulrich Weigand
@ 2014-09-14 14:35       ` Maciej W. Rozycki
  2014-09-15 11:40         ` Ulrich Weigand
  1 sibling, 1 reply; 12+ messages in thread
From: Maciej W. Rozycki @ 2014-09-14 14:35 UTC (permalink / raw)
  To: Ulrich Weigand; +Cc: Andrew Pinski, gdb-patches

On Wed, 27 Aug 2014, Maciej W. Rozycki wrote:

> > I guess it isn't too hard to support gdbarch_convert_register_p in that
> > routine as well; I just didn't have any target to test on.
> > 
> > Can you try whether something along the following lines works for you?
> 
>  I'll see if I can push it through testing, though it may take a few days 
> as some of MIPS hardware I use (and especially 64-bit one) is slooow (and 
> I already have a test run under way).

 I had results a few days ago already, but I had to find some time to 
filter out intermittent failures.  I have done it now and the results look 
good.  For the record, I tested the mips-linux-gnu target and the 
following multilibs:

-EB
-EB -msoft-float
-EB -mips16
-EB -mips16 -msoft-float
-EB -mmicromips
-EB -mmicromips -msoft-float
-EB -mabi=n32
-EB -mabi=n32 -msoft-float
-EB -mabi=64
-EB -mabi=64 -msoft-float

and the -EL variants of same.  My toolchain defaults to and test hardware 
used implement the MIPS32r2 or MIPS64r2 ISA as applicable.

  Maciej

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg
  2014-09-14 14:35       ` Maciej W. Rozycki
@ 2014-09-15 11:40         ` Ulrich Weigand
  0 siblings, 0 replies; 12+ messages in thread
From: Ulrich Weigand @ 2014-09-15 11:40 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Andrew Pinski, gdb-patches

Maciej W. Rozycki wrote:
> On Wed, 27 Aug 2014, Maciej W. Rozycki wrote:
> 
> > > I guess it isn't too hard to support gdbarch_convert_register_p in that
> > > routine as well; I just didn't have any target to test on.
> > > 
> > > Can you try whether something along the following lines works for you?
> > 
> >  I'll see if I can push it through testing, though it may take a few days 
> > as some of MIPS hardware I use (and especially 64-bit one) is slooow (and 
> > I already have a test run under way).
> 
>  I had results a few days ago already, but I had to find some time to 
> filter out intermittent failures.  I have done it now and the results look 
> good.

Excellent, thanks again for verifying!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-09-15 11:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-14 17:00 [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg Ulrich Weigand
2014-04-16 15:29 ` Joel Brobecker
2014-04-17 12:18   ` Ulrich Weigand
2014-08-27  4:01 ` Andrew Pinski
2014-08-27 12:21   ` Ulrich Weigand
2014-08-27 18:15     ` Maciej W. Rozycki
2014-08-28 11:51       ` Ulrich Weigand
2014-08-28 12:37         ` pinskia
2014-09-14 14:35       ` Maciej W. Rozycki
2014-09-15 11:40         ` Ulrich Weigand
2014-09-06 22:34     ` Andrew Pinski
2014-09-10 17:06       ` [COMMITTED] " Ulrich Weigand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).