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

Torbjorn SVENSSON via Gdb-patches
2023-01-05T20:53:32Z

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