Re: [RFA] Cached function lookup

[Andrew Cagney <ac131313 at cygnus dot com>]

> (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