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

Message ID 20230119102948.3069226-2-torbjorn.svensson@foss.st.com
State New
Headers
Series [v3,1/2] gdb: dwarf2 generic implementation for caching function data |

Commit Message

Torbjörn SVENSSON Jan. 19, 2023, 10:29 a.m. UTC
  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(-)
  

Comments

Luis Machado Jan. 25, 2023, 4:55 p.m. UTC | #1
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?

> +				       arm_dwarf2_prev_register, size);
> +
> +      if (tdep->have_sec_ext)
> +	{
> +	  cache->sp
> +	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +
> +	  cache->msp_s
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_s_regnum);
> +	  cache->msp_ns
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_ns_regnum);
> +	  cache->psp_s
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_s_regnum);
> +	  cache->psp_ns
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_ns_regnum);
> +	}
> +      else if (tdep->is_m)
> +	{
> +	  cache->sp
> +	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +
> +	  cache->msp
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_msp_regnum);
> +	  cache->psp
> +	    = get_frame_register_unsigned (this_frame,
> +					   tdep->m_profile_psp_regnum);
> +	}
> +    }
>   
>     if (regnum == ARM_PC_REGNUM)
>       {
> @@ -4011,33 +4065,18 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>   
>         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);
> +	    && (cache->msp_s == cache->sp || cache->msp_ns == cache->sp);
>   	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
> -	    && (msp_s == sp);
> +	    && (cache->msp_s == cache->sp);
>   	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
> -	    && (msp_ns == sp);
> +	    && (cache->msp_ns == cache->sp);
>   	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
> -	    && (psp_s == sp || psp_ns == sp);
> +	    && (cache->psp_s == cache->sp || cache->psp_ns == cache->sp);
>   	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
> -	    && (psp_s == sp);
> +	    && (cache->psp_s == cache->sp);
>   	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
> -	    && (psp_ns == sp);
> +	    && (cache->psp_ns == cache->sp);
>   
>   	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
>   	    || is_psp || is_psp_s || is_psp_ns;
> @@ -4045,17 +4084,10 @@ arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>   	}
>         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);
> +	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
> +	    && (cache->sp == cache->msp);
> +	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
> +	    && (cache->sp == cache->psp);
>   
>   	  override_with_sp_value = is_msp || is_psp;
>   	}

Otherwise LGTM for the arm-specific parts.
  
Simon Marchi Jan. 25, 2023, 5:12 p.m. UTC | #2
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
  
Torbjörn SVENSSON Jan. 25, 2023, 8:15 p.m. UTC | #3
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
  

Patch

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,
+				       arm_dwarf2_prev_register, size);
+
+      if (tdep->have_sec_ext)
+	{
+	  cache->sp
+	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+	  cache->msp_s
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_s_regnum);
+	  cache->msp_ns
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_ns_regnum);
+	  cache->psp_s
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_s_regnum);
+	  cache->psp_ns
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_ns_regnum);
+	}
+      else if (tdep->is_m)
+	{
+	  cache->sp
+	    = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
+
+	  cache->msp
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_msp_regnum);
+	  cache->psp
+	    = get_frame_register_unsigned (this_frame,
+					   tdep->m_profile_psp_regnum);
+	}
+    }
 
   if (regnum == ARM_PC_REGNUM)
     {
@@ -4011,33 +4065,18 @@  arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
 
       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);
+	    && (cache->msp_s == cache->sp || cache->msp_ns == cache->sp);
 	  bool is_msp_s = (regnum == tdep->m_profile_msp_s_regnum)
-	    && (msp_s == sp);
+	    && (cache->msp_s == cache->sp);
 	  bool is_msp_ns = (regnum == tdep->m_profile_msp_ns_regnum)
-	    && (msp_ns == sp);
+	    && (cache->msp_ns == cache->sp);
 	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
-	    && (psp_s == sp || psp_ns == sp);
+	    && (cache->psp_s == cache->sp || cache->psp_ns == cache->sp);
 	  bool is_psp_s = (regnum == tdep->m_profile_psp_s_regnum)
-	    && (psp_s == sp);
+	    && (cache->psp_s == cache->sp);
 	  bool is_psp_ns = (regnum == tdep->m_profile_psp_ns_regnum)
-	    && (psp_ns == sp);
+	    && (cache->psp_ns == cache->sp);
 
 	  override_with_sp_value = is_msp || is_msp_s || is_msp_ns
 	    || is_psp || is_psp_s || is_psp_ns;
@@ -4045,17 +4084,10 @@  arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
 	}
       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);
+	  bool is_msp = (regnum == tdep->m_profile_msp_regnum)
+	    && (cache->sp == cache->msp);
+	  bool is_psp = (regnum == tdep->m_profile_psp_regnum)
+	    && (cache->sp == cache->psp);
 
 	  override_with_sp_value = is_msp || is_psp;
 	}