[patch] gdbserver regcache fetch all regs

[patch] gdbserver regcache fetch all regs

[Aleksandar Ristovski]

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

Hello,

I believe this is a bug in gdbserver/regcache.c

Current code fetches regno 0 and marks regcache valid. I 
believe correct should be: fetch all regs, and mark regache 
valid.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems


ChangeLog:


* regcache.c (get_regcache): Fetch all registers instead of
regno 0 only.

[-- Attachment #2: gdbserver-regcache.c-fetchallregs-20090616.diff --]
[-- Type: text/x-patch, Size: 556 bytes --]

Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/regcache.c,v
retrieving revision 1.19
diff -u -p -r1.19 regcache.c
--- regcache.c	22 Mar 2009 23:57:10 -0000	1.19
+++ regcache.c	17 Jun 2009 01:21:03 -0000
@@ -53,7 +53,7 @@ get_regcache (struct thread_info *inf, i
   /* FIXME - fetch registers for INF */
   if (fetch && regcache->registers_valid == 0)
     {
-      fetch_inferior_registers (0);
+      fetch_inferior_registers (-1);
       regcache->registers_valid = 1;
     }
 

Re: [patch] gdbserver regcache fetch all regs

[Doug Evans]

On Tue, Jun 16, 2009 at 6:24 PM, Aleksandar Ristovski<aristovski@qnx.com> wrote:
> Hello,
>
> I believe this is a bug in gdbserver/regcache.c
>
> Current code fetches regno 0 and marks regcache valid. I believe correct
> should be: fetch all regs, and mark regache valid.
> [...];
> ChangeLog:
>
>
> * regcache.c (get_regcache): Fetch all registers instead of
> regno 0 only.
>

Hi.  At first I thought "Yikes!". :-)
But it turns out that all fetch_registers routines treat 0 and -1 equivalently.
I wouldn't hold up this patch (it's fine with me fwiw), though I would
change the ChangeLog entry to something like: "Use -1 instead of 0 to
fetch all registers." since passing 0 does actually fetch all
registers.

It would be good to remove this oddity and stop the conflation of 0
and -1, but I don't know what would break.
We could run the testsuite and see what happens as a start.
Does anyone know the history behind this?

Re: [patch] gdbserver regcache fetch all regs

[Aleksandar Ristovski]

Doug Evans wrote:
> On Tue, Jun 16, 2009 at 6:24 PM, Aleksandar Ristovski<aristovski@qnx.com> wrote:
>> Hello,
>>
>> I believe this is a bug in gdbserver/regcache.c
>>
>> Current code fetches regno 0 and marks regcache valid. I believe correct
>> should be: fetch all regs, and mark regache valid.
>> [...];
>> ChangeLog:
>>
>>
>> * regcache.c (get_regcache): Fetch all registers instead of
>> regno 0 only.
>>
> 
> Hi.  At first I thought "Yikes!". :-)
> But it turns out that all fetch_registers routines treat 0 and -1 equivalently.
> I wouldn't hold up this patch (it's fine with me fwiw), though I would
> change the ChangeLog entry to something like: "Use -1 instead of 0 to
> fetch all registers." since passing 0 does actually fetch all
> registers.

Very interesting. I see that now, but in target.h it says 
quite unambiguously:

   /* Fetch registers from the inferior process.

      If REGNO is -1, fetch all registers; otherwise, fetch 
at least REGNO.  */

   void (*fetch_registers) (int regno);


> 
> It would be good to remove this oddity and stop the conflation of 0
> and -1, but I don't know what would break.
> We could run the testsuite and see what happens as a start.
> Does anyone know the history behind this?
> 


Thanks,
-- 
Aleksandar Ristovski
QNX Software Systems

Re: [patch] gdbserver regcache fetch all regs

[Daniel Jacobowitz]

On Fri, Jun 19, 2009 at 08:35:13AM -0700, Doug Evans wrote:
> Hi.  At first I thought "Yikes!". :-)
> But it turns out that all fetch_registers routines treat 0 and -1 equivalently.
> I wouldn't hold up this patch (it's fine with me fwiw), though I would
> change the ChangeLog entry to something like: "Use -1 instead of 0 to
> fetch all registers." since passing 0 does actually fetch all
> registers.

Agreed on all counts.

> It would be good to remove this oddity and stop the conflation of 0
> and -1, but I don't know what would break.
> We could run the testsuite and see what happens as a start.
> Does anyone know the history behind this?

Pretty sure nothing will break - since nothing else calls these
functions.  I could be mistaken though.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] gdbserver regcache fetch all regs

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Fri, Jun 19, 2009 at 08:35:13AM -0700, Doug Evans wrote:
>> Hi.  At first I thought "Yikes!". :-)
>> But it turns out that all fetch_registers routines treat 0 and -1 equivalently.
>> I wouldn't hold up this patch (it's fine with me fwiw), though I would
>> change the ChangeLog entry to something like: "Use -1 instead of 0 to
>> fetch all registers." since passing 0 does actually fetch all
>> registers.
> 
> Agreed on all counts.
> 
>> It would be good to remove this oddity and stop the conflation of 0
>> and -1, but I don't know what would break.
>> We could run the testsuite and see what happens as a start.
>> Does anyone know the history behind this?
> 
> Pretty sure nothing will break - since nothing else calls these
> functions.  I could be mistaken though.
> 

Ok, this would be a more complete patch (with cleanups in 
spots where -1 and 0 are used interchangeably).

If you think it would be more appropriate to first commit 
regcache.c change only, and then the cleanup, let me know.




ChangeLog:

* linux-low.c (usr_fetch_inferior_registers): Remove check 
for regno 0.
* proc-service.c (ps_lgetregs): Pass -1 to fetch all registers.
* regcache.c (get_regcache): Likewise.
* spu-low.c (spu_fetch_registers): Remove 0 to -1 conversion.
* win32-low.c (child_fetch_inferior_registers): Remove check 
for regno 0.


-- 
Aleksandar Ristovski
QNX Software Systems


ChangeLog:

* linux-low.c (usr_fetch_inferior_registers): Remove check 
for regno 0.
* proc-service.c (ps_lgetregs): Pass -1 to fetch all registers.
* regcache.c (get_regcache): Likewise.
* spu-low.c (spu_fetch_registers): Remove 0 to -1 conversion.
* win32-low.c (child_fetch_inferior_registers): Remove check 
for regno 0.

[-- Attachment #2: gdbserver-regcache.c-fetchallregs-20090619.diff --]
[-- Type: text/x-patch, Size: 2842 bytes --]

Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.105
diff -u -p -r1.105 linux-low.c
--- linux-low.c	24 May 2009 17:44:19 -0000	1.105
+++ linux-low.c	19 Jun 2009 15:56:05 -0000
@@ -2054,7 +2054,7 @@ error_exit:;
 static void
 usr_fetch_inferior_registers (int regno)
 {
-  if (regno == -1 || regno == 0)
+  if (regno == -1)
     for (regno = 0; regno < the_low_target.num_regs; regno++)
       fetch_register (regno);
   else
Index: proc-service.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/proc-service.c,v
retrieving revision 1.12
diff -u -p -r1.12 proc-service.c
--- proc-service.c	1 Apr 2009 22:50:24 -0000	1.12
+++ proc-service.c	19 Jun 2009 15:56:05 -0000
@@ -110,7 +110,7 @@ ps_lgetregs (gdb_ps_prochandle_t ph, lwp
   save_inferior = current_inferior;
   current_inferior = reg_inferior;
 
-  the_target->fetch_registers (0);
+  the_target->fetch_registers (-1);
   gregset_info()->fill_function (gregset);
 
   current_inferior = save_inferior;
Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/regcache.c,v
retrieving revision 1.19
diff -u -p -r1.19 regcache.c
--- regcache.c	22 Mar 2009 23:57:10 -0000	1.19
+++ regcache.c	19 Jun 2009 15:56:05 -0000
@@ -53,7 +53,7 @@ get_regcache (struct thread_info *inf, i
   /* FIXME - fetch registers for INF */
   if (fetch && regcache->registers_valid == 0)
     {
-      fetch_inferior_registers (0);
+      fetch_inferior_registers (-1);
       regcache->registers_valid = 1;
     }
 
Index: spu-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/spu-low.c,v
retrieving revision 1.24
diff -u -p -r1.24 spu-low.c
--- spu-low.c	3 Apr 2009 14:38:39 -0000	1.24
+++ spu-low.c	19 Jun 2009 15:56:06 -0000
@@ -471,10 +471,6 @@ spu_fetch_registers (int regno)
   int fd;
   CORE_ADDR addr;
 
-  /* ??? Some callers use 0 to mean all registers.  */
-  if (regno == 0)
-    regno = -1;
-
   /* We must be stopped on a spu_run system call.  */
   if (!parse_spufs_run (&fd, &addr))
     return;
Index: win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.35
diff -u -p -r1.35 win32-low.c
--- win32-low.c	1 Apr 2009 22:50:24 -0000	1.35
+++ win32-low.c	19 Jun 2009 15:56:06 -0000
@@ -331,7 +331,7 @@ child_fetch_inferior_registers (int r)
 {
   int regno;
   win32_thread_info *th = thread_rec (current_inferior_ptid (), TRUE);
-  if (r == -1 || r == 0 || r > NUM_REGS)
+  if (r == -1 || r > NUM_REGS)
     child_fetch_inferior_registers (NUM_REGS);
   else
     for (regno = 0; regno < r; regno++)

Re: [patch] gdbserver regcache fetch all regs

[Pedro Alves]

On Friday 19 June 2009 17:09:02, Aleksandar Ristovski wrote:

> Ok, this would be a more complete patch (with cleanups in 
> spots where -1 and 0 are used interchangeably).
> 
> If you think it would be more appropriate to first commit 
> regcache.c change only, and then the cleanup, let me know.

Go ahead and check in as is, no need to split so much.  Please
don't be so afraid of running the testsuite though.

-- 
Pedro Alves

Re: [patch] gdbserver regcache fetch all regs

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Friday 19 June 2009 17:09:02, Aleksandar Ristovski wrote:
> 
>> Ok, this would be a more complete patch (with cleanups in 
>> spots where -1 and 0 are used interchangeably).
>>
>> If you think it would be more appropriate to first commit 
>> regcache.c change only, and then the cleanup, let me know.
> 
> Go ahead and check in as is, no need to split so much.  Please
> don't be so afraid of running the testsuite though.
> 

Committed.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems