[RFA/RFC] Restore old handling of multi-register variables

[RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

I came across a situation on AVR where pretty much none of the
local variable could be printed. Consider for instance the following
code:

    procedure Foo is
       My_Variable : Integer := 16#12cd#;
    begin
       Do_Nothing (My_Variable'Address);
    end Foo;

After having run the program up until the call to `Do_Nothing',
the debugger is unable to print the value of the variable:

    (gdb) p my_variable
    $1 = 0

The problem is that the address of my_variable is improperly computed.
The debug info says:

        .uleb128 0x4     ;  (DIE (0x48) DW_TAG_variable)
        .long   .LASF5   ;  DW_AT_name: "my_variable"
        .long   0x57     ;  DW_AT_type
        .byte   0x2      ;  DW_AT_location
        .byte   0x8c     ;  DW_OP_breg28
        .sleb128 1

This says that the address of my_variable is to be computed
by adding 1 to the value of register r28.  But this is not
actually correct. Registers on AVR are 1-byte long, whereas
addresses are 2-byte long.  In reality, we should be reading
an address spanning over r28 and r29, and then add 1.

Although the debugging information is at fault in this case,
this used to work.  I also think that we might be hitting
the same problem on platforms that are still using stabs.
It's just very visible on the AVR because registers on this
microcontroller are only 1 byte long (whereas addresses are
2 bytes long).

I've opened a ticket internally for looking at the debugging
info generated by the compiler.  But in the meantime, I propose
to restore the old way of handling multi-register entities:
When trying to fetch the value of an entity from a register, and
the size of that entity is larger than the register value, assume
that the value is to be fetched by reading multiple consecutive
registers.

Not ideal, but I see a better way to preserve support for all
these imprecise compilers out there...

gdb/ChangeLog:

        * findvar.c (value_from_register): Handle the case where
        the size of the entity to be read is larger than the size
        of the register where the entity is supposed to be read from.

Tested on x86_64-linux. No regression.
Thoughts?

Thanks,
-- 
Joel

---
 gdb/findvar.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 8e986f1..c7e4c63 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -668,9 +668,35 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
       v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
 
       /* Get the data.  */
-      v2 = get_frame_register_value (frame, regnum);
+      if (len > register_size (gdbarch, regnum))
+	{
+	  /* This is probably a situation where the data is stored over
+	     multiple registers.  We normally expect the debugging info
+	     to describe this multi-register layout explicitly, but this
+	     is not always the case - either because the debugging info
+	     format does not support that level of detail (stabs) or
+	     because the compiler is generating incorrect debugging info.
+	     Try to be as helpful as possible, by assuming that the data
+	     is stored over consecutive registers.  */
+	  int optim, unavail, ok;
+
+	  ok = get_frame_register_bytes (frame, regnum, value_offset (v),
+					 len, value_contents_raw (v),
+					 &optim, &unavail);
+	  if (!ok)
+	    {
+	      if (optim)
+		set_value_optimized_out (v, 1);
+	      if (unavail)
+		mark_value_bytes_unavailable (v, 0, TYPE_LENGTH (type));
+	    }
+	}
+      else
+	{
+	  v2 = get_frame_register_value (frame, regnum);
 
-      value_contents_copy (v, 0, v2, value_offset (v), len);
+	  value_contents_copy (v, 0, v2, value_offset (v), len);
+	}
     }
 
   return v;
-- 
1.7.1

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

Hi Joel.

This is a bit a step backwards in that it doesn't allow
marking parts of the value as unavailable when the type
is longer than one register.  get_frame_register_value
was invented to allow for partially available registers.

> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -668,9 +668,35 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
>        v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
>  
>        /* Get the data.  */
> -      v2 = get_frame_register_value (frame, regnum);
> +      if (len > register_size (gdbarch, regnum))
> +       {

I'd rather we get rid of get_frame_register_bytes.
That is, pass down a value to get_frame_register_bytes (renaming
it along the way) instead of a buffer, and have it do basically
the same, but filling the value contents instead of writting to the buffer,
and have it mark the value pieces that are unavailable instead of
bailing out with error.  (May need to pass two different offsets
to it, one of register offset, other for value embedded offset,
not sure).  We could then reimplement get_frame_register_bytes as a
wrapper for the new  function as an interim until we get rid of
get_frame_register_bytes completely.  Something like:

int
get_frame_register_bytes (*type, regnum, offset, len, *myaddr,
                          *optimizedp, *unavailablep)
{
  val = allocate_value (type);
  read_frame_register_value?(v, frame, regnum, offset, len,
                             val_contents_all_raw (val));
  if (value_optimized_out (val))
    *optimizedp = 1;
  if (value_bytes_available (val, offset, len))
    *unavailablep = 1;
  if (!*optimizedp && !*unavailablep)
    {
      memcpy (myaddr, val_contents_all_raw (val) + offset, len);
      return 1;
    }
   return 0;
}

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

> This is a bit a step backwards in that it doesn't allow
> marking parts of the value as unavailable when the type
> is longer than one register.  get_frame_register_value
> was invented to allow for partially available registers.

I understand.

The bummer part is that I'm entering my busiest time of the year,
and I haven't followed all this close enough to whip up a quick fix.
So I'll keep that on my list to look at. Note that AVR will
have to remain completely broken. We'll just have to add a note
if we release 7.4 with that problem.

-- 
Joel

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

On Thursday 06 October 2011 21:10:53, Joel Brobecker wrote:
> > This is a bit a step backwards in that it doesn't allow
> > marking parts of the value as unavailable when the type
> > is longer than one register.  get_frame_register_value
> > was invented to allow for partially available registers.
> 
> I understand.
> 
> The bummer part is that I'm entering my busiest time of the year,
> and I haven't followed all this close enough to whip up a quick fix.
> So I'll keep that on my list to look at. Note that AVR will
> have to remain completely broken. We'll just have to add a note
> if we release 7.4 with that problem.

Well, your patch doesn't really make it worse, so I won't
object if you want to put it in.  It's not perfect, but it's
better than what we have now, which doesn't work even
for fully available values.

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

> Well, your patch doesn't really make it worse, so I won't
> object if you want to put it in.  It's not perfect, but it's
> better than what we have now, which doesn't work even
> for fully available values.

Thanks!  But let me see what I can do. I might have a little bit
more time mid-to-late November. Worse case scenario, we can put
this workaround in the 7.4 branch...

-- 
Joel

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

On Friday 07 October 2011 17:38:12, Joel Brobecker wrote:

> Worse case scenario, we can put this workaround in the 7.4 branch...

That I'll object in principle.  :-)  We should not put fixes in
a branch that we don't fix on the trunk.  That's bad practice that
lends itself to a bug being forgotten and reappearing on the
next release.  Best put it both on the trunk and on the branch, and
then replace the fix with a better one on the trunk when we have it.

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

Hi Pedro,

> This is a bit a step backwards in that it doesn't allow
> marking parts of the value as unavailable when the type
> is longer than one register.  get_frame_register_value
> was invented to allow for partially available registers.
> 
> > --- a/gdb/findvar.c
> > +++ b/gdb/findvar.c
> > @@ -668,9 +668,35 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
> >        v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
> >  
> >        /* Get the data.  */
> > -      v2 = get_frame_register_value (frame, regnum);
> > +      if (len > register_size (gdbarch, regnum))
> > +       {
> 
> I'd rather we get rid of get_frame_register_bytes.

Purely in terms of solving the AVR problem, what do you think
of the attached patch? Does it look correct to you?

I tested it on AVR as well as x86_64-linux, and it seems to work.

Going beyond that, the new function doesn't provide the extended
interface that you suggest. Doing so seems to be complicating
the implementation more than it's worth. I think that what we
should do, we want to eliminate get_frame_register_value, is look
at the current uses and try to eliminate them.

The biggest culprit is the register_to_value gdbarch method (11
hits). But there is only one location where it's actually called,
and it is.... value_from_register! (just above the section of code
that we're improving). I think it would be easy to change the
profile of that method to return a value. Then the register_to_value
implementations could use get_frame_register_value instead.

Other two uses that are different:
  - dwarf2loc.c: For DW_OP_piece (read/write) support;
  - spu-tdep.c: We just read the contents of a single register
    (get_frame_register_value + extract_unsigned_integer,
    so it should be easy to replace them with something else.

Thanks,
-- 
Joel

[-- Attachment #2: multi-register-value.diff --]
[-- Type: text/x-diff, Size: 2807 bytes --]

commit 1c14f39bb70a05475bb9dfaf5e21103772a9605f
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Fri Oct 21 14:34:54 2011 -0700

    handle variables stored in muliple consecutive registers
    
    gdb/ChangeLog:
    
            * value.c (read_frame_register_value): New function.
            * findvar.c (value_from_register): Use read_frame_register_value
            instead of get_frame_register_value + value_contents_copy
            to get value contents.

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 8e986f1..a417c02 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -661,16 +661,11 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
     }
   else
     {
-      int len = TYPE_LENGTH (type);
-      struct value *v2;
-
       /* Construct the value.  */
       v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
 
       /* Get the data.  */
-      v2 = get_frame_register_value (frame, regnum);
-
-      value_contents_copy (v, 0, v2, value_offset (v), len);
+      read_frame_register_value (v, frame);
     }
 
   return v;
diff --git a/gdb/value.c b/gdb/value.c
index 087cdfd..8dc9258 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3140,6 +3140,35 @@ using_struct_return (struct gdbarch *gdbarch,
 	  != RETURN_VALUE_REGISTER_CONVENTION);
 }
 
+/* VALUE must be an lval_register value.  If regnum is the value's
+   associated register number, and len the length of the values type,
+   read one or more registers in FRAME, starting with register REGNUM,
+   until we've read LEN bytes.  */
+
+void
+read_frame_register_value (struct value *value, struct frame_info *frame)
+{
+  int offset = 0;
+  int regnum = value->regnum;
+  const int len = TYPE_LENGTH (value_type (value));
+
+  gdb_assert (value->lval == lval_register);
+
+  while (offset < len)
+    {
+      struct value *regval = get_frame_register_value (frame, regnum);
+      int reg_len = TYPE_LENGTH (value_type (regval));
+
+      if (offset + reg_len > len)
+        reg_len = len - offset;
+      value_contents_copy (value, offset, regval, value_offset (regval),
+			   reg_len);
+
+      offset += reg_len;
+      regnum++;
+    }
+}
+
 /* Set the initialized field in a value struct.  */
 
 void
diff --git a/gdb/value.h b/gdb/value.h
index 5d61a0b..a933958 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -493,6 +493,9 @@ extern struct value *value_from_register (struct type *type, int regnum,
 extern CORE_ADDR address_from_register (struct type *type, int regnum,
 					struct frame_info *frame);
 
+extern void read_frame_register_value (struct value *value,
+				       struct frame_info *frame);
+
 extern struct value *value_of_variable (struct symbol *var, struct block *b);
 
 extern struct value *address_of_variable (struct symbol *var, struct block *b);

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

On Saturday 22 October 2011 00:38:02, Joel Brobecker wrote:

> Purely in terms of solving the AVR problem, what do you think
> of the attached patch? Does it look correct to you?
> 
> I tested it on AVR as well as x86_64-linux, and it seems to work.
> 
> Going beyond that, the new function doesn't provide the extended
> interface that you suggest. Doing so seems to be complicating
> the implementation more than it's worth. I think that what we
> should do, we want to eliminate get_frame_register_value, is look
> at the current uses and try to eliminate them.

Yeah.

> The biggest culprit is the register_to_value gdbarch method (11
> hits). But there is only one location where it's actually called,
> and it is.... value_from_register! (just above the section of code
> that we're improving). I think it would be easy to change the
> profile of that method to return a value. 
> Then the register_to_value implementations could use get_frame_register_value instead.

Yeah, though I suspect that your new read_frame_register_value
method may evolve into looking what I suggested anyway.  :-)

> Other two uses that are different:
>   - dwarf2loc.c: For DW_OP_piece (read/write) support;

Looks fixable.  Hmm, I like this.

>   - spu-tdep.c: We just read the contents of a single register
>     (get_frame_register_value + extract_unsigned_integer,
>     so it should be easy to replace them with something else.


> diff --git a/gdb/value.c b/gdb/value.c
> index 087cdfd..8dc9258 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3140,6 +3140,35 @@ using_struct_return (struct gdbarch *gdbarch,
>           != RETURN_VALUE_REGISTER_CONVENTION);
>  }
>  
> +/* VALUE must be an lval_register value.  If regnum is the value's
> +   associated register number, and len the length of the values type,
> +   read one or more registers in FRAME, starting with register REGNUM,
> +   until we've read LEN bytes.  */
> +
> +void
> +read_frame_register_value (struct value *value, struct frame_info *frame)
> +{

I think this should be in frame.c instead.  value.c is for core
struct value stuff.

> +  int offset = 0;
> +  int regnum = value->regnum;
> +  const int len = TYPE_LENGTH (value_type (value));

Do we need check_typedefs here?

> +  gdb_assert (value->lval == lval_register);
> +
> +  while (offset < len)
> +    {
> +      struct value *regval = get_frame_register_value (frame, regnum);
> +      int reg_len = TYPE_LENGTH (value_type (regval));
> +
> +      if (offset + reg_len > len)
> +        reg_len = len - offset;
> +      value_contents_copy (value, offset, regval, value_offset (regval),
> +                          reg_len);
> +
> +      offset += reg_len;
> +      regnum++;
> +    }
> +}

Otherwise looks good to me.  Thanks!

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

Hi Pedro,

Thanks for the review. One question:

> > +/* VALUE must be an lval_register value.  If regnum is the value's
> > +   associated register number, and len the length of the values type,
> > +   read one or more registers in FRAME, starting with register REGNUM,
> > +   until we've read LEN bytes.  */
> > +
> > +void
> > +read_frame_register_value (struct value *value, struct frame_info *frame)
> 
> I think this should be in frame.c instead.  value.c is for core
> struct value stuff.

That's what I thought originally too.  The reason why I didn't put
that function there is because I thought that the only way to access
some of the fields was by using the deprecated_[...]_hack functions.
So I thought we weren't supposed to be able to access those components
of a struct value.  But looking closer, I think I get the reason why
it's called a hack and deprecated - it's to allow the previous usage
of using the VALUE_something macros to change the value of the
associated component. So I'm assuming that...

        regnum = VALUE_REGNUM (val)

... is OK. While...

        VALUE_REGNUM (val) = regnum

... is definitely frowned upon.

I will make that change if you agree.

> > +  const int len = TYPE_LENGTH (value_type (value));
> 
> Do we need check_typedefs here?

I haven't faced a situation where this might make a difference,
but I think you are right. When taking the length of a type,
it should never be a typedef.  One might even wonder if it would
make sense to adjust TYPE_LENGTH to to a check_typedef systematically...

-- 
Joel

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

On Tuesday 25 October 2011 21:30:22, Joel Brobecker wrote:
> Hi Pedro,
> 
> Thanks for the review. One question:
> 
> > > +/* VALUE must be an lval_register value.  If regnum is the value's
> > > +   associated register number, and len the length of the values type,
> > > +   read one or more registers in FRAME, starting with register REGNUM,
> > > +   until we've read LEN bytes.  */
> > > +
> > > +void
> > > +read_frame_register_value (struct value *value, struct frame_info *frame)
> > 
> > I think this should be in frame.c instead.  value.c is for core
> > struct value stuff.
> 
> That's what I thought originally too.  The reason why I didn't put
> that function there is because I thought that the only way to access
> some of the fields was by using the deprecated_[...]_hack functions.
> So I thought we weren't supposed to be able to access those components
> of a struct value.  But looking closer, I think I get the reason why
> it's called a hack and deprecated - it's to allow the previous usage
> of using the VALUE_something macros to change the value of the
> associated component. So I'm assuming that...
> 
>         regnum = VALUE_REGNUM (val)
> 
> ... is OK. While...
> 
>         VALUE_REGNUM (val) = regnum
> 
> ... is definitely frowned upon.

Correct.  Ideally we'd replace those hacks by separate 
setter and a getter, or better yet, see if we can get
rid of the need to have a setter.  VALUE_REGNUM as an
rval is definitely okay, as an lval, not so okay.

> I will make that change if you agree.
> 
> > > +  const int len = TYPE_LENGTH (value_type (value));
> > 
> > Do we need check_typedefs here?
> 
> I haven't faced a situation where this might make a difference,
> but I think you are right. When taking the length of a type,
> it should never be a typedef.  

I'd think we'd reach here with typedef'ed variables that lives in
a registers, but maybe we're stripping typedefs earlier per chance.

> One might even wonder if it would
> make sense to adjust TYPE_LENGTH to to a check_typedef systematically...

Maybe.  I guess that we'd need profiling to make sure that wouldn't
regress performance.  I don't know if we have paths were we'd
needlessly call check_typedef more times more.

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

Here is a new version of the patch that reads values over multiple
registers if needed. You suggested that read_frame_register_value
should be in frame.c, but it seemed more logical to put it in findvar.
I think that the proximity with the ther frame_register routines
made it more natural, whereas I couldn't find a natural place where
the new routine would fit in frame.c.  But I can move it to frame.c
if you think it's a better place.

I have also started looking at converting the register_to_value
gdbarch method to using values instead of buffer and a couple of
parameters. Not very difficult, but not trivial either. That makes
me worried about introducing bugs during the conversion, and
I won't be able to test most configurations.

I'll send a patch as a follow-up email to this one.  It's not complete,
but gives us an idea, and we can decide whether we want to continue
or not (FYI: I will have very little time for this within the next
couple of weeks).

gdb/ChangeLog:

        * value.h (read_frame_register_value): Add declaration.
        * findvar.c (read_frame_register_value): New function.
        (value_from_register): Use read_frame_register_value
        instead of get_frame_register_value + value_contents_copy
        to get value contents.

Tested on AVR using AdaCore's testsuite. Tested on x86_64-linux
using the official testsuite.

OK?

Thanks,
-- 
Joel

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

> I have also started looking at converting the register_to_value
> gdbarch method to using values instead of buffer and a couple of
> parameters. Not very difficult, but not trivial either. That makes
> me worried about introducing bugs during the conversion, and
> I won't be able to test most configurations.
> 
> I'll send a patch as a follow-up email to this one.  It's not complete,
> but gives us an idea, and we can decide whether we want to continue
> or not (FYI: I will have very little time for this within the next
> couple of weeks).

So, here is the patch. I only did the conversion for alpha and
i386/i387. It shows the benefits, but it also reveals that the
implementation can be a little awkward, because the support routines
that are used of course where implemented using buffers rather than
values.  And in the end, the transformations need to be performed
on the contents of those buffers, so in their context, it might
even make some kind of sense.

Overall, this still seems like a step in the right direction, but
I am definitely concerned about introducing bugs on platforms that
I cannot test...

So, what do you think we should do?

-- 
Joel

[-- Attachment #2: wip-register_to_value.diff --]
[-- Type: text/x-diff, Size: 10874 bytes --]

commit 50bc176d304fef14f4823670e5ce6075c64419d7
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Oct 26 11:11:56 2011 -0700

    WIP (gdbarch_register_to_value)

diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index b6b9cf8..e67bdec 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -492,7 +492,7 @@ F:int:get_longjmp_target:struct frame_info *frame, CORE_ADDR *pc:frame, pc
 v:int:believe_pcc_promotion:::::::
 #
 m:int:convert_register_p:int regnum, struct type *type:regnum, type:0:generic_convert_register_p::0
-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:struct value *:register_to_value:struct frame_info *frame, int regnum, struct type *type:frame, regnum, type: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
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7fedca7..975377f 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -402,8 +402,8 @@ typedef int (gdbarch_convert_register_p_ftype) (struct gdbarch *gdbarch, int reg
 extern int gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type);
 extern void set_gdbarch_convert_register_p (struct gdbarch *gdbarch, gdbarch_convert_register_p_ftype *convert_register_p);
 
-typedef int (gdbarch_register_to_value_ftype) (struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep);
-extern int gdbarch_register_to_value (struct gdbarch *gdbarch, struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep);
+typedef struct value * (gdbarch_register_to_value_ftype) (struct frame_info *frame, int regnum, struct type *type);
+extern struct value * gdbarch_register_to_value (struct gdbarch *gdbarch, struct frame_info *frame, int regnum, struct type *type);
 extern void set_gdbarch_register_to_value (struct gdbarch *gdbarch, gdbarch_register_to_value_ftype *register_to_value);
 
 typedef void (gdbarch_value_to_register_ftype) (struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index db4d882..cfa55b0 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -2339,14 +2339,14 @@ set_gdbarch_convert_register_p (struct gdbarch *gdbarch,
   gdbarch->convert_register_p = convert_register_p;
 }
 
-int
-gdbarch_register_to_value (struct gdbarch *gdbarch, struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep)
+struct value *
+gdbarch_register_to_value (struct gdbarch *gdbarch, struct frame_info *frame, int regnum, struct type *type)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->register_to_value != NULL);
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_register_to_value called\n");
-  return gdbarch->register_to_value (frame, regnum, type, buf, optimizedp, unavailablep);
+  return gdbarch->register_to_value (frame, regnum, type);
 }
 
 void
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 54e7b4a..5b8e679 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -663,8 +663,6 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
 
   if (gdbarch_convert_register_p (gdbarch, regnum, type1))
     {
-      int optim, unavail, ok;
-
       /* The ISA/ABI need to something weird when obtaining the
          specified value from this register.  It might need to
          re-order non-adjacent, starting with REGNUM (see MIPS and
@@ -672,21 +670,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
          the corresponding [integer] type (see Alpha).  The assumption
          is that gdbarch_register_to_value populates the entire value
          including the location.  */
-      v = allocate_value (type);
-      VALUE_LVAL (v) = lval_register;
-      VALUE_FRAME_ID (v) = get_frame_id (frame);
-      VALUE_REGNUM (v) = regnum;
-      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
-				      value_contents_raw (v), &optim,
-				      &unavail);
-
-      if (!ok)
-	{
-	  if (optim)
-	    set_value_optimized_out (v, 1);
-	  if (unavail)
-	    mark_value_bytes_unavailable (v, 0, TYPE_LENGTH (type));
-	}
+      v = gdbarch_register_to_value (gdbarch, frame, regnum, type1);
     }
   else
     {
diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c
index 2e4bb6f..3913ca7 100644
--- a/gdb/alpha-tdep.c
+++ b/gdb/alpha-tdep.c
@@ -211,7 +211,10 @@ alpha_lds (struct gdbarch *gdbarch, void *out, const void *in)
 }
 
 /* Similarly, this represents exactly the conversion performed by
-   the STS instruction.  */
+   the STS instruction.
+
+   Note: IN and OUT can be equal, in which case the buffer contents
+   will simply be replaced by the value after conversion.  */
 
 static void
 alpha_sts (struct gdbarch *gdbarch, void *out, const void *in)
@@ -238,25 +241,23 @@ alpha_convert_register_p (struct gdbarch *gdbarch, int regno,
 	  && TYPE_LENGTH (type) != 8);
 }
 
-static int
+static struct value *
 alpha_register_to_value (struct frame_info *frame, int regnum,
-			 struct type *valtype, gdb_byte *out,
-			int *optimizedp, int *unavailablep)
+			 struct type *valtype)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  gdb_byte in[MAX_REGISTER_SIZE];
+  struct value *value;
 
-  /* Convert to TYPE.  */
-  if (!get_frame_register_bytes (frame, regnum, 0,
-				 register_size (gdbarch, regnum),
-				 in, optimizedp, unavailablep))
-    return 0;
+  value = gdbarch_value_from_register (gdbarch, valtype, regnum, frame);
+  read_frame_register_value (value, frame);
+  if (value_optimized_out (value) || !value_entirely_available (value))
+    return value;
 
   if (TYPE_LENGTH (valtype) == 4)
     {
-      alpha_sts (gdbarch, out, in);
-      *optimizedp = *unavailablep = 0;
-      return 1;
+      gdb_byte *contents = value_contents_writeable (value);
+      alpha_sts (gdbarch, contents, contents);
+      return value;
     }
 
   error (_("Cannot retrieve value from floating point register"));
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index a381a51..eedcb29 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3106,39 +3106,20 @@ i386_convert_register_p (struct gdbarch *gdbarch,
 /* Read a value of type TYPE from register REGNUM in frame FRAME, and
    return its contents in TO.  */
 
-static int
+static struct value *
 i386_register_to_value (struct frame_info *frame, int regnum,
-			struct type *type, gdb_byte *to,
-			int *optimizedp, int *unavailablep)
+			struct type *type)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
-  int len = TYPE_LENGTH (type);
+  struct value *value;
 
   if (i386_fp_regnum_p (gdbarch, regnum))
-    return i387_register_to_value (frame, regnum, type, to,
-				   optimizedp, unavailablep);
-
-  /* Read a value spread across multiple registers.  */
-
-  gdb_assert (len > 4 && len % 4 == 0);
-
-  while (len > 0)
-    {
-      gdb_assert (regnum != -1);
-      gdb_assert (register_size (gdbarch, regnum) == 4);
-
-      if (!get_frame_register_bytes (frame, regnum, 0,
-				     register_size (gdbarch, regnum),
-				     to, optimizedp, unavailablep))
-	return 0;
+    return i387_register_to_value (frame, regnum, type);
 
-      regnum = i386_next_regnum (regnum);
-      len -= 4;
-      to += 4;
-    }
-
-  *optimizedp = *unavailablep = 0;
-  return 1;
+  value = gdbarch_value_from_register (gdbarch, type, regnum, frame);
+  read_frame_register_value (value, frame);
+  
+  return value;
 }
 
 /* Write the contents FROM of a value of type TYPE into register
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index c4ace82..e505421 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -307,12 +307,12 @@ i387_convert_register_p (struct gdbarch *gdbarch, int regnum,
 /* Read a value of type TYPE from register REGNUM in frame FRAME, and
    return its contents in TO.  */
 
-int
+struct value *
 i387_register_to_value (struct frame_info *frame, int regnum,
-			struct type *type, gdb_byte *to,
-			int *optimizedp, int *unavailablep)
+			struct type *type)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct value *value;
   gdb_byte from[I386_MAX_REGISTER_SIZE];
 
   gdb_assert (i386_fp_regnum_p (gdbarch, regnum));
@@ -321,19 +321,24 @@ i387_register_to_value (struct frame_info *frame, int regnum,
   if (TYPE_CODE (type) != TYPE_CODE_FLT)
     {
       warning (_("Cannot convert floating-point register value "
-	       "to non-floating-point type."));
-      *optimizedp = *unavailablep = 0;
-      return 0;
+               "to non-floating-point type."));
+      value = allocate_value (type);
+      mark_value_bytes_unavailable (value, 0,
+                                    TYPE_LENGTH (check_typedef (type)));
+				    
+      return value;
     }
 
   /* Convert to TYPE.  */
-  if (!get_frame_register_bytes (frame, regnum, 0, TYPE_LENGTH (type),
-				 from, optimizedp, unavailablep))
-    return 0;
-
-  convert_typed_floating (from, i387_ext_type (gdbarch), to, type);
-  *optimizedp = *unavailablep = 0;
-  return 1;
+  value = gdbarch_value_from_register (gdbarch, type, regnum, frame);
+  read_frame_register_value (value, frame);
+  if (value_optimized_out (value) || !value_entirely_available (value))
+    return value;
+
+  memcpy (from, value_contents (value), TYPE_LENGTH (check_typedef (type)));
+  convert_typed_floating (from, i387_ext_type (gdbarch),
+                          value_contents_writeable (value), type);
+  return value;
 }
 
 /* Write the contents FROM of a value of type TYPE into register
diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
index b876549..d317a25 100644
--- a/gdb/i387-tdep.h
+++ b/gdb/i387-tdep.h
@@ -26,6 +26,7 @@ struct frame_info;
 struct regcache;
 struct type;
 struct ui_file;
+struct value;
 
 /* Number of i387 floating point registers.  */
 #define I387_NUM_REGS	16
@@ -63,12 +64,10 @@ extern void i387_print_float_info (struct gdbarch *gdbarch,
 extern int i387_convert_register_p (struct gdbarch *gdbarch, int regnum,
 				    struct type *type);
 
-/* Read a value of type TYPE from register REGNUM in frame FRAME, and
-   return its contents in TO.  */
+/* Return a value of type TYPE from register REGNUM in frame FRAME.  */
 
-extern int i387_register_to_value (struct frame_info *frame, int regnum,
-				   struct type *type, gdb_byte *to,
-				   int *optimizedp, int *unavailablep);
+extern struct value *i387_register_to_value (struct frame_info *frame,
+					     int regnum, struct type *type);
 
 /* Write the contents FROM of a value of type TYPE into register
    REGNUM in frame FRAME.  */

Re: [RFA/RFC] Restore old handling of multi-register variables

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Overall, this still seems like a step in the right direction, but
Joel> I am definitely concerned about introducing bugs on platforms that
Joel> I cannot test...

Some ideas about future directions here came up during the AVX thread:

    http://sourceware.org/ml/gdb-patches/2011-07/msg00351.html

In particular, Ulrich said he had a patch that, IMO, went pretty far in
the unification direction:

    http://sourceware.org/ml/gdb-patches/2011-07/msg00605.html

Tom

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

> In particular, Ulrich said he had a patch that, IMO, went pretty far in
> the unification direction:
> 
>     http://sourceware.org/ml/gdb-patches/2011-07/msg00605.html

Seems like an even better cleanup indeed. I will commit my patch,
and let Ulrich finish with his...

-- 
Joel

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

> gdb/ChangeLog:
> 
>         * value.h (read_frame_register_value): Add declaration.
>         * findvar.c (read_frame_register_value): New function.
>         (value_from_register): Use read_frame_register_value
>         instead of get_frame_register_value + value_contents_copy
>         to get value contents.

(sigh)

-- 
Joel

[-- Attachment #2: multi-register-value.diff --]
[-- Type: text/x-diff, Size: 2836 bytes --]

commit 7f0a0ff60859de96d30b20d1462841a1f2283a72
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Fri Oct 21 14:34:54 2011 -0700

    handle variables stored in muliple consecutive registers
    
    gdb/ChangeLog:
    
            * value.h (read_frame_register_value): Add declaration.
            * findvar.c (read_frame_register_value): New function.
            (value_from_register): Use read_frame_register_value
            instead of get_frame_register_value + value_contents_copy
            to get value contents.

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 8e986f1..54e7b4a 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -623,6 +623,35 @@ default_value_from_register (struct type *type, int regnum,
   return value;
 }
 
+/* VALUE must be an lval_register value.  If regnum is the value's
+   associated register number, and len the length of the values type,
+   read one or more registers in FRAME, starting with register REGNUM,
+   until we've read LEN bytes.  */
+
+void
+read_frame_register_value (struct value *value, struct frame_info *frame)
+{
+  int offset = 0;
+  int regnum = VALUE_REGNUM (value);
+  const int len = TYPE_LENGTH (check_typedef (value_type (value)));
+
+  gdb_assert (VALUE_LVAL (value) == lval_register);
+
+  while (offset < len)
+    {
+      struct value *regval = get_frame_register_value (frame, regnum);
+      int reg_len = TYPE_LENGTH (value_type (regval));
+
+      if (offset + reg_len > len)
+        reg_len = len - offset;
+      value_contents_copy (value, offset, regval, value_offset (regval),
+			   reg_len);
+
+      offset += reg_len;
+      regnum++;
+    }
+}
+
 /* Return a value of type TYPE, stored in register REGNUM, in frame FRAME.  */
 
 struct value *
@@ -661,16 +690,11 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
     }
   else
     {
-      int len = TYPE_LENGTH (type);
-      struct value *v2;
-
       /* Construct the value.  */
       v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
 
       /* Get the data.  */
-      v2 = get_frame_register_value (frame, regnum);
-
-      value_contents_copy (v, 0, v2, value_offset (v), len);
+      read_frame_register_value (v, frame);
     }
 
   return v;
@@ -695,3 +719,4 @@ address_from_register (struct type *type, int regnum, struct frame_info *frame)
 
   return result;
 }
+
diff --git a/gdb/value.h b/gdb/value.h
index f18390e..d2c58ec 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -502,6 +502,9 @@ extern struct value *default_value_from_register (struct type *type,
 						  int regnum,
 						  struct frame_info *frame);
 
+extern void read_frame_register_value (struct value *value,
+				       struct frame_info *frame);
+
 extern struct value *value_from_register (struct type *type, int regnum,
 					  struct frame_info *frame);
 

Re: [RFA/RFC] Restore old handling of multi-register variables

[Pedro Alves]

On Wednesday 26 October 2011 22:37:26, Joel Brobecker wrote:
> Here is a new version of the patch that reads values over multiple
> registers if needed. You suggested that read_frame_register_value
> should be in frame.c, but it seemed more logical to put it in findvar.
> I think that the proximity with the ther frame_register routines
> made it more natural, whereas I couldn't find a natural place where
> the new routine would fit in frame.c.  
> But I can move it to frame.c if you think it's a better place.

That's fine.  As long as it's not in value.c :-).

findvar.c is a weird file.  It seems to contains a mix of two
or three different things.  But that's precedent.

> gdb/ChangeLog:
> 
>         * value.h (read_frame_register_value): Add declaration.
>         * findvar.c (read_frame_register_value): New function.
>         (value_from_register): Use read_frame_register_value
>         instead of get_frame_register_value + value_contents_copy
>         to get value contents.
> 
> Tested on AVR using AdaCore's testsuite. Tested on x86_64-linux
> using the official testsuite.
> 
> OK?

Looks good to me.

-- 
Pedro Alves

Re: [RFA/RFC] Restore old handling of multi-register variables

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * value.h (read_frame_register_value): Add declaration.
> >         * findvar.c (read_frame_register_value): New function.
> >         (value_from_register): Use read_frame_register_value
> >         instead of get_frame_register_value + value_contents_copy
> >         to get value contents.
> > 
> > Tested on AVR using AdaCore's testsuite. Tested on x86_64-linux
> > using the official testsuite.
>
> Looks good to me.

Thanks! Checked in.

-- 
Joel

[RFA] read_frame_register_value and big endian arches

[Joel Brobecker]

Hello,

I am afraid I introduced a regression on big-endian architectures
(Eg. sparc-solaris, erc32-elf) after my latest patch:
    [RFA/RFC] Restore old handling of multi-register variables
    http://www.sourceware.org/ml/gdb-patches/2011-10/msg00709.html

It's just an oversight: The problem appears when trying to get the value
of an entity stored inside a register, and when the size of the entity
is smaller than the size of the register.

    For instance, the testcase shows a parameter whose type is
    a 2-byte integer.  This parameter is passed by register, in
    a 4-byte register.

In that case, the new function read_frame_register_value was always
reading from the first bytes of the register, which is wrong for
big-endian architectures.

gdb/ChangeLog:

        * findvar.c (read_frame_register_value): Read the correct bytes
        from registers on big-endian architectures.

gdb/testsuite/ChangeLog:

        * gdb.ada/small_reg_param: New testcase.

Tested on x86_64-linux. Also tested on erc32-elf and sparc-solaris,
but with AdaCore's testsuite (I am still forbidden from running
the testsuite on any of our Solaris machine because it crashes
the system really bad). I also checked the patch against AVR,
and make sure that it does not create any regression there.

Also included is a new testcase which, when run manually, reproduces
the problem on erc32-elf, and probably on sparc-solaris too.

---
 gdb/findvar.c                                 |   14 ++++++--
 gdb/testsuite/gdb.ada/small_reg_param.exp     |   46 +++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/small_reg_param/foo.adb |   21 +++++++++++
 gdb/testsuite/gdb.ada/small_reg_param/pck.adb |   22 ++++++++++++
 gdb/testsuite/gdb.ada/small_reg_param/pck.ads |   22 ++++++++++++
 5 files changed, 122 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/small_reg_param.exp
 create mode 100644 gdb/testsuite/gdb.ada/small_reg_param/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/small_reg_param/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/small_reg_param/pck.ads

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 54e7b4a..426b488 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -641,11 +641,19 @@ read_frame_register_value (struct value *value, struct frame_info *frame)
     {
       struct value *regval = get_frame_register_value (frame, regnum);
       int reg_len = TYPE_LENGTH (value_type (regval));
+      int reg_offset = 0;
 
+      /* If the register length is larger than the number of bytes
+         remaining to copy, then only copy the appropriate bytes.  */
       if (offset + reg_len > len)
-        reg_len = len - offset;
-      value_contents_copy (value, offset, regval, value_offset (regval),
-			   reg_len);
+	{
+	  reg_len = len - offset;
+	  if (gdbarch_byte_order (get_frame_arch (frame)) == BFD_ENDIAN_BIG)
+	    reg_offset = TYPE_LENGTH (value_type (regval)) - reg_len;
+	}
+
+      value_contents_copy (value, offset, regval,
+                           value_offset (regval) + reg_offset, reg_len);
 
       offset += reg_len;
       regnum++;
diff --git a/gdb/testsuite/gdb.ada/small_reg_param.exp b/gdb/testsuite/gdb.ada/small_reg_param.exp
new file mode 100644
index 0000000..00b066b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/small_reg_param.exp
@@ -0,0 +1,46 @@
+# Copyright 2011 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+set testdir "small_reg_param"
+set testfile "${testdir}/foo"
+set srcfile ${srcdir}/${subdir}/${testfile}.adb
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug optimize=-O1]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto main] then {
+  perror "Couldn't run ${testfile}"
+  return
+} 
+
+gdb_breakpoint "call_me"
+
+# Continue until we hit the breakpoint inside `Call_Me'.  We verify
+# that the parameter value is correct.
+gdb_test "continue" \
+    "Breakpoint .*, pck\\.call_me \\(w=50\\) at .*/pck.adb:.*" \
+    "continue to call_me"
+
+# And just to make sure, we also verify that the parameter value
+# is correct when we print it manually.
+gdb_test "print w" " = 50"
+
diff --git a/gdb/testsuite/gdb.ada/small_reg_param/foo.adb b/gdb/testsuite/gdb.ada/small_reg_param/foo.adb
new file mode 100644
index 0000000..e6b1f53
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/small_reg_param/foo.adb
@@ -0,0 +1,21 @@
+--  Copyright 2011 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with Pck; use Pck;
+
+procedure Foo is
+begin
+   Call_Me (50);
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/small_reg_param/pck.adb b/gdb/testsuite/gdb.ada/small_reg_param/pck.adb
new file mode 100644
index 0000000..55455c7
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/small_reg_param/pck.adb
@@ -0,0 +1,22 @@
+--  Copyright 2011 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package body Pck is
+   Last_Word : Word := 0;
+   procedure Call_Me (W : Word) is
+   begin
+      Last_Word := W;
+   end Call_Me;
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/small_reg_param/pck.ads b/gdb/testsuite/gdb.ada/small_reg_param/pck.ads
new file mode 100644
index 0000000..7ae299a
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/small_reg_param/pck.ads
@@ -0,0 +1,22 @@
+--  Copyright 2011 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package Pck is
+
+   type Word is range 0 .. 16#FFFF#;
+   for Word'size use 16;
+
+   procedure Call_Me (W : Word);
+end Pck;
-- 
1.7.1

Re: [RFA] read_frame_register_value and big endian arches

[Pedro Alves]

Hi Joel,

On Monday 31 October 2011 01:03:16, Joel Brobecker wrote:

> gdb/ChangeLog:
> 
>         * findvar.c (read_frame_register_value): Read the correct bytes
>         from registers on big-endian architectures.

Looks good to me.

> +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug optimize=-O1]] != "" } {

Does the test really depend on the compiler doing the right optimizations?

-- 
Pedro Alves

Re: [RFA] read_frame_register_value and big endian arches

[Joel Brobecker]

Hi Pedro,

> > gdb/ChangeLog:
> > 
> >         * findvar.c (read_frame_register_value): Read the correct bytes
> >         from registers on big-endian architectures.
> 
> Looks good to me.

Thanks for double-checking my change...

> > +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug optimize=-O1]] != "" } {
> 
> Does the test really depend on the compiler doing the right optimizations?

Yes. I haven't looked at the unoptimized version, but my guess is
that the parameter value gets immediately pushed to the stack, thus
hiding the problem.

-- 
Joel

Checked in: [RFA] read_frame_register_value and big endian arches

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * findvar.c (read_frame_register_value): Read the correct bytes
> >         from registers on big-endian architectures.
> 
> Looks good to me.

Now checked in. Thanks again for the review...

-- 
Joel

Re: Checked in: [RFA] read_frame_register_value and big endian arches

[Ulrich Weigand]

Joel Brobecker wrote:
> > > gdb/ChangeLog:
> > > 
> > >         * findvar.c (read_frame_register_value): Read the correct bytes
> > >         from registers on big-endian architectures.
> > 
> > Looks good to me.
> 
> Now checked in. Thanks again for the review...

It seems this completely broke reading of register variables on SPU.  These
are in the high-order element of the (always vector) registers, even though
we have a big-endian architecture.

It seems the underlying problem is that your new read_frame_register_value
routine completely ignores the value_offset of the lval_register value.
This is the place where this information is encoded: note that for
big-endian architectures, value_offset is already correct, so if you'd
respect it, you wouldn't need any special-purpose big-endian code.

Also, if you'd respect value_offset, the SPU special cases would work.

In general, if V is a value with value_lval (V) == lval_register, its
contents must be retrieved by reading the register(s) starting with
number VALUE_REGNUM (V) in their "natural" sizes, skipping the first
value_offset (V) bytes of the concatenated result, and using the
next TYPE_LENGTH (value_type (V)) bytes as the value contents.


If you look at the original code your patch:
http://www.sourceware.org/ml/gdb-patches/2011-10/msg00713.html
replaced, it does take value_offset into account:

      /* Get the data.  */
      v2 = get_frame_register_value (frame, regnum);

      value_contents_copy (v, 0, v2, value_offset (v), len);

While get_frame_register_value always reads the full register,
the value_contents_copy call then extracts only the part
starting at value_offset.

Note that to handle multi-register values, you may actually
need to skip full registers if value_offset starts out bigger
than the size of a register.  See the get_frame_register_bytes
routine how this should be handled.

Note that before Tom's patch here:
http://sourceware.org/ml/gdb-patches/2011-07/msg00351.html
the value_from_register routine actually called
get_frame_register_bytes to handle all those situations ...

The only problematic thing with get_frame_register_bytes is that
it doesn't create a value as output, and therefore does not
propagate precise availability information.  The new
read_frame_register_value should probably implement this
(or maybe even replace get_frame_register_bytes).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Checked in: [RFA] read_frame_register_value and big endian arches

[Joel Brobecker]

> It seems this completely broke reading of register variables on SPU.  These
> are in the high-order element of the (always vector) registers, even though
> we have a big-endian architecture.

Ooops, sorry about that.

> It seems the underlying problem is that your new
> read_frame_register_value routine completely ignores the value_offset
> of the lval_register value.

Yeah, I'm not very with that part of struct values.

> note that for big-endian architectures, value_offset is already
> correct, so if you'd respect it, you wouldn't need any special-purpose
> big-endian code.
> 
> Also, if you'd respect value_offset, the SPU special cases would work.

I will try to fix ASAP. I am sorry if that caused some fustration
when trying to do your testing. I didn't want to do the work in
the first place but I, too, got my testing broken by someone else's
change!

-- 
Joel

Re: Checked in: [RFA] read_frame_register_value and big endian arches

[Ulrich Weigand]

Joel Brobecker wrote:

> > note that for big-endian architectures, value_offset is already
> > correct, so if you'd respect it, you wouldn't need any special-purpose
> > big-endian code.
> > 
> > Also, if you'd respect value_offset, the SPU special cases would work.
> 
> I will try to fix ASAP. I am sorry if that caused some fustration
> when trying to do your testing. I didn't want to do the work in
> the first place but I, too, got my testing broken by someone else's
> change!

No problem :-)

I've now implemented a fix for read_frame_register_value by simply
following the same logic as get_frame_register_bytes does.

This fixes the regressions on SPU for me.  Does this work for the
platforms where you were seeing issues as well?

Bye,
Ulrich


ChangeLog:

	* findvar.c (read_frame_register_value): Respect value_offset
	of the register value.  Remove big-endian special case.

Index: gdb/findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.139
diff -u -p -r1.139 findvar.c
--- gdb/findvar.c	10 Nov 2011 17:14:40 -0000	1.139
+++ gdb/findvar.c	18 Nov 2011 16:49:05 -0000
@@ -631,31 +631,37 @@ default_value_from_register (struct type
 void
 read_frame_register_value (struct value *value, struct frame_info *frame)
 {
+  struct gdbarch *gdbarch = get_frame_arch (frame);
   int offset = 0;
+  int reg_offset = value_offset (value);
   int regnum = VALUE_REGNUM (value);
   const int len = TYPE_LENGTH (check_typedef (value_type (value)));
 
   gdb_assert (VALUE_LVAL (value) == lval_register);
 
-  while (offset < len)
+  /* Skip registers wholly inside of REG_OFFSET.  */
+  while (reg_offset >= register_size (gdbarch, regnum))
+    {
+      reg_offset -= register_size (gdbarch, regnum);
+      regnum++;
+    }
+
+  /* Copy the data.  */
+  while (len > 0)
     {
       struct value *regval = get_frame_register_value (frame, regnum);
-      int reg_len = TYPE_LENGTH (value_type (regval));
-      int reg_offset = 0;
+      int reg_len = TYPE_LENGTH (value_type (regval)) - reg_offset;
 
       /* If the register length is larger than the number of bytes
          remaining to copy, then only copy the appropriate bytes.  */
-      if (offset + reg_len > len)
-	{
-	  reg_len = len - offset;
-	  if (gdbarch_byte_order (get_frame_arch (frame)) == BFD_ENDIAN_BIG)
-	    reg_offset = TYPE_LENGTH (value_type (regval)) - reg_len;
-	}
+      if (reg_len > len)
+	reg_len = len;
 
-      value_contents_copy (value, offset, regval,
-                           value_offset (regval) + reg_offset, reg_len);
+      value_contents_copy (value, offset, regval, reg_offset, reg_len);
 
       offset += reg_len;
+      len -= reg_len;
+      reg_offset = 0;
       regnum++;
     }
 }


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: Checked in: [RFA] read_frame_register_value and big endian arches

[Joel Brobecker]

> This fixes the regressions on SPU for me.  Does this work for the
> platforms where you were seeing issues as well?

Thanks for doing this, Ulrich. I tested the patch on x86_64-linux
for kicks, but also on sparc-elf and avr, where the problems where
detected, and no regression.

> ChangeLog:
> 
> 	* findvar.c (read_frame_register_value): Respect value_offset
> 	of the register value.  Remove big-endian special case.

Just one tiny detail:

> +  struct gdbarch *gdbarch = get_frame_arch (frame);
>    int offset = 0;
> +  int reg_offset = value_offset (value);
>    int regnum = VALUE_REGNUM (value);
>    const int len = TYPE_LENGTH (check_typedef (value_type (value)));
     ^^^^^

I had to remove the const, since we are decrementing len each time
we read contents from a register...

-- 
Joel

[commit] Re: Checked in: [RFA] read_frame_register_value and big endian arches

[Ulrich Weigand]

Joel Brobecker wrote:
> > This fixes the regressions on SPU for me.  Does this work for the
> > platforms where you were seeing issues as well?
> 
> Thanks for doing this, Ulrich. I tested the patch on x86_64-linux
> for kicks, but also on sparc-elf and avr, where the problems where
> detected, and no regression.

Thanks for the verification!

> Just one tiny detail:
> 
> > +  struct gdbarch *gdbarch = get_frame_arch (frame);
> >    int offset = 0;
> > +  int reg_offset = value_offset (value);
> >    int regnum = VALUE_REGNUM (value);
> >    const int len = TYPE_LENGTH (check_typedef (value_type (value)));
>      ^^^^^
> 
> I had to remove the const, since we are decrementing len each time
> we read contents from a register...

Oops.  I noticed this when building, and fixed it in my source
tree, but then forgot to regenerate the patch ...  Sorry.

I've now checked in the version below.

Bye,
Ulrich


ChangeLog:

	* findvar.c (read_frame_register_value): Respect value_offset
	of the register value.  Remove big-endian special case.

Index: gdb/findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.139
diff -u -p -r1.139 findvar.c
--- gdb/findvar.c	10 Nov 2011 17:14:40 -0000	1.139
+++ gdb/findvar.c	18 Nov 2011 20:01:55 -0000
@@ -631,31 +631,37 @@ default_value_from_register (struct type
 void
 read_frame_register_value (struct value *value, struct frame_info *frame)
 {
+  struct gdbarch *gdbarch = get_frame_arch (frame);
   int offset = 0;
+  int reg_offset = value_offset (value);
   int regnum = VALUE_REGNUM (value);
-  const int len = TYPE_LENGTH (check_typedef (value_type (value)));
+  int len = TYPE_LENGTH (check_typedef (value_type (value)));
 
   gdb_assert (VALUE_LVAL (value) == lval_register);
 
-  while (offset < len)
+  /* Skip registers wholly inside of REG_OFFSET.  */
+  while (reg_offset >= register_size (gdbarch, regnum))
+    {
+      reg_offset -= register_size (gdbarch, regnum);
+      regnum++;
+    }
+
+  /* Copy the data.  */
+  while (len > 0)
     {
       struct value *regval = get_frame_register_value (frame, regnum);
-      int reg_len = TYPE_LENGTH (value_type (regval));
-      int reg_offset = 0;
+      int reg_len = TYPE_LENGTH (value_type (regval)) - reg_offset;
 
       /* If the register length is larger than the number of bytes
          remaining to copy, then only copy the appropriate bytes.  */
-      if (offset + reg_len > len)
-	{
-	  reg_len = len - offset;
-	  if (gdbarch_byte_order (get_frame_arch (frame)) == BFD_ENDIAN_BIG)
-	    reg_offset = TYPE_LENGTH (value_type (regval)) - reg_len;
-	}
+      if (reg_len > len)
+	reg_len = len;
 
-      value_contents_copy (value, offset, regval,
-                           value_offset (regval) + reg_offset, reg_len);
+      value_contents_copy (value, offset, regval, reg_offset, reg_len);
 
       offset += reg_len;
+      len -= reg_len;
+      reg_offset = 0;
       regnum++;
     }
 }


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com