[PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Torbjörn SVENSSON]

v2 -> v3:

Addressed comments from Tom in
https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html


---

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.

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).

dwarf2_frame_get_fn_data can be used to retrieve the function unique
cache object.
dwarf2_frame_allocate_fn_data can be used to allocate and retrieve the
function unique cache object.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
---
 gdb/dwarf2/frame.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/dwarf2/frame.h | 37 ++++++++++++++++++++++++++--
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index bd36a6c7fb4..a81b2500cb2 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -831,6 +831,22 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
 }
 
 \f
+/* Custom function data object for architecture specific prev_register
+   implementation.  Main purpose of this object is to allow caching of
+   expensive data lookups in the prev_register handling.  */
+
+struct dwarf2_frame_fn_data
+{
+  /* The cookie to identify the custom function data by.  */
+  fn_prev_register cookie;
+
+  /* The custom function data.  */
+  void *data;
+
+  /* Pointer to the next custom function data object for this frame.  */
+  struct dwarf2_frame_fn_data *next;
+};
+
 struct dwarf2_frame_cache
 {
   /* DWARF Call Frame Address.  */
@@ -862,6 +878,8 @@ struct dwarf2_frame_cache
      dwarf2_tailcall_frame_unwind unwinder so this field does not apply for
      them.  */
   void *tailcall_cache;
+
+  struct dwarf2_frame_fn_data *fn_data;
 };
 
 static struct dwarf2_frame_cache *
@@ -1221,6 +1239,49 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
     }
 }
 
+/* See frame.h.  */
+
+void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
+				fn_prev_register cookie)
+{
+  struct dwarf2_frame_fn_data *fn_data = nullptr;
+  struct dwarf2_frame_cache *cache
+    = dwarf2_frame_cache (this_frame, this_cache);
+
+  /* Find the object for the function.  */
+  for (fn_data = cache->fn_data; fn_data; fn_data = fn_data->next)
+    if (fn_data->cookie == cookie)
+      return fn_data->data;
+
+  return nullptr;
+}
+
+/* See frame.h.  */
+
+void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
+				     void **this_cache,
+				     fn_prev_register cookie,
+				     unsigned long size)
+{
+  struct dwarf2_frame_fn_data *fn_data = nullptr;
+  struct dwarf2_frame_cache *cache
+    = dwarf2_frame_cache (this_frame, this_cache);
+
+  /* First try to find an existing object.  */
+  void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, cookie);
+  if (data)
+    return data;
+
+  /* No object found, lets create a new instance.  */
+  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
+  fn_data->cookie = cookie;
+  fn_data->data = frame_obstack_zalloc (size);
+  fn_data->next = cache->fn_data;
+  cache->fn_data = fn_data;
+
+  return fn_data->data;
+}
+
 /* Proxy for tailcall_frame_dealloc_cache for bottom frame of a virtual tail
    call frames chain.  */
 
diff --git a/gdb/dwarf2/frame.h b/gdb/dwarf2/frame.h
index 4444c153741..5643e557513 100644
--- a/gdb/dwarf2/frame.h
+++ b/gdb/dwarf2/frame.h
@@ -66,6 +66,9 @@ enum dwarf2_frame_reg_rule
 
 /* Register state.  */
 
+typedef struct value *(*fn_prev_register) (frame_info_ptr this_frame,
+					   void **this_cache, int regnum);
+
 struct dwarf2_frame_state_reg
 {
   /* Each register save state can be described in terms of a CFA slot,
@@ -78,8 +81,7 @@ struct dwarf2_frame_state_reg
       const gdb_byte *start;
       ULONGEST len;
     } exp;
-    struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
-			 int regnum);
+    fn_prev_register fn;
   } loc;
   enum dwarf2_frame_reg_rule how;
 };
@@ -262,4 +264,35 @@ extern int dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
 				  const gdb_byte **cfa_start_out,
 				  const gdb_byte **cfa_end_out);
 
+
+/* Allocate a new instance of the function unique data.
+
+   The main purpose of this custom function data object is to allow caching the
+   value of expensive lookups in the prev_register implementation.
+
+   THIS_FRAME is the frame that the custom data object should be associated
+   with.
+   THIS_CACHE is the dwarf2 cache object to store the pointer on.
+   COOKIE is the key for the prev_function implementation.
+   SIZE is the size of the custom data object to allocate.  */
+
+extern void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
+					    void **this_cache,
+					    fn_prev_register cookie,
+					    unsigned long size);
+
+/* Retrieve the function unique data for this frame or NULL if none exists.
+
+   The main purpose of this custom function data object is to allow caching the
+   value of expensive lookups in the prev_register implementation.
+
+   THIS_FRAME is the frame that the custom data object should be associated
+   with.
+   THIS_CACHE is the dwarf2 cache object to store the pointer on.
+   COOKIE is the key for the prev_function implementation.  */
+
+extern void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame,
+				       void **this_cache,
+				       fn_prev_register cookie);
+
 #endif /* dwarf2-frame.h */
-- 
2.25.1

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

[Torbjörn SVENSSON]

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,
+				       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;
 	}
-- 
2.25.1

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Simon Marchi]

On 1/19/23 05:29, Torbjörn SVENSSON via Gdb-patches wrote:
> v2 -> v3:
> 
> Addressed comments from Tom in
> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html
> 
> 
> ---
> 
> 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.
> 
> 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).
> 
> dwarf2_frame_get_fn_data can be used to retrieve the function unique
> cache object.
> dwarf2_frame_allocate_fn_data can be used to allocate and retrieve the
> function unique cache object.
> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>

Hi,

My main question is: this approach requires each custom prev_register
function to set up and manage its own cache.  Did you consider caching
values at a higher level?

Perhaps:

 1. In dwarf2_frame_prev_register, just for DWARF2_FRAME_REG_FN
 2. In dwarf2_frame_prev_register, but for all kinds of registers
 3. In frame_unwind_register_value, which would cache all register values
    for all frames regardless of the unwinder

I don't know if other ways of unwinding would benefit that much from
caching, but I guess it wouldn't hurt.  For instance, evaluating DWARF
expressions is probably generally not super expensive, but it might
still benefit from getting cached.  And it would be nice to write the
caching code just once and have it work transparently for everything.

> @@ -262,4 +264,35 @@ extern int dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>  				  const gdb_byte **cfa_start_out,
>  				  const gdb_byte **cfa_end_out);
>  
> +
> +/* Allocate a new instance of the function unique data.
> +
> +   The main purpose of this custom function data object is to allow caching the
> +   value of expensive lookups in the prev_register implementation.

I would replace "in the prev_register implementation" with "in
prev_register implementations".  There isn't only one prev_register
implementation AFAIK.

Simon

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Torbjorn SVENSSON]

On 2023-01-19 17:53, Simon Marchi wrote:
> 
> 
> On 1/19/23 05:29, Torbjörn SVENSSON via Gdb-patches wrote:
>> v2 -> v3:
>>
>> Addressed comments from Tom in
>> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html
>>
>>
>> ---
>>
>> 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.
>>
>> 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).
>>
>> dwarf2_frame_get_fn_data can be used to retrieve the function unique
>> cache object.
>> dwarf2_frame_allocate_fn_data can be used to allocate and retrieve the
>> function unique cache object.
>>
>> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
>> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> 
> Hi,
> 
> My main question is: this approach requires each custom prev_register
> function to set up and manage its own cache.  Did you consider caching
> values at a higher level?
> 
> Perhaps:
> 
>   1. In dwarf2_frame_prev_register, just for DWARF2_FRAME_REG_FN
>   2. In dwarf2_frame_prev_register, but for all kinds of registers
>   3. In frame_unwind_register_value, which would cache all register values
>      for all frames regardless of the unwinder
> 
> I don't know if other ways of unwinding would benefit that much from
> caching, but I guess it wouldn't hurt.  For instance, evaluating DWARF
> expressions is probably generally not super expensive, but it might
> still benefit from getting cached.  And it would be nice to write the
> caching code just once and have it work transparently for everything.

I suppose a generic cache might be useful in the long run, but it would 
have an impact on other code paths and I'm not comfortable of doing this 
major change now. Doing this change would imply that almost everything 
related to unwinding is impacted in one way or another and I suppose we 
would need to test that. With the solution I proposed, only Arm Cortex 
would be impacted and the scope for testing would be rather small.

I'm also aiming to resolve this for GDB13, so doing this major change is 
a bit late in the sprint, right?

If you have a better idea on how to cache everything, like in your 3 
ideas above, please don't hesitate do share the implementation. :)

> 
>> @@ -262,4 +264,35 @@ extern int dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>>   				  const gdb_byte **cfa_start_out,
>>   				  const gdb_byte **cfa_end_out);
>>   
>> +
>> +/* Allocate a new instance of the function unique data.
>> +
>> +   The main purpose of this custom function data object is to allow caching the
>> +   value of expensive lookups in the prev_register implementation.
> 
> I would replace "in the prev_register implementation" with "in
> prev_register implementations".  There isn't only one prev_register
> implementation AFAIK.

Will do that for v4 or when merging if no v4 is needed.

> 
> Simon


Kind regards,
Torbjörn

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Simon Marchi]

> I suppose a generic cache might be useful in the long run, but it
> would have an impact on other code paths and I'm not comfortable of
> doing this major change now. Doing this change would imply that almost
> everything related to unwinding is impacted in one way or another and
> I suppose we would need to test that. With the solution I proposed,
> only Arm Cortex would be impacted and the scope for testing would be
> rather small.

I understand the feeling.  It's impossible to test every change on every
configuration.  We do our best to understand the code and how the change
will impact different configurations, but we also have to accept a
certain level of risk that we will break something, otherwise we will
never change / improve anything.  More eyes on the code, more people
testing regularly with different configurations and more configurations
on the Buildbot can help mitigate that.

> I'm also aiming to resolve this for GDB13, so doing this major change
> is a bit late in the sprint, right?

Indeed.  Then I think it's fine to introduce what you have, and then we
could replace it with a more general solution later.

> If you have a better idea on how to cache everything, like in your 3
> ideas above, please don't hesitate do share the implementation. :)

I imagine something simple.  A hash table in frame_info or somewhere
else, where frame_unwind_register_value would put values returned by
`next_frame->unwind->prev_register`, which would be returned directly on
subsequent calls.  This makes the assumption that a given frame info
object has the same unwinder throughout its lifetime, and that it will
return the same register value throughout its lifetime.  I don't know of
any situation where that is not true.

To keep the allocation / deallocation cost down, the hash tables could
be allocated on the frame obstack.

Simon

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Luis Machado]

On 1/20/23 17:28, Simon Marchi wrote:
> 
>> I suppose a generic cache might be useful in the long run, but it
>> would have an impact on other code paths and I'm not comfortable of
>> doing this major change now. Doing this change would imply that almost
>> everything related to unwinding is impacted in one way or another and
>> I suppose we would need to test that. With the solution I proposed,
>> only Arm Cortex would be impacted and the scope for testing would be
>> rather small.
> 
> I understand the feeling.  It's impossible to test every change on every
> configuration.  We do our best to understand the code and how the change
> will impact different configurations, but we also have to accept a
> certain level of risk that we will break something, otherwise we will
> never change / improve anything.  More eyes on the code, more people
> testing regularly with different configurations and more configurations
> on the Buildbot can help mitigate that.
> 
>> I'm also aiming to resolve this for GDB13, so doing this major change
>> is a bit late in the sprint, right?
> 
> Indeed.  Then I think it's fine to introduce what you have, and then we
> could replace it with a more general solution later.
> 
>> If you have a better idea on how to cache everything, like in your 3
>> ideas above, please don't hesitate do share the implementation. :)
> 
> I imagine something simple.  A hash table in frame_info or somewhere
> else, where frame_unwind_register_value would put values returned by
> `next_frame->unwind->prev_register`, which would be returned directly on
> subsequent calls.  This makes the assumption that a given frame info
> object has the same unwinder throughout its lifetime, and that it will
> return the same register value throughout its lifetime.  I don't know of
> any situation where that is not true.
> 
> To keep the allocation / deallocation cost down, the hash tables could
> be allocated on the frame obstack.

Maybe not a solution for now, but can't we use something like the trad-frame structs to cache
values/locations of registers for a given frame?

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Simon Marchi]

> Maybe not a solution for now, but can't we use something like the trad-frame structs to cache
> values/locations of registers for a given frame?

It's true that it's very similar, but I don't immediately see how I
would use it in this case.  The trad-frame stuff saves the info in its
own format, whereas here it would be nice to cache struct values
directly.  And it wouldn't be optimal for trad-frame to cache values,
because we would end up allocating values that might not be needed in
the end.

However, I see that trad-frame uses an array of size
gdbarch_num_cooked_regs.  It would make sense to use that here too,
instead of a hash table, it makes the lookups O(1).

Simon

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Tom Tromey]

>>>>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:

> v2 -> v3:
> Addressed comments from Tom in
> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html

Sorry, I'm afraid you missed a few.

> +struct dwarf2_frame_fn_data
> +{
> +  /* The cookie to identify the custom function data by.  */
> +  fn_prev_register cookie;

The previous review mentioned changing the type of this, but honestly I
don't really care about that one, this is as good as anything now that
it's documented.

> +/* See frame.h.  */
> +
> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register cookie)

gdb style puts the function name at the start of the line.
There are many examples in the source.

> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +				     void **this_cache,
> +				     fn_prev_register cookie,
> +				     unsigned long size)

Here too.

> +{
> +  struct dwarf2_frame_fn_data *fn_data = nullptr;
> +  struct dwarf2_frame_cache *cache
> +    = dwarf2_frame_cache (this_frame, this_cache);
> +
> +  /* First try to find an existing object.  */
> +  void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, cookie);
> +  if (data)
> +    return data;

It seems to me that there is no need to do this check.
IMO it's fine to just assert that it isn't found.

thanks,
Tom

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Torbjorn SVENSSON]

On 2023-01-20 18:43, Simon Marchi wrote:
>> Maybe not a solution for now, but can't we use something like the trad-frame structs to cache
>> values/locations of registers for a given frame?
> 
> It's true that it's very similar, but I don't immediately see how I
> would use it in this case.  The trad-frame stuff saves the info in its
> own format, whereas here it would be nice to cache struct values
> directly.  And it wouldn't be optimal for trad-frame to cache values,
> because we would end up allocating values that might not be needed in
> the end.
> 
> However, I see that trad-frame uses an array of size
> gdbarch_num_cooked_regs.  It would make sense to use that here too,
> instead of a hash table, it makes the lookups O(1).
> 
> Simon

I think this is something that can be looked at later (GDB14?).

My first impression is that an array of pointers to struct value might 
work. If the pointer is nullptr, then there is no cached value, 
otherwise the pointer could be returned directly. But lest think of this 
after GDB13.

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Torbjorn SVENSSON]

On 2023-01-20 20:59, Tom Tromey wrote:
>>>>>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> v2 -> v3:
>> Addressed comments from Tom in
>> https://sourceware.org/pipermail/gdb-patches/2023-January/195882.html
> 
> Sorry, I'm afraid you missed a few.
> 
>> +struct dwarf2_frame_fn_data
>> +{
>> +  /* The cookie to identify the custom function data by.  */
>> +  fn_prev_register cookie;
> 
> The previous review mentioned changing the type of this, but honestly I
> don't really care about that one, this is as good as anything now that
> it's documented.
> 
>> +/* See frame.h.  */
>> +
>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
>> +				fn_prev_register cookie)
> 
> gdb style puts the function name at the start of the line.
> There are many examples in the source.
> 
>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>> +				     void **this_cache,
>> +				     fn_prev_register cookie,
>> +				     unsigned long size)
> 
> Here too.
> 
>> +{
>> +  struct dwarf2_frame_fn_data *fn_data = nullptr;
>> +  struct dwarf2_frame_cache *cache
>> +    = dwarf2_frame_cache (this_frame, this_cache);
>> +
>> +  /* First try to find an existing object.  */
>> +  void *data = dwarf2_frame_get_fn_data (this_frame, this_cache, cookie);
>> +  if (data)
>> +    return data;
> 
> It seems to me that there is no need to do this check.
> IMO it's fine to just assert that it isn't found.
> 
> thanks,
> Tom

Thanks for the review.

I've corrected the points noted above. Still not sure why the 
check_GNU_style.py script in GCC contrib does not complain about this..

Anyway, is it okay for trunk with these changes or should I send a v4?

Kind regards,
Torbjörn

Re: [PATCH v3 1/2] gdb: dwarf2 generic implementation for caching function data

[Tom Tromey]

>>>>> "Torbjorn" == Torbjorn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:

Torbjorn> Anyway, is it okay for trunk with these changes or should I send a v4?

Yeah, it's ok.  Thank you.

Tom

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

[Luis Machado]

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.

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

[Simon Marchi]

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

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

[Torbjorn SVENSSON]

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