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.

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