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

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

On 2023-01-18 19:47, Tom Tromey wrote:
>>>>>> 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.

Fixed in v3.

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

Fixed in v3.

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

I think the format is correct, but it looks strange in the mail as the 
'+' sign is "eaten" by the first tab. Looking in my editor for the 
source code, it looks correct.

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

Yes, it's the cookie/key to identify the right object for the 
prev_register function. In v3, I've renamed the variable (and the 
member) to be called "cookie" instead if that's better. The "fn" name 
came from the struct where it was originally defined.
The cookie/key could be a string, but then it should be auto-generated 
when calling the functions rather than letting the user type it. The 
reason is that if the user types it, it's less clear what the 
consequences of reusing it will be. Also, if we go for a string, it 
would consume more memory than having a function pointer (not that 
memory should be an issue, but anyway...).
If there is a function pointer, doesn't GDB lookup the symbol for the 
function pointer if there are debug symbols in the application?

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

What happens if there are 2 prev_register functions that both wants some 
custom data? With the approach I have (looping over the list and 
returning the matching data for the cookie/key) would allow us to reuse 
the existing object rather than create a new one. I could change the 
block that checks if there was a match and returns it to an assert, but 
what would the benefit be? Instead of reusing the existing object, we 
would crash GDB. Is this better?
> 
>> +
>> +/* 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.

Improved in v3.

> 
> thanks,
> Tom

Kind regards,
Torbjörn + Yvan