PATCH: per-inferior register cache for gdbserver

PATCH: per-inferior register cache for gdbserver

[Daniel Jacobowitz]

Moving right along.  This is the initial per-inferior register cache
interface; in my development sources (which are almost ready for
submittal) the interface is actually somewhat different, but that's
entangled with the rest of threading right now.  I'll be extracting it in
the next few days.

So, this patch is a step on the road, but don't get too attached to the
interface.  It'll be much nicer, I promise.  This version works as-is, which
I can't say for the nicer one at the moment.

Committed.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-04-20  Daniel Jacobowitz  <drow@mvista.com>

	* gdbserver/inferiors.c (struct inferior_info): Add regcache_data.
	(add_inferior): Call create_register_cache.
	(clear_inferiors): Call free_register_cache.
	(inferior_regcache_data, set_inferior_regcache_data): New functions.
	* gdbserver/regcache.c (struct inferior_regcache_data): New.
	(registers): Remove.
	(get_regcache): New function.
	(create_register_cache, free_register_cache): New functions.
	(set_register_cache): Don't initialize the register cache here.
	(registers_to_string, registers_from_string, register_data): Call
	get_regcache.
	* gdbserver/regcache.h: Add prototypes.
	* gdbserver/server.h: Likewise.

Index: inferiors.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/inferiors.c,v
retrieving revision 1.2
diff -u -p -r1.2 inferiors.c
--- inferiors.c	20 Apr 2002 17:04:09 -0000	1.2
+++ inferiors.c	20 Apr 2002 17:11:47 -0000
@@ -29,6 +29,7 @@ struct inferior_info
 {
   int pid;
   void *target_data;
+  void *regcache_data;
   struct inferior_info *next;
 };
 
@@ -52,6 +53,8 @@ add_inferior (int pid)
   if (current_inferior == NULL)
     current_inferior = inferiors;
 
+  create_register_cache (new_inferior);
+
   if (signal_pid == 0)
     signal_pid = pid;
 }
@@ -67,6 +70,8 @@ clear_inferiors (void)
 
       if (inf->target_data)
 	free (inf->target_data);
+      if (inf->regcache_data)
+	free_register_cache (inf);
 
       free (inf);
       inf = next_inf;
@@ -85,4 +90,16 @@ void
 set_inferior_target_data (struct inferior_info *inferior, void *data)
 {
   inferior->target_data = data;
+}
+
+void *
+inferior_regcache_data (struct inferior_info *inferior)
+{
+  return inferior->regcache_data;
+}
+
+void
+set_inferior_regcache_data (struct inferior_info *inferior, void *data)
+{
+  inferior->regcache_data = data;
 }
Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/regcache.c,v
retrieving revision 1.3
diff -u -p -r1.3 regcache.c
--- regcache.c	9 Apr 2002 21:11:35 -0000	1.3
+++ regcache.c	20 Apr 2002 17:11:48 -0000
@@ -25,7 +25,11 @@
 #include <stdlib.h>
 #include <string.h>
 
-static char *registers;
+struct inferior_regcache_data
+{
+  char *registers;
+};
+
 static int register_bytes;
 
 static struct reg *reg_defs;
@@ -33,6 +37,19 @@ static int num_registers;
 
 const char **gdbserver_expedite_regs;
 
+static struct inferior_regcache_data *
+get_regcache (struct inferior_info *inf)
+{
+  struct inferior_regcache_data *regcache;
+
+  regcache = (struct inferior_regcache_data *) inferior_regcache_data (inf);
+
+  if (regcache == NULL)
+    fatal ("no register cache");
+
+  return regcache;
+}
+
 int
 registers_length (void)
 {
@@ -40,6 +57,28 @@ registers_length (void)
 }
 
 void
+create_register_cache (struct inferior_info *inferior)
+{
+  struct inferior_regcache_data *regcache;
+
+  regcache = malloc (sizeof (*regcache));
+
+  regcache->registers = malloc (register_bytes);
+  if (regcache->registers == NULL)
+    fatal ("Could not allocate register cache.");
+
+  set_inferior_regcache_data (inferior, regcache);
+}
+
+void
+free_register_cache (struct inferior_info *inferior)
+{
+  free (get_regcache (current_inferior)->registers);
+  free (get_regcache (current_inferior));
+  set_inferior_regcache_data (inferior, NULL);
+}
+
+void
 set_register_cache (struct reg *regs, int n)
 {
   int offset, i;
@@ -55,14 +94,13 @@ set_register_cache (struct reg *regs, in
     }
 
   register_bytes = offset / 8;
-  registers = malloc (offset / 8);
-  if (!registers)
-    fatal ("Could not allocate register cache.");
 }
 
 void
 registers_to_string (char *buf)
 {
+  char *registers = get_regcache (current_inferior)->registers;
+
   convert_int_to_ascii (registers, buf, register_bytes);
 }
 
@@ -70,6 +108,7 @@ void
 registers_from_string (char *buf)
 {
   int len = strlen (buf);
+  char *registers = get_regcache (current_inferior)->registers;
 
   if (len != register_bytes * 2)
     {
@@ -119,6 +158,8 @@ register_size (int n)
 char *
 register_data (int n)
 {
+  char *registers = get_regcache (current_inferior)->registers;
+
   return registers + (reg_defs[n].offset / 8);
 }
 
Index: regcache.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/regcache.h,v
retrieving revision 1.2
diff -u -p -r1.2 regcache.h
--- regcache.h	9 Apr 2002 21:11:35 -0000	1.2
+++ regcache.h	20 Apr 2002 17:11:48 -0000
@@ -21,6 +21,14 @@
 #ifndef REGCACHE_H
 #define REGCACHE_H
 
+/* Create a new register cache for INFERIOR.  */
+
+void create_register_cache (struct inferior_info *inferior);
+
+/* Release all memory associated with the register cache for INFERIOR.  */
+
+void free_register_cache (struct inferior_info *inferior);
+
 /* Convert all registers to a string in the currently specified remote
    format.  */
 
Index: server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.10
diff -u -p -r1.10 server.h
--- server.h	20 Apr 2002 17:04:09 -0000	1.10
+++ server.h	20 Apr 2002 17:11:48 -0000
@@ -54,6 +54,9 @@
    least the size of a (void *).  */
 typedef long long CORE_ADDR;
 
+/* Opaque inferior process information.  */
+struct inferior_info;
+
 #include "regcache.h"
 #include "gdb/signals.h"
 
@@ -70,14 +73,14 @@ extern char *registers;
 
 /* From inferiors.c.  */
 
-struct inferior_info;
 extern struct inferior_info *current_inferior;
 extern int signal_pid;
 void add_inferior (int pid);
 void clear_inferiors (void);
 void *inferior_target_data (struct inferior_info *);
 void set_inferior_target_data (struct inferior_info *, void *);
-
+void *inferior_regcache_data (struct inferior_info *);
+void set_inferior_regcache_data (struct inferior_info *, void *);
 
 /* Public variables in server.c */
 

Re: PATCH: per-inferior register cache for gdbserver

[Andrew Cagney]

> Moving right along.  This is the initial per-inferior register cache
> interface; in my development sources (which are almost ready for
> submittal) the interface is actually somewhat different, but that's
> entangled with the rest of threading right now.  I'll be extracting it in
> the next few days.
> 
> So, this patch is a step on the road, but don't get too attached to the
> interface.  It'll be much nicer, I promise.  This version works as-is, which
> I can't say for the nicer one at the moment.

Um, would there be more benefit in getting this sort of thing 
implemented in the core of GDB?  I just don't see a per-lwp cache in the 
remote target making much difference to GDB's thread debugging performance.

Andrew

Re: PATCH: per-inferior register cache for gdbserver

[Daniel Jacobowitz]

On Sat, Apr 20, 2002 at 02:48:49PM -0400, Andrew Cagney wrote:
> >Moving right along.  This is the initial per-inferior register cache
> >interface; in my development sources (which are almost ready for
> >submittal) the interface is actually somewhat different, but that's
> >entangled with the rest of threading right now.  I'll be extracting it in
> >the next few days.
> >
> >So, this patch is a step on the road, but don't get too attached to the
> >interface.  It'll be much nicer, I promise.  This version works as-is, 
> >which
> >I can't say for the nicer one at the moment.
> 
> Um, would there be more benefit in getting this sort of thing 
> implemented in the core of GDB?  I just don't see a per-lwp cache in the 
> remote target making much difference to GDB's thread debugging performance.

There would be more benefit in doing it in GDB, certainly.  But like a
lot of optimizations I'm doing in the server right now, this is
beneficial to the server itself.  Note that I completely replaced the
global cache; I switch back and forth amongst LWPs during operations,
sometimes.  It simplifies life in the server greatly if I can just say
"give me this thread's current registers" and let the register cache
handle the details.

In GDB, this would be a performance optimization; in the server it's
there for simplicity more than anything else.  It also lets me delay
register fetching for other threads until it's actually needed, which
it may never be (in the next version of the cache, which has valid
flags).  I can get numbers later on how many register fetches it
actually saves; I suspect it's fairly small.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: PATCH: per-inferior register cache for gdbserver

[Andrew Cagney]

> Um, would there be more benefit in getting this sort of thing 
>> implemented in the core of GDB?  I just don't see a per-lwp cache in the 
>> remote target making much difference to GDB's thread debugging performance.


> I can get numbers later on how many register fetches it
> actually saves; I suspect it's fairly small.

Can you please add a comment to the code explaining this - it more of a 
design benefit than a performance benefit (the performance gains are 
unknown).

You'll notice that I'm sitting on the frame cache patch because I'm 
still analyzing its effect.  GDB already contains too much code that 
``claims to improve performance''.

enjoy,
Andrew

Re: PATCH: per-inferior register cache for gdbserver

[Daniel Jacobowitz]

On Sat, Apr 20, 2002 at 09:23:07PM -0400, Andrew Cagney wrote:
> >Um, would there be more benefit in getting this sort of thing 
> >>implemented in the core of GDB?  I just don't see a per-lwp cache in the 
> >>remote target making much difference to GDB's thread debugging 
> >>performance.
> 
> 
> >I can get numbers later on how many register fetches it
> >actually saves; I suspect it's fairly small.
> 
> Can you please add a comment to the code explaining this - it more of a 
> design benefit than a performance benefit (the performance gains are 
> unknown).

Good idea, done.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2002-06-13  Daniel Jacobowitz  <drow@mvista.com>

	* gdbserver/regcache.c: Add comment.

Index: regcache.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/regcache.c,v
retrieving revision 1.5
diff -u -p -r1.5 regcache.c
--- regcache.c	11 Jun 2002 17:32:39 -0000	1.5
+++ regcache.c	13 Jun 2002 19:28:07 -0000
@@ -25,6 +25,10 @@
 #include <stdlib.h>
 #include <string.h>
 
+/* The private data for the register cache.  Note that we have one
+   per inferior; this is primarily for simplicity, as the performance
+   benefit is minimal.  */
+
 struct inferior_regcache_data
 {
   int registers_valid;

Re: PATCH: per-inferior register cache for gdbserver

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Sat, Apr 20, 2002 at 02:48:49PM -0400, Andrew Cagney wrote:
> > >Moving right along.  This is the initial per-inferior register cache
> > >interface; in my development sources (which are almost ready for
> > >submittal) the interface is actually somewhat different, but that's
> > >entangled with the rest of threading right now.  I'll be extracting it in
> > >the next few days.
> > >
> > >So, this patch is a step on the road, but don't get too attached to the
> > >interface.  It'll be much nicer, I promise.  This version works as-is,
> > >which
> > >I can't say for the nicer one at the moment.
> >
> > Um, would there be more benefit in getting this sort of thing
> > implemented in the core of GDB?  I just don't see a per-lwp cache in the
> > remote target making much difference to GDB's thread debugging performance.
> 
> There would be more benefit in doing it in GDB, certainly.  But like a
> lot of optimizations I'm doing in the server right now, this is
> beneficial to the server itself.  Note that I completely replaced the
> global cache; I switch back and forth amongst LWPs during operations,
> sometimes.  It simplifies life in the server greatly if I can just say
> "give me this thread's current registers" and let the register cache
> handle the details.
> 
> In GDB, this would be a performance optimization; in the server it's
> there for simplicity more than anything else.  It also lets me delay
> register fetching for other threads until it's actually needed, which
> it may never be (in the next version of the cache, which has valid
> flags).  I can get numbers later on how many register fetches it
> actually saves; I suspect it's fairly small.

For what it's worth, I explored a lot of things like this
(including exactly this, I think), when I was re-writing
procfs.c.  In the end, they resulted in no measurable performance
improvement at all.   ;-|

But I had fun playing around with them, of course...

Michael