[PATCH] mips-tdep.c: Fix bug in evaluating signed address

[PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Corinna Vinschen]

Hi,

the below patch fixes the mips_integer_to_address function.  What
happens is this:

If the incoming type is an *unsigned* value, then unpack_long takes the
(un)signedness into account.  This results in `val' not being signed
extended, as the mips_integer_to_address function was originally
designed for.

So, to enforce correct signedness of the resulting address, I changed
the unpack_long call to extract_signed_integer.  This way, val is always
sign extended and the mips address is correctly signed.


Ok to apply?


Thanks,
Corinna


	* mips-tdep.c (mips_integer_to_address): Call extract_signed_integer
	instead of unpack_long to maintain signedness.


Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/cvsfiles/gnupro/gdb/mips-tdep.c,v
retrieving revision 1.24
diff -u -p -r1.24 mips-tdep.c
--- gdb/mips-tdep.c	15 Jun 2006 08:57:17 -0000	1.24
+++ gdb/mips-tdep.c	16 Jun 2006 16:15:48 -0000
@@ -4749,7 +4749,7 @@ mips_integer_to_address (struct gdbarch 
 			 struct type *type, const gdb_byte *buf)
 {
   gdb_byte *tmp = alloca (TYPE_LENGTH (builtin_type_void_data_ptr));
-  LONGEST val = unpack_long (type, buf);
+  LONGEST val = extract_signed_integer (buf, TYPE_LENGTH (type));
   store_signed_integer (tmp, TYPE_LENGTH (builtin_type_void_data_ptr), val);
   return extract_signed_integer (tmp,
 				 TYPE_LENGTH (builtin_type_void_data_ptr));

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Corinna Vinschen]

Ping?

On Jun 16 18:23, Corinna Vinschen wrote:
> If the incoming type is an *unsigned* value, then unpack_long takes the
> (un)signedness into account.  This results in `val' not being signed
> extended, as the mips_integer_to_address function was originally
> designed for.

I missed to add the fact that this can be demonstrated using an existing
testcase, gdb.base/ending-run.exp with mips64-elf.  There's a statement

  (gdb) cle *0xa0123456

Which fails with

  No breakpoint at 0xa0123456

The reason is that the breakpoint address is correctly stored sign
extended (0xffffffffa0123456), but the evaluation of "*0xa0123456"
evaluates the expression 0xa0123456 as unsigned int type.  This in
turn leads to the above described behaviour.

> So, to enforce correct signedness of the resulting address, I changed
> the unpack_long call to extract_signed_integer.  This way, val is always
> sign extended and the mips address is correctly signed.
> 
> 
> Ok to apply?
> 
> 
> Thanks,
> Corinna
> 
> 
> 	* mips-tdep.c (mips_integer_to_address): Call extract_signed_integer
> 	instead of unpack_long to maintain signedness.
> 
> 
> Index: gdb/mips-tdep.c
> ===================================================================
> RCS file: /cvs/cvsfiles/gnupro/gdb/mips-tdep.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 mips-tdep.c
> --- gdb/mips-tdep.c	15 Jun 2006 08:57:17 -0000	1.24
> +++ gdb/mips-tdep.c	16 Jun 2006 16:15:48 -0000
> @@ -4749,7 +4749,7 @@ mips_integer_to_address (struct gdbarch 
>  			 struct type *type, const gdb_byte *buf)
>  {
>    gdb_byte *tmp = alloca (TYPE_LENGTH (builtin_type_void_data_ptr));
> -  LONGEST val = unpack_long (type, buf);
> +  LONGEST val = extract_signed_integer (buf, TYPE_LENGTH (type));
>    store_signed_integer (tmp, TYPE_LENGTH (builtin_type_void_data_ptr), val);
>    return extract_signed_integer (tmp,
>  				 TYPE_LENGTH (builtin_type_void_data_ptr));

Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Daniel Jacobowitz]

On Fri, Jun 16, 2006 at 06:23:32PM +0200, Corinna Vinschen wrote:
> Hi,
> 
> the below patch fixes the mips_integer_to_address function.  What
> happens is this:
> 
> If the incoming type is an *unsigned* value, then unpack_long takes the
> (un)signedness into account.  This results in `val' not being signed
> extended, as the mips_integer_to_address function was originally
> designed for.
> 
> So, to enforce correct signedness of the resulting address, I changed
> the unpack_long call to extract_signed_integer.  This way, val is always
> sign extended and the mips address is correctly signed.

I might be missing something, but isn't the function mostly redundant
now?

> Index: gdb/mips-tdep.c
> ===================================================================
> RCS file: /cvs/cvsfiles/gnupro/gdb/mips-tdep.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 mips-tdep.c
> --- gdb/mips-tdep.c	15 Jun 2006 08:57:17 -0000	1.24
> +++ gdb/mips-tdep.c	16 Jun 2006 16:15:48 -0000
> @@ -4749,7 +4749,7 @@ mips_integer_to_address (struct gdbarch 
>  			 struct type *type, const gdb_byte *buf)
>  {
>    gdb_byte *tmp = alloca (TYPE_LENGTH (builtin_type_void_data_ptr));
> -  LONGEST val = unpack_long (type, buf);
> +  LONGEST val = extract_signed_integer (buf, TYPE_LENGTH (type));
>    store_signed_integer (tmp, TYPE_LENGTH (builtin_type_void_data_ptr), val);
>    return extract_signed_integer (tmp,
>  				 TYPE_LENGTH (builtin_type_void_data_ptr));

Those last two have got to be a no-op.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Corinna Vinschen]

On Jul 12 15:31, Daniel Jacobowitz wrote:
> On Fri, Jun 16, 2006 at 06:23:32PM +0200, Corinna Vinschen wrote:
> > Hi,
> > 
> > the below patch fixes the mips_integer_to_address function.  What
> > happens is this:
> > 
> > If the incoming type is an *unsigned* value, then unpack_long takes the
> > (un)signedness into account.  This results in `val' not being signed
> > extended, as the mips_integer_to_address function was originally
> > designed for.
> > 
> > So, to enforce correct signedness of the resulting address, I changed
> > the unpack_long call to extract_signed_integer.  This way, val is always
> > sign extended and the mips address is correctly signed.
> 
> I might be missing something, but isn't the function mostly redundant
> now?
> 
> > Index: gdb/mips-tdep.c
> > ===================================================================
> > RCS file: /cvs/cvsfiles/gnupro/gdb/mips-tdep.c,v
> > retrieving revision 1.24
> > diff -u -p -r1.24 mips-tdep.c
> > --- gdb/mips-tdep.c	15 Jun 2006 08:57:17 -0000	1.24
> > +++ gdb/mips-tdep.c	16 Jun 2006 16:15:48 -0000
> > @@ -4749,7 +4749,7 @@ mips_integer_to_address (struct gdbarch 
> >  			 struct type *type, const gdb_byte *buf)
> >  {
> >    gdb_byte *tmp = alloca (TYPE_LENGTH (builtin_type_void_data_ptr));
> > -  LONGEST val = unpack_long (type, buf);
> > +  LONGEST val = extract_signed_integer (buf, TYPE_LENGTH (type));
> >    store_signed_integer (tmp, TYPE_LENGTH (builtin_type_void_data_ptr), val);
> >    return extract_signed_integer (tmp,
> >  				 TYPE_LENGTH (builtin_type_void_data_ptr));
> 
> Those last two have got to be a no-op.

Hmm, yes, you're right.  I didn't realize that when changing unpack_long
to extract_signed_integer.  Thanks, correct version below.


Corinna

	* mips-tdep.c (mips_integer_to_address): Simplify be calling
	extract_signed_integer directly.  Fix comment.


Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.396
diff -u -p -r1.396 mips-tdep.c
--- mips-tdep.c	19 Jun 2006 18:50:09 -0000	1.396
+++ mips-tdep.c	13 Jul 2006 11:36:13 -0000
@@ -4647,19 +4647,14 @@ mips_register_sim_regno (int regnum)
 }
 
 
-/* Convert an integer into an address.  By first converting the value
-   into a pointer and then extracting it signed, the address is
-   guarenteed to be correctly sign extended.  */
+/* Convert an integer into an address.  Extracting the value signed
+   guarantees a correctly sign extended address.  */
 
 static CORE_ADDR
 mips_integer_to_address (struct gdbarch *gdbarch,
 			 struct type *type, const gdb_byte *buf)
 {
-  gdb_byte *tmp = alloca (TYPE_LENGTH (builtin_type_void_data_ptr));
-  LONGEST val = unpack_long (type, buf);
-  store_signed_integer (tmp, TYPE_LENGTH (builtin_type_void_data_ptr), val);
-  return extract_signed_integer (tmp,
-				 TYPE_LENGTH (builtin_type_void_data_ptr));
+  return (CORE_ADDR) extract_signed_integer (buf, TYPE_LENGTH (type));
 }
 
 static void

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Daniel Jacobowitz]

On Thu, Jul 13, 2006 at 01:54:56PM +0200, Corinna Vinschen wrote:
> 
> Hmm, yes, you're right.  I didn't realize that when changing unpack_long
> to extract_signed_integer.  Thanks, correct version below.
> 
> 
> Corinna
> 
> 	* mips-tdep.c (mips_integer_to_address): Simplify be calling
> 	extract_signed_integer directly.  Fix comment.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] mips-tdep.c: Fix bug in evaluating signed address

[Corinna Vinschen]

On Jul 13 09:00, Daniel Jacobowitz wrote:
> On Thu, Jul 13, 2006 at 01:54:56PM +0200, Corinna Vinschen wrote:
> > 	* mips-tdep.c (mips_integer_to_address): Simplify be calling
> > 	extract_signed_integer directly.  Fix comment.
> 
> OK.

Thanks, applied.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat