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

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

On 2022-12-20 22:02, 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.
> 
> 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.

Right now; I don't know, but as the fn member in the 
dwarf2_frame_state_reg struct contains one function pointer per 
register, the architecture allows more than one function pointer per frame.
If we went with a simpler solution, to only have one data block per 
frame, regardless of what function that is "owning" the data, then it 
could lead to nasty surprises if there is some unwinder that expects to 
be able to use more than data type...
If we move the function pointer from the register scope to the frame 
scope, then I agree that only one data object would be needed.
If we stick to having the function pointer per register, I could accept 
to have one data block, but somewhere, an assert should added so that 
the wrongful assumption mentioned above would be caught early rather 
than leading to strange bugs. This would mean that the type needs to be 
stored in the dwarf2_frame_cache struct somehow, but the type is 
currently internal to another compile unit.
This is basically the reason why I went with the decoupled solution that 
is provided in this patch.

> 
>> +struct dwarf2_frame_fn_data
>> +{
> 
> New type should have a comment.

Okay, I'll add comments, but I would like to know if this is the way to 
go or if there should be some alternative implementation before spending 
more time on this.

> 
>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>> +		       int regnum);
> 
> Shouldn't this use the fn_prev_register typedef?

Indeed.

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

Don't know why the contrib/check_GNU_style.py in the GCC source tree did 
not flag this. Anyway, will be fixed in v3.

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

Can you point me to an example and I will do something similar for these 
new functions if we decide to go this way.

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

What more detail would you like to include?

> 
> Tom

Kind regards,
Torbjörn