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

Tom Tromey
2022-12-20T21:03:23Z

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

Thanks for the patch.

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

> The cache object is unique for each function and frame, so if there are
> more than one function pointer stored in the dwarf2_frame_cache->reg
> array, then the appropriate pointer will be supplied (the type is not
> known by the dwarf2 implementation).

Does this ever happen? If not perhaps a simpler approach would be
better.

> +struct dwarf2_frame_fn_data
> +{

New type should have a comment.

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

Shouldn't this use the fn_prev_register typedef?

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

Wrong placement of the "*", but really a lot of the code isn't following
the GNU / gdb style.

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

Normally new functions get a comment referring to the header where they
are declared.

> +
> +/* 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,

I think these comments could perhaps be expanded a bit.

Tom