Re: [PATCH 3/3] gdb/arm: PR 29738 Cache value for stack pointers for dwarf2 frames

[Tomas Vanek <vanekt@volny.cz>]

On 11/11/2022 10:15, Luis Machado wrote:
> Hi Torbjörn,
>
> On 11/4/22 14:44, Torbjörn SVENSSON wrote:
>> Without this patch, the number of calls to arm_dwarf2_prev_register
>> would grow in a too rapid way when the number of frames increase.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> ---
>>   gdb/arm-tdep.c | 141 +++++++++++++++++++++++--------------------------
>>   1 file changed, 66 insertions(+), 75 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index c011b2aa973..a6fb660bcbc 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3992,78 +3992,6 @@ arm_dwarf2_prev_register (frame_info_ptr 
>> this_frame, void **this_cache,
>>         cpsr = reconstruct_t_bit (gdbarch, lr, cpsr);
>>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>>       }
>> -  else if (arm_is_alternative_sp_register (tdep, regnum))
>> -    {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      bool override_with_sp_value = false;
>> -      CORE_ADDR val;
>> -
>> -      if (tdep->have_sec_ext)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_s_regnum);
>> -      CORE_ADDR msp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_ns_regnum);
>> -      CORE_ADDR psp_s
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_s_regnum);
>> -      CORE_ADDR psp_ns
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_ns_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> -        && (msp_s == sp || msp_ns == sp);
>> -      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> -        && (msp_s == sp);
>> -      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> -        && (msp_ns == sp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> -        && (psp_s == sp || psp_ns == sp);
>> -      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> -        && (psp_s == sp);
>> -      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> -        && (psp_ns == sp);
>> -
>> -      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> -        || is_psp || is_psp_s || is_psp_ns;
>> -
>> -    }
>> -      else if (tdep->is_m)
>> -    {
>> -      CORE_ADDR sp
>> -        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> -      CORE_ADDR msp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_msp_regnum);
>> -      CORE_ADDR psp
>> -        = get_frame_register_unsigned (this_frame,
>> -                       tdep->m_profile_psp_regnum);
>> -
>> -      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> -      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> -
>> -      override_with_sp_value = is_msp || is_psp;
>> -    }
>> -
>> -      if (override_with_sp_value)
>> -    {
>> -      /* Use value of SP from previous frame.  */
>> -      frame_info_ptr prev_frame = get_prev_frame (this_frame);
>> -      if (prev_frame)
>> -        val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
>> -      else
>> -        val = get_frame_base (this_frame);
>> -    }
>> -      else
>> -    /* Use value for the register from previous frame.  */
>> -    val = get_frame_register_unsigned (this_frame, regnum);
>> -
>> -      return frame_unwind_got_constant (this_frame, regnum, val);
>> -    }
>>       internal_error (_("Unexpected register %d"), regnum);
>>   }
>> @@ -5202,9 +5130,72 @@ arm_dwarf2_frame_init_reg (struct gdbarch 
>> *gdbarch, int regnum,
>>       reg->how = DWARF2_FRAME_REG_CFA;
>>     else if (arm_is_alternative_sp_register (tdep, regnum))
>>       {
>> -      /* Handle the alternative SP registers on Cortex-M.  */
>> -      reg->how = DWARF2_FRAME_REG_FN;
>> -      reg->loc.fn = arm_dwarf2_prev_register;
>> +      /* Identify what stack pointers that are synced with sp. */
>> +      bool override_with_sp_value = false;
>> +
>> +      if (tdep->have_sec_ext)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_s_regnum);
>> +      CORE_ADDR msp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_ns_regnum);
>> +      CORE_ADDR psp_s
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_s_regnum);
>> +      CORE_ADDR psp_ns
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_ns_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum)
>> +        && (msp_s == sp || msp_ns == sp);
>> +      bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
>> +        && (msp_s == sp);
>> +      bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
>> +        && (msp_ns == sp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum)
>> +        && (psp_s == sp || psp_ns == sp);
>> +      bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
>> +        && (psp_s == sp);
>> +      bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
>> +        && (psp_ns == sp);
>> +
>> +      override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>> +        || is_psp || is_psp_s || is_psp_ns;
>> +
>> +    }
>> +      else if (tdep->is_m)
>> +    {
>> +      CORE_ADDR sp
>> +        = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>> +
>> +      CORE_ADDR msp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_msp_regnum);
>> +      CORE_ADDR psp
>> +        = get_frame_register_unsigned (this_frame,
>> +                       tdep->m_profile_psp_regnum);
>> +
>> +      bool is_msp = (regnum == tdep->m_profile_msp_regnum) && (sp == 
>> msp);
>> +      bool is_psp = (regnum == tdep->m_profile_psp_regnum) && (sp == 
>> psp);
>> +
>> +      override_with_sp_value = is_msp || is_psp;
>> +    }
>> +
>> +      if (override_with_sp_value)
>> +    {
>> +      /* Use the CFA value for this stack pointer register.  */
>> +      reg->how = DWARF2_FRAME_REG_CFA;
>> +    }
>> +      else
>> +    {
>> +      /* This frame does not have any update for this stack 
>> pointer.  */
>> +      reg->how = DWARF2_FRAME_REG_SAME_VALUE;
>> +    }
>>       }
>>   }
>
> Although it does make sense to cache the values to address this issue, 
> I don't think this approach is appropriate.
>
> The init_reg () hook is supposed to initialize the rules. Even if 
> unwinding works from this particular function, I don't think it is 
> safe to do so.
>
> If we attempt to point at a register whose rule hasn't been initialize 
> yet, we may run into issues. I'm not sure if that is technically 
> possible, I haven't exercised this in practice.
>
> I think we need to take a step back and re-assess how we're 
> determining the active sp register. From what I can tell, it is just a 
> value check. Since we need to check at least 6 values, it gets fairly 
> expensive the more frames you have in the debugging session.
>
> One idea that comes to mind is to introduce a pseudo-register that 
> stores the active sp register number (say active_sp). It may not need 
> a name if it is only going to be used for unwinding purposes.
>
> In the sentinel frame, we can easily determine this value by doing 
> quick checks with all the alternate sp registers. The other frames 
> would query this pseudo-register to know what value they must use for 
> the alternate sp registers.
>
> I think this will reduce the number of alternate sp value fetches 
> considerably, and thus will address this performance issue.
>
> For the alternate sp registers, we can still use the 
> arm_dwarf2_prev_register function, but now we'll fetch the active_sp 
> pseudo_register to help us decide what value to return.
>
> On other unwinders of different frame types (prologue, exception etc), 
> we can adjust the value of active_sp accordingly if the active stack 
> pointer changed.
>
> Does that make sense?

It makes a lot of sense to me.

Although I doubt if adding 'active_sp' is enough to prevent the rapid 
growth of the number of fetches.
To resolve prev SP value the unwinder fetches 'active_sp' and the 
specific xsp_x pointed by active_sp.
The prev frame needs active_sp again to find if the specific xsp_x is 
updated to CFA and if not it fetches xsp_x.
And if we consider the evaluation of active_sp may be as expensive as 
looking up secure gateway veneer name
and checking if prefixed by '__acle_se_' as used in
https://sourceware.org/pipermail/gdb-patches/2022-November/193433.html
I would incline to use better caching in the dwarf2 frames regardless of 
using 'active_sp'.

My RFC patch series is sub-optimal from the performance point of view
and having handy 'active_sp' pseudo register available could make the 
code simpler (and hopefully faster).
The bad news for me is that the change series would need almost a 
complete rework (for the second time).

Please keep me informed.

Tomas