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

Tom Tromey
2023-01-18T18:47:21Z

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