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

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

Hi,

Any comments on my last reply?

Kind regards,
Torbjörn

On 2022-12-28 17:16, Torbjorn SVENSSON wrote:
> 
> 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