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

[Luis Machado <luis.machado@arm.com>]

Hi,

On 11/18/22 15:52, Torbjörn SVENSSON wrote:
> 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.
> 
> 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).
> 
> dwarf2_frame_get_fn_data can be used to retrieve the function unique
> cache object.
> dwarf2_frame_allocate_fn_data can be used to allocate and retrieve the
> function unqiue cache object.

unqiue -> unique

> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> ---
>   gdb/dwarf2/frame.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>   2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 3f884abe1d5..bff3b706e7e 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>   }
>   
>   \f
> +struct dwarf2_frame_fn_data
> +{
> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);
> +  void *data;
> +  struct dwarf2_frame_fn_data* next;
> +};
> +

I'm wondering if we really need to have a function pointer here. Isn't the cache supposed to be frame-wide and not
function-specific?

If we don't need it, the cache just becomes an opaque data pointer.

>   struct dwarf2_frame_cache
>   {
>     /* DWARF Call Frame Address.  */
> @@ -862,6 +870,8 @@ struct dwarf2_frame_cache
>        dwarf2_tailcall_frame_unwind unwinder so this field does not apply for
>        them.  */
>     void *tailcall_cache;
> +
> +  struct dwarf2_frame_fn_data *fn_data;
>   };
>   
>   static struct dwarf2_frame_cache *
> @@ -1221,6 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>       }
>   }
>   
> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register fn)
> +{
> +  struct dwarf2_frame_fn_data *fn_data = nullptr;
> +  struct dwarf2_frame_cache *cache
> +    = dwarf2_frame_cache (this_frame, this_cache);
> +
> +  /* Find the object for the function.  */
> +  for (fn_data = cache->fn_data; fn_data; fn_data = fn_data->next)
> +    if (fn_data->fn == fn)
> +      return fn_data->data;
> +
> +  return nullptr;
> +}
> +
> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +				     void **this_cache,
> +				     fn_prev_register fn, unsigned long size)
> +{
> +  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;
> +}

And if we only have a data pointer, we can return a reference to it through the argument, and then DWARF can cache it.

We could even have a destructor/cleanup that can get called once the frames are destroyed.

> +
>   /* Proxy for tailcall_frame_dealloc_cache for bottom frame of a virtual tail
>      call frames chain.  */
>   
> diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
> index 06c8a10c178..444afd9f8eb 100644
> --- a/gdb/dwarf2/frame.h
> +++ b/gdb/dwarf2/frame.h
> @@ -66,6 +66,9 @@ enum dwarf2_frame_reg_rule
>   
>   /* Register state.  */
>   
> +typedef struct value *(*fn_prev_register) (frame_info_ptr this_frame,
> +					   void **this_cache, int regnum);
> +
>   struct dwarf2_frame_state_reg
>   {
>     /* Each register save state can be described in terms of a CFA slot,
> @@ -78,8 +81,7 @@ struct dwarf2_frame_state_reg
>         const gdb_byte *start;
>         ULONGEST len;
>       } exp;
> -    struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> -			 int regnum);
> +    fn_prev_register fn;
>     } loc;
>     enum dwarf2_frame_reg_rule how;
>   };
> @@ -262,4 +264,18 @@ extern int dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>   				  const gdb_byte **cfa_start_out,
>   				  const gdb_byte **cfa_end_out);
>   
> +
> +/* 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);
> +
>   #endif /* dwarf2-frame.h */

As we've discussed before, I think the cache idea is nice if we have to deal with targets with multiple CFA's (in our case, we have either 4 SP's or 2 SP's, plus aliases).

DWARF doesn't seem to support this at the moment, and the function HOW for DWARF is not smart enough to remember a previously-fetched value. So it seems we have room
for some improvement, unless there is enough reason elsewhere about why we shouldn't have a cache.

It would be nice to have some opinions from others, so we can potentially shape this in a way that makes it useful for the general case.