[RFA] Cached function lookup

[RFA] Cached function lookup

[Klee Dienes]

(This is basically the same patch I sent last week, just updated
to the latest source base.)

This patch allows functions in the target used by GDB ("malloc",
"scm_lookup_cstr", and later a ton of Objective-C functions) to have
their values cached and re-used unless the symbol table has changed
in-between calls.  This is a performance win overall, and a particular
win when dispatching Objective-C method calls and looking up
Objective-C type information from the runtime.

2002-02-04  Klee Dienes <kdienes@apple.com>

         * breakpoint.c (breakpoint_re_set, breakpoint_re_set_all,
         breakpoint_update): Instead of re-parsing all deferred 
breakpoints
         every time breakpoint_re_set is called, increment a generation
         number.  When breakpoints need to be up-to-date, call
         breakpoint_update.  This prevents unnecessary re-parsing of
         breakpoint information (and massive future-break spam) when
         multiple shared libraries are loaded at the same time.

         * breakpoint.h: export symbol_generation, breakpoint_update.

         * gcore.c (default_derive_heap_segment): update to use
         create_cached_function.

         * scm-lang.c (scm_lookup_name): update to use 
create_cached_function.

         * valops.c (create_cached_function, lookup_cached_function): add.
         These functions create a new data type (a `cached_value'), whose
         purpose is to store the lookup of commonly used symbols GDB needs
         from the inferior.  For example, evaluating the expression 'print
         "hello"' causes GDB to call `malloc' in the target.  Looking up
         the symbol for `malloc' takes a non-trivial amount of time, and
         has no need to be done if the symbols have not changed.
         create/lookup_cached_function allow GDB to cache the results of
         these lookups; re-looking them up only when the symbols have
         changed.
         (find_function_in_inferior): add a default type as second
         argument.  This type will be used for the returned value if no
         type information is available for the function (previously, the
         type was assumed to be (char *) (*).
         (value_allocate_space_in_inferior): use new cached_function
         interface.

         * value.h (cached_value) add.
         (create_cached_function) add.
         (lookup_cached_function) add.
         (find_function_in_inferior) update to new signature.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.64
diff -u -r1.64 breakpoint.c
--- breakpoint.c        2002/02/03 11:43:19     1.64
+++ breakpoint.c        2002/02/05 06:37:37
@@ -70,8 +70,12 @@

  static void ignore_command (char *, int);

+void breakpoint_update (void);
+
  static int breakpoint_re_set_one (PTR);

+static void breakpoint_re_set_all (void);
+
  static void clear_command (char *, int);

  static void catch_command (char *, int);
@@ -720,6 +724,7 @@
    static char message1[] = "Error inserting catchpoint %d:\n";
    static char message[sizeof (message1) + 30];

+  breakpoint_update ();

    ALL_BREAKPOINTS_SAFE (b, temp)
    {
@@ -6891,9 +6896,27 @@
    return 0;
  }

-/* Re-set all breakpoints after symbols have been re-loaded.  */
-void
-breakpoint_re_set (void)
+/* Re-set all breakpoints after symbols have been re-loaded.  */
+unsigned int symbol_generation = 1;
+static unsigned int breakpoint_generation = 0;
+
+void breakpoint_update (void)
+{
+  if (breakpoint_generation != symbol_generation) {
+    breakpoint_re_set_all ();
+    breakpoint_generation = symbol_generation;
+  }
+}
+
+void
+breakpoint_re_set (void)
+{
+  symbol_generation++;
+}
+
+/* Re-set all breakpoints after symbols have been re-loaded.  */
+static void
+breakpoint_re_set_all (void)
  {
    struct breakpoint *b, *temp;
    enum language save_language;
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.10
diff -u -r1.10 breakpoint.h
--- breakpoint.h        2001/10/20 23:54:29     1.10
+++ breakpoint.h        2002/02/05 06:37:38
@@ -305,6 +305,8 @@

  typedef struct bpstats *bpstat;

+extern unsigned int symbol_generation;
+
  /* Interface:  */
  /* Clear a bpstat so that it says we are not at any breakpoint.
     Also free any storage that is part of a bpstat.  */
@@ -525,6 +527,8 @@
  extern int breakpoint_thread_match (CORE_ADDR, ptid_t);

  extern void until_break_command (char *, int);
+
+extern void breakpoint_update (void);

  extern void breakpoint_re_set (void);

Index: gdb/gcore.c
===================================================================
RCS file: /cvs/src/src/gdb/gcore.c,v
retrieving revision 1.3
diff -u -r1.3 gcore.c
--- gcore.c     2002/01/14 20:00:48     1.3
+++ gcore.c     2002/02/05 06:37:39
@@ -274,7 +274,7 @@
         }
      }
    /* Now get the top-of-heap by calling sbrk in the inferior.  */
-  if ((sbrk = find_function_in_inferior ("sbrk")) == NULL)
+  if ((sbrk = find_function_in_inferior ("sbrk", NULL)) == NULL)
      return 0;
    if ((zero = value_from_longest (builtin_type_int, (LONGEST) 0)) == 
NULL)
      return 0;
Index: gdb/scm-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/scm-lang.c,v
retrieving revision 1.7
diff -u -r1.7 scm-lang.c
--- scm-lang.c  2002/01/04 05:20:08     1.7
+++ scm-lang.c  2002/02/05 06:37:39
@@ -152,7 +152,7 @@
  {
    struct value *args[3];
    int len = strlen (str);
-  struct value *func;
+  static struct cached_value *func = NULL;
    struct value *val;
    struct symbol *sym;
    args[0] = value_allocate_space_in_inferior (len);
@@ -169,8 +169,9 @@
      /* FIXME in this case, we should try lookup_symbol first */
      args[2] = value_from_longest (builtin_type_scm, SCM_EOL);

-  func = find_function_in_inferior ("scm_lookup_cstr");
-  val = call_function_by_hand (func, 3, args);
+  if (func == NULL)
+    func = create_cached_function ("scm_lookup_cstr", NULL);
+  val = call_function_by_hand (lookup_cached_function (func), 3, args);
    if (!value_logical_not (val))
      return value_ind (val);

@@ -186,14 +187,15 @@
  struct value *
  scm_evaluate_string (char *str, int len)
  {
-  struct value *func;
+  static struct cached_value *func = NULL;
    struct value *addr = value_allocate_space_in_inferior (len + 1);
    LONGEST iaddr = value_as_long (addr);
    write_memory (iaddr, str, len);
    /* FIXME - should find and pass env */
    write_memory (iaddr + len, "", 1);
-  func = find_function_in_inferior ("scm_evstr");
-  return call_function_by_hand (func, 1, &addr);
+  if (func == NULL)
+    func = create_cached_function ("scm_evstr", NULL);
+  return call_function_by_hand (lookup_cached_function (func), 1, 
&addr);
  }

  static struct value *
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.50
diff -u -r1.50 valops.c
--- valops.c    2002/02/04 02:14:46     1.50
+++ valops.c    2002/02/05 06:37:44
@@ -89,11 +89,49 @@
  int unwind_on_signal_p = 0;



+struct cached_value *
+create_cached_function (char *name, struct type *type)
+{
+  struct cached_value *ptr;
+
+  ptr = (struct cached_value *) xmalloc (sizeof (struct cached_value));
+  ptr->name = xstrdup (name);
+  ptr->type = type;
+  memset (&ptr->val, 0, sizeof (struct value));
+  ptr->generation = (unsigned int) -1;
+
+  return ptr;
+}
+
+struct value *
+lookup_cached_function (struct cached_value *cval)
+{
+  struct value *val = NULL;
+  struct value *next = NULL;
+
+  if (cval->generation != symbol_generation)
+    {
+      val = find_function_in_inferior (cval->name, cval->type);
+      cval->val = *val;
+      cval->val.next = NULL;
+      cval->generation = symbol_generation;
+    }
+
+  val = allocate_value (cval->val.type);
+  next = val->next;
+  *val = cval->val;
+  val->next = next;
+
+  return val;
+}

-/* Find the address of function name NAME in the inferior.  */
+/* Find the address of function name NAME in the inferior.  If no type
+   information is available for NAME, use `type' as the type for the
+   resulting value.
+*/

-struct value *
-find_function_in_inferior (char *name)
+struct value *
+find_function_in_inferior (char *name, struct type *type)
  {
    register struct symbol *sym;
    sym = lookup_symbol (name, 0, VAR_NAMESPACE, 0, NULL);
@@ -111,13 +149,18 @@
        struct minimal_symbol *msymbol = lookup_minimal_symbol (name, 
NULL, NULL);
        if (msymbol != NULL)
         {
-         struct type *type;
-         CORE_ADDR maddr;
-         type = lookup_pointer_type (builtin_type_char);
-         type = lookup_function_type (type);
-         type = lookup_pointer_type (type);
-         maddr = SYMBOL_VALUE_ADDRESS (msymbol);
-         return value_from_pointer (type, maddr);
+         if (type != NULL)
+           return value_from_longest (type, (LONGEST) 
SYMBOL_VALUE_ADDRESS (msymbol));
+         else
+           {
+             struct type *type;
+             CORE_ADDR maddr;
+             type = lookup_pointer_type (builtin_type_char);
+             type = lookup_function_type (type);
+             type = lookup_pointer_type (type);
+             maddr = SYMBOL_VALUE_ADDRESS (msymbol);
+             return value_from_pointer (type, maddr);
+           }
         }
        else
         {
@@ -136,10 +179,14 @@
  value_allocate_space_in_inferior (int len)
  {
    struct value *blocklen;
-  struct value *val = find_function_in_inferior ("malloc");
+  struct value *val = NULL;
+  static struct cached_value *fval = NULL;
+
+  if (fval == NULL)
+    fval = create_cached_function ("malloc", NULL);

    blocklen = value_from_longest (builtin_type_int, (LONGEST) len);
-  val = call_function_by_hand (val, 1, &blocklen);
+  val = call_function_by_hand (lookup_cached_function (fval), 1, 
&blocklen);
    if (value_logical_not (val))
      {
        if (!target_has_execution)
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.26
diff -u -r1.26 value.h
--- value.h     2002/01/04 23:21:38     1.26
+++ value.h     2002/02/05 06:37:44
@@ -564,12 +564,24 @@
  extern void find_rt_vbase_offset (struct type *, struct type *, char *, 
int,
                                   int *, int *);

-extern struct value *find_function_in_inferior (char *);
+extern struct value *find_function_in_inferior (char *, struct type *);

  extern struct value *value_allocate_space_in_inferior (int);

  extern CORE_ADDR default_push_arguments (int nargs, struct value ** 
args,
                                          CORE_ADDR sp, int struct_return,
                                          CORE_ADDR struct_addr);
+
+struct cached_value
+{
+  char *name;
+  struct type *type;
+  struct value val;
+  unsigned int generation;
+};
+
+extern struct cached_value *create_cached_function (char *, struct 
type *);
+
+extern struct value *lookup_cached_function (struct cached_value *cval);

  #endif /* !defined (VALUE_H) */

Re: [RFA] Cached function lookup

[Jim Blandy]

Klee Dienes <kdienes@apple.com> writes:
> (This is basically the same patch I sent last week, just updated
> to the latest source base.)
> 
> This patch allows functions in the target used by GDB ("malloc",
> "scm_lookup_cstr", and later a ton of Objective-C functions) to have
> their values cached and re-used unless the symbol table has changed
> in-between calls.  This is a performance win overall, and a particular
> win when dispatching Objective-C method calls and looking up
> Objective-C type information from the runtime.
> 
> 2002-02-04  Klee Dienes <kdienes@apple.com>
> 
>          * breakpoint.c (breakpoint_re_set, breakpoint_re_set_all,
>          breakpoint_update): Instead of re-parsing all deferred
> breakpoints
>          every time breakpoint_re_set is called, increment a generation
>          number.  When breakpoints need to be up-to-date, call
>          breakpoint_update.  This prevents unnecessary re-parsing of
>          breakpoint information (and massive future-break spam) when
>          multiple shared libraries are loaded at the same time.

Let me see if I understand the situation:

The inferior hits a shlib event breakpoint, indicating that it's
loaded or unloaded some stuff.  GDB scans the dynamic linker's table,
and discovers that several new shared libraries have been loaded.

As the code currently stands, GDB will recompute its breakpoints'
addresses after each solib's symbols are read.  Your change makes GDB
delay this until it's about to insert the breakpoints.

Is that correct?

Re: [RFA] Cached function lookup

[Andrew Cagney]

Klee,

I'll follow this up next week. There's a pretty cool idea in this one.

Andrew

> (This is basically the same patch I sent last week, just updated
> to the latest source base.)
> 
> This patch allows functions in the target used by GDB ("malloc",
> "scm_lookup_cstr", and later a ton of Objective-C functions) to have
> their values cached and re-used unless the symbol table has changed
> in-between calls.  This is a performance win overall, and a particular
> win when dispatching Objective-C method calls and looking up
> Objective-C type information from the runtime.
> 
> 2002-02-04  Klee Dienes <kdienes@apple.com>
> 
>         * breakpoint.c (breakpoint_re_set, breakpoint_re_set_all,
>         breakpoint_update): Instead of re-parsing all deferred breakpoints
>         every time breakpoint_re_set is called, increment a generation
>         number.  When breakpoints need to be up-to-date, call
>         breakpoint_update.  This prevents unnecessary re-parsing of
>         breakpoint information (and massive future-break spam) when
>         multiple shared libraries are loaded at the same time.
> 
>         * breakpoint.h: export symbol_generation, breakpoint_update.
> 
>         * gcore.c (default_derive_heap_segment): update to use
>         create_cached_function.
> 
>         * scm-lang.c (scm_lookup_name): update to use create_cached_function.
> 
>         * valops.c (create_cached_function, lookup_cached_function): add.
>         These functions create a new data type (a `cached_value'), whose
>         purpose is to store the lookup of commonly used symbols GDB needs
>         from the inferior.  For example, evaluating the expression 'print
>         "hello"' causes GDB to call `malloc' in the target.  Looking up
>         the symbol for `malloc' takes a non-trivial amount of time, and
>         has no need to be done if the symbols have not changed.
>         create/lookup_cached_function allow GDB to cache the results of
>         these lookups; re-looking them up only when the symbols have
>         changed.
>         (find_function_in_inferior): add a default type as second
>         argument.  This type will be used for the returned value if no
>         type information is available for the function (previously, the
>         type was assumed to be (char *) (*).
>         (value_allocate_space_in_inferior): use new cached_function
>         interface.
> 
>         * value.h (cached_value) add.
>         (create_cached_function) add.
>         (lookup_cached_function) add.
>         (find_function_in_inferior) update to new signature.

Re: [RFA] Cached function lookup

[Andrew Cagney]

> (This is basically the same patch I sent last week, just updated
> to the latest source base.)
> 
> This patch allows functions in the target used by GDB ("malloc",
> "scm_lookup_cstr", and later a ton of Objective-C functions) to have
> their values cached and re-used unless the symbol table has changed
> in-between calls.  This is a performance win overall, and a particular
> win when dispatching Objective-C method calls and looking up
> Objective-C type information from the runtime.

Klee, I think there are three changes in this:

o 
a change to the signature of the function
	find_function_in_inferior().  I guess this is
	either connected the print (float) f(3.0) change
	or something related to an ObjectiveC patch.

	I'll skip that for now.

o 
a change to breakpoint.c so it can more efficiently
	handle symbol table updates (defering things until
	when breakpoint_update is called).

	I'll comment on that separatly - if it is separated
	out there is a much better chance of MichaelS approving
	it :-)

o 
a change add a function value cache

I'll comment on the third, I think with that resolved and committed, it 
should be possible for the second to just fall out.

The patch to breakpoint.c introduced the global variable 
symbol_generation.  I don't think it is a good idea for the function 
cache code to be depending on a global variable controlled by breakpoint.c.

Have a look at the target_new_objfile_hook() chain as a way of notifying 
the function-cache code of symtab updates updates?  (I'm getting the 
feeling that the hook may have been wrongly named (?)).

Can I suggest changing ``struct cached_value { ... }'' in value.h to the 
opaque (and hopefully better named):
	struct cached_function;  /* or cached_function_value???? */

 > +  if (cval->generation != symbol_generation)
 > +    {
 > +      val = find_function_in_inferior (cval->name, cval->type);
 > +      cval->val = *val;
 > +      cval->val.next = NULL;
 > +      cval->generation = symbol_generation;
 > +    }

I think:
	free_value(cval->val)
	cval->val = find.....()
	release_value(cval->val)
is more correct.

cvsl->val would need to become a ``struct value *'' but that is a good 
as as ``struct value'' should be treated as an opaque.

 > +  val = allocate_value (cval->val.type);
 > +  next = val->next;
 > +  *val = cval->val;
 > +  val->next = next;

I think value_copy() is more correct.

enjoy,
Andrew

Re: [RFA] Cached function lookup

[Andrew Cagney]

> This patch allows functions in the target used by GDB ("malloc",
> "scm_lookup_cstr", and later a ton of Objective-C functions) to have
> their values cached and re-used unless the symbol table has changed
> in-between calls.  This is a performance win overall, and a particular
> win when dispatching Objective-C method calls and looking up
> Objective-C type information from the runtime.

For the breakpoint changes, assuming that the function-cache code is 
committed, can the two declarations:

> +extern unsigned int symbol_generation;
> +
>  /* Interface:  */
>  /* Clear a bpstat so that it says we are not at any breakpoint.
>     Also free any storage that is part of a bpstat.  */
> @@ -525,6 +527,8 @@
>  extern int breakpoint_thread_match (CORE_ADDR, ptid_t);
> 
>  extern void until_break_command (char *, int);
> +
> +extern void breakpoint_update (void);

be removed (made static)?

Can I suggest adding a comment explaining that the re_set is delayed 
until the breakpoints are re-inserted.

>  extern void breakpoint_re_set (void);


Hmm, I can think of two race conditions:

o 
with the function-cache code - I don't think
	this matters - the cache is always flushed flushed
	before breakpoint code does the update (and the
	breakpoint code doesn't actually use the function
	cache (?)).

o 
with things like ``info breakpoints''
	Do those functions also need to breakpoint_update()?

enjoy,
Andrew