Re: [PATCH v2 3/4] gdb: dwarf2 generic implementation for caching function data

[Tom Tromey <tom@tromey.com>]

>>>>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:

> When there is no dwarf2 data for a register, a function can be called
> to provide the value of this register.  In some situations, it might
> not be trivial to determine the value to return and it would cause a
> performance bottleneck to do the computation each time.

> This patch allows the called function to have a "cache" object that it
> can use to store some metadata between calls to reduce the performance
> impact of the complex logic.

> +struct dwarf2_frame_fn_data
> +{

This struct should have some kind of introductory comment, as should the
fields.

> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);

Seems like this should use the fn_prev_register typedef.  (But see below.)

> +  void *data;
> +  struct dwarf2_frame_fn_data* next;

Wrong "*" placement.

> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register fn)

Wrong formatting.

> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +				     void **this_cache,
> +				     fn_prev_register fn, unsigned long size)

Wrong formatting.

> +{
> +  struct dwarf2_frame_fn_data *fn_data = nullptr;
> +  struct dwarf2_frame_cache *cache
> +    = dwarf2_frame_cache (this_frame, this_cache);
> +
> +  /* First try to find an existing object.  */
> +  void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, fn);
> +  if (data)
> +    return data;
> +
> +  /* No object found, lets create a new instance.  */
> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
> +  fn_data->fn = fn;
> +  fn_data->data = frame_obstack_zalloc (size);
> +  fn_data->next = cache->fn_data;
> +  cache->fn_data = fn_data;
> +
> +  return fn_data->data;

This API seems a bit weird to me.

It seems like the 'fn' parameter is never really used.  It's maybe just
a sort of cookie.  But if so, I think it would be better to just use a
'const void *' or 'const char *' or something like that.  (A string is
nice because then it can also be seen in the debugger and give a clue
where it came from.)

Ok, I dug up the follow-up patch and indeed it is just a cookie.  I
think naming it as such and changing the type would make this more
clear.

Also in the follow-up I see that it calls dwarf2_frame_fn_data first.
So if you're going to go that route, then it seems that
dwarf2_frame_allocate_fn_data does not need to find an existing
object -- it can just assert there isn't one.

> +
> +/* Allocate a new instance of the function unique data.  */
> +
> +extern void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +					    void **this_cache,
> +					    fn_prev_register fn,
> +					    unsigned long size);
> +
> +/* Retrieve the function unique data for this frame.  */
> +
> +extern void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame,
> +				       void **this_cache,
> +				       fn_prev_register fn);

IMO both of these could use a longer comment.  From this it's impossible
to tell what the point of them is.

thanks,
Tom