Wrong data type in function unpack_varlen_hex()

Wrong data type in function unpack_varlen_hex()

[zhigang gong]

Hi,

        I am working on an mips4k platform. When I added hw watchpoint
support for my target board and debug it with gdb. The write
watchpoint works fine, but rwatch and awatch doesn't work. After trace
the source code of the gdb, I found there is a bug in
unpack_varlen_hex. The local variable retval is a signed integer. For
my case, the ULONGEST is a 64bit integer type. So when the
watchpoint's address is 0x8XXXXXXX, the "retval" will be 0x8XXXXXXX,
and pass its value to variable "result", the "result"'s value will be
sign extended to 0xFFFFFFFF8XXXXXXX. Then when i set a rwatch point,
the address matching will fail when the read watchpoint ocurred.
     The patch is as belows. And I test it,


--- src/gdb/remote.c    2006-05-06 04:08:45.000000000 +0800
+++ insight-6550-060529/gdb/remote.c    2006-06-15 19:26:46.000000000 +0800
@@ -1125,7 +1125,7 @@
                   ULONGEST *result)
 {
   int nibble;
-  int retval = 0;
+  ULONGEST retval = 0;

   while (ishex (*buff, &nibble))
     {

--- src/gdb/ChangeLog   2006-05-28 13:56:50.000000000 +0800
+++ insight-6550-060529/gdb/ChangeLog   2006-06-16 09:19: 55.000000000 +0800
@@ -1,3 +1,8 @@
+2006-06-16  Zhigang Gong < zhigang.gong@gmail.com>
+       * remote.c(unpack_varlen_hex):
+       Change the type of local variable "retval" to ULONGEST, so it
will avoid
+       incorrect sign extending.
+
 2006-05-28  Alexandre Oliva  < aoliva@redhat.com>


Best Regards,
Zhigang Gong

Re: Wrong data type in function unpack_varlen_hex()

[Daniel Jacobowitz]

On Fri, Jun 16, 2006 at 08:58:33PM +0800, zhigang gong wrote:
> Hi,
> 
>        I am working on an mips4k platform. When I added hw watchpoint
> support for my target board and debug it with gdb. The write
> watchpoint works fine, but rwatch and awatch doesn't work. After trace
> the source code of the gdb, I found there is a bug in
> unpack_varlen_hex. The local variable retval is a signed integer. For
> my case, the ULONGEST is a 64bit integer type. So when the
> watchpoint's address is 0x8XXXXXXX, the "retval" will be 0x8XXXXXXX,
> and pass its value to variable "result", the "result"'s value will be
> sign extended to 0xFFFFFFFF8XXXXXXX. Then when i set a rwatch point,
> the address matching will fail when the read watchpoint ocurred.
>     The patch is as belows. And I test it,

Thank you for the patch.  This is definitely an improvement, so I have
committed it.

There may be more problems here: addresses on MIPS are generally
considered to be signed, so it might be a bug somewhere else in GDB
that you're getting 0x000000008xxxxxxx.  But, we'll worry about that
only if it causes a problem for someone else.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Wrong data type in function unpack_varlen_hex()

[zhigang gong]

On 7/13/06, Daniel Jacobowitz <drow@false.org> wrote:
> On Fri, Jun 16, 2006 at 08:58:33PM +0800, zhigang gong wrote:
> > Hi,
> >
> >        I am working on an mips4k platform. When I added hw watchpoint
> > support for my target board and debug it with gdb. The write
> > watchpoint works fine, but rwatch and awatch doesn't work. After trace
> > the source code of the gdb, I found there is a bug in
> > unpack_varlen_hex. The local variable retval is a signed integer. For
> > my case, the ULONGEST is a 64bit integer type. So when the
> > watchpoint's address is 0x8XXXXXXX, the "retval" will be 0x8XXXXXXX,
> > and pass its value to variable "result", the "result"'s value will be
> > sign extended to 0xFFFFFFFF8XXXXXXX. Then when i set a rwatch point,
> > the address matching will fail when the read watchpoint ocurred.
> >     The patch is as belows. And I test it,
>
> Thank you for the patch.  This is definitely an improvement, so I have
> committed it.
>
> There may be more problems here: addresses on MIPS are generally
> considered to be signed, so it might be a bug somewhere else in GDB
> that you're getting 0x000000008xxxxxxx.  But, we'll worry about that
> only if it causes a problem for someone else.
I have worried about that too. As I foud that in gdb the signed extention should
be the default way. And I also found the watchpoint address
0x000000008xxxxxxx was calculated from the address expression, which
is inputted in the command line as belows:
gdb ) rwatch  *0x8xxxxxxx
Maybe  change the watchpoint address expression calculating, make it
to do an signed extension, is better.

>
> --
> Daniel Jacobowitz
> CodeSourcery
>

Re: Wrong data type in function unpack_varlen_hex()

[Daniel Jacobowitz]

On Thu, Jul 13, 2006 at 11:19:01PM +0800, zhigang gong wrote:
> I have worried about that too. As I foud that in gdb the signed extention 
> should
> be the default way. And I also found the watchpoint address
> 0x000000008xxxxxxx was calculated from the address expression, which
> is inputted in the command line as belows:
> gdb ) rwatch  *0x8xxxxxxx
> Maybe  change the watchpoint address expression calculating, make it
> to do an signed extension, is better.

You may want to test again with HEAD then.  As it happens, Corinna
submitted another patch which also went in today, and her patch changes
the MIPS port to sign extend "*0x8xxxxxxx" by default.

Corinna, what do you think?  The context is the remote protocol reply
"rwatch:0x80000000"; a bug previously caused this to be sign extended,
but now that's been fixed.  So I'm afraid we now have the same bug you
were both trying to fix back again.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Wrong data type in function unpack_varlen_hex()

[Daniel Jacobowitz]

On Thu, Jul 13, 2006 at 11:23:49AM -0400, Daniel Jacobowitz wrote:
> Corinna, what do you think?  The context is the remote protocol reply
> "rwatch:0x80000000"; a bug previously caused this to be sign extended,
> but now that's been fixed.  So I'm afraid we now have the same bug you
> were both trying to fix back again.

Maybe we should be using gdbarch_integer_to_address here too?

Or maybe that hook ought to be completely removed and consolidated with
something else.  There's a whole bunch of related hooks.  With your
recent change, mips_integer_to_address basically matches
signed_pointer_to_address.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Wrong data type in function unpack_varlen_hex()

[zhigang gong]

Hi,

   I checked HEAD out and tested it just now. The "rwatch" and
"awatch" both work well.



On 7/13/06, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Jul 13, 2006 at 11:23:49AM -0400, Daniel Jacobowitz wrote:
> > Corinna, what do you think?  The context is the remote protocol reply
> > "rwatch:0x80000000"; a bug previously caused this to be sign extended,
> > but now that's been fixed.  So I'm afraid we now have the same bug you
> > were both trying to fix back again.
>
> Maybe we should be using gdbarch_integer_to_address here too?
>
> Or maybe that hook ought to be completely removed and consolidated with
> something else.  There's a whole bunch of related hooks.  With your
> recent change, mips_integer_to_address basically matches
> signed_pointer_to_address.
>
> --
> Daniel Jacobowitz
> CodeSourcery
>

Re: Wrong data type in function unpack_varlen_hex()

[Daniel Jacobowitz]

On Sat, Jul 15, 2006 at 06:33:31PM +0800, zhigang gong wrote:
> Hi,
> 
>   I checked HEAD out and tested it just now. The "rwatch" and
> "awatch" both work well.

Thanks!  Well, then, I guess we'll ignore the problem unless it becomes
a difficulty.

-- 
Daniel Jacobowitz
CodeSourcery