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

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

Ping! :)

Can someone (Simon?, Andrew?, Pedro?, Joel?) take a look at the proposed 
patch?
I would like to conclude something for the GDB13 release...

Kind regards,
Torbjörn


On 2022-11-21 22:16, Luis Machado wrote:
> 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.