Re: [PATCH v3 2/2] gdb/arm: Use new dwarf2 function cache

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

On 2023-01-25 18:12, Simon Marchi wrote:
> 
> 
> On 1/25/23 11:55, Luis Machado via Gdb-patches wrote:
>> On 1/19/23 10:29, Torbjörn SVENSSON wrote:
>>> v2 -> v3:
>>> No changes, just rebase.
>>>
>>> ---
>>>
>>> This patch resolves the performance issue reported in pr/29738 by
>>> caching the values for the stack pointers for the inner frame.  By
>>> doing so, the impact can be reduced to checking the state and
>>> returning the appropriate value.
>>>
>>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>>> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
>>> ---
>>>    gdb/arm-tdep.c | 96 +++++++++++++++++++++++++++++++++-----------------
>>>    1 file changed, 64 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>>> index 51ec5236af1..be7219ca66e 100644
>>> --- a/gdb/arm-tdep.c
>>> +++ b/gdb/arm-tdep.c
>>> @@ -3964,6 +3964,18 @@ struct frame_base arm_normal_base = {
>>>      arm_normal_frame_base
>>>    };
>>>    +struct arm_dwarf2_prev_register_cache
>>> +{
>>> +  /* Cached value of the coresponding stack pointer for the inner frame.  */
>>> +  CORE_ADDR sp;
>>> +  CORE_ADDR msp;
>>> +  CORE_ADDR msp_s;
>>> +  CORE_ADDR msp_ns;
>>> +  CORE_ADDR psp;
>>> +  CORE_ADDR psp_s;
>>> +  CORE_ADDR psp_ns;
>>> +};
>>> +
>>>    static struct value *
>>>    arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>>>                  int regnum)
>>> @@ -3972,6 +3984,48 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>>>      arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
>>>      CORE_ADDR lr;
>>>      ULONGEST cpsr;
>>> +  struct arm_dwarf2_prev_register_cache *cache
>>> +    = (struct arm_dwarf2_prev_register_cache *) dwarf2_frame_get_fn_data (
>>> +      this_frame, this_cache, arm_dwarf2_prev_register);
>>> +
>>> +  if (!cache)
>>> +    {
>>> +      const unsigned int size = sizeof (struct arm_dwarf2_prev_register_cache);
>>> +      cache = (struct arm_dwarf2_prev_register_cache *)
>>> +    dwarf2_frame_allocate_fn_data (this_frame, this_cache,
>>
>> Still some funny spacing above. Could you please check the patch again, just to be sure?
> 
> To clarify, I would probably write the first part as:
> 
>    arm_dwarf2_prev_register_cache *cache
>      = ((arm_dwarf2_prev_register_cache *)
>         dwarf2_frame_get_fn_data (this_frame, this_cache,
> 				 arm_dwarf2_prev_register));
> 
> 
> The changes are:
> 
>   - dropping the struct keyword (unnecessary in C++)
>   - the assigment operator is on the second line (mandated by GNU style)
>   - the addition of parenthesis when breaking an expression (also
>     mandated by GNU style, is it so that the following line(s) align
>     nicely with the opening parenthesis)
> 
> Similarly, the second one would be:
> 
>        cache = ((arm_dwarf2_prev_register_cache *)
> 	       dwarf2_frame_allocate_fn_data (this_frame, this_cache,
> 					      arm_dwarf2_prev_register, size);
> 
> This is just how I would do it, there are probably other valid ways.
> 
> Simon


I pushed the patchset with the above changes to trunk.

Thanks for the review of these!

Kind regards,
Torbjörn