[PATCH 0/4] v2 gdb/arm: Fixes for Cortex-M stack unwinding

[PATCH 0/4] v2 gdb/arm: Fixes for Cortex-M stack unwinding

[Torbjörn SVENSSON]

Hi,

This patchset attempts to attack the issue reported in pr/29738.

Based on my testing on a STM32L552 with TrustZone, it appears to work as
expected, but I'd love to have another set of eyes on this!

Most of the items reported can be seen using the GTZC example from ST:
https://github.com/STMicroelectronics/STM32CubeL5/tree/master/Projects/NUCLEO-L552ZE-Q/Examples/GTZC/GTZC_TZSC_MPCBB_TrustZone

I've manually verfied the register content by setting break points in
* main
* Error_Handler
* SecureFault_Handler


For the dwarf2 problem in pr/29738, I added 7-chained functions like
below and placed a breakpoint at myfunc7:

  void myfunc7()
    {
      HAL_Delay(1);
    }
  ...
  void myfunc1()
    {
      HAL_Delay(1);
      myfunc2();
    }

For each breakpoint, I then used the below macros to print the
registers. For each of the printed frames, I then manually checked if
the values maked sense.

define show_reg
  shell printf "%-8s " '$arg0'
  p/x $arg0
end
define show_stacks
  shell echo
  shell echo -e "\x1b[35mContent at $arg0\x1b[0m"
  bt
  set $i = 0
  while $i <= $arg1
    f $i
    show_reg $lr
    show_reg $pc
    show_reg $sp
    show_reg $msp
    show_reg $msp_s
    show_reg $msp_ns
    show_reg $psp
    show_reg $psp_s
    show_reg $psp_ns

    shell echo
    set $i = $i + 1
  end
  shell echo
  shell echo
end


The show_stacks macro takes 2 arguments, a description and the number of
frames to print registers for.
To show the registers for all the frames when at the Error_Handler
breakpoint, I used:
show_stacks "Error_Handler" 7


After talkning to Luis about the v1 series of these patches, we
concluded that a generic way to store the data needed by the
prev_register function was better than trying to squeze the logic
into the _init function.


Kind regards,
Torbjörn

[PATCH v2 1/4] gdb/arm: Update active msp/psp when switching stack

[Torbjörn SVENSSON]

For targets with secext, msp and psp can be seen as an alias for one
of msp_s, msp_ns, psp_s or psp_ns. When switching active sp, the
coresponding msp/psp needs to be switched too.

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
 gdb/arm-tdep.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7cb3f5f3050..124a94dc87d 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -504,8 +504,23 @@ arm_cache_switch_prev_sp (struct arm_prologue_cache *cache,
   gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum));
 
   if (tdep->have_sec_ext)
-    gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
-		&& sp_regnum != tdep->m_profile_psp_regnum);
+    {
+      gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
+		  && sp_regnum != tdep->m_profile_psp_regnum);
+
+      if (sp_regnum == tdep->m_profile_msp_s_regnum
+	  || sp_regnum == tdep->m_profile_psp_s_regnum)
+	{
+	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
+	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
+	}
+      else if (sp_regnum == tdep->m_profile_msp_ns_regnum
+	       || sp_regnum == tdep->m_profile_psp_ns_regnum)
+	{
+	  cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
+	  cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
+	}
+    }
 
   cache->active_sp_regnum = sp_regnum;
 }
-- 
2.25.1

[PATCH v2 2/4] gdb/arm: Ensure that stack pointers are in sync

[Torbjörn SVENSSON]

Without this patch, sp might be secure, but msp or psp is non-secure
(this state can not happen in the hardware).

Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
---
 gdb/arm-tdep.c | 86 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 28 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 124a94dc87d..c011b2aa973 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -324,20 +324,6 @@ reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
   return psr;
 }
 
-/* Initialize stack pointers, and flag the active one.  */
-
-static inline void
-arm_cache_init_sp (int regnum, CORE_ADDR* member,
-				      struct arm_prologue_cache *cache,
-				      frame_info_ptr frame)
-{
-  CORE_ADDR val = get_frame_register_unsigned (frame, regnum);
-  if (val == cache->sp)
-    cache->active_sp_regnum = regnum;
-
-  *member = val;
-}
-
 /* Initialize CACHE fields for which zero is not adequate (CACHE is
    expected to have been ZALLOC'ed before calling this function).  */
 
@@ -362,34 +348,78 @@ arm_cache_init (struct arm_prologue_cache *cache, frame_info_ptr frame)
 
   if (tdep->have_sec_ext)
     {
-      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
-      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
-
-      arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
-
+      const CORE_ADDR msp_val
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
+      const CORE_ADDR psp_val
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
+
+      cache->msp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
+      cache->msp_ns
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_ns_regnum);
+      cache->psp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
+      cache->psp_ns
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_ns_regnum);
+
+      /* Identify what msp is alias for (msp_s or msp_ns).  */
       if (msp_val == cache->msp_s)
 	cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
       else if (msp_val == cache->msp_ns)
 	cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine msp alias."));
+	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
+	}
+
+      /* Identify what psp is alias for (psp_s or psp_ns).  */
       if (psp_val == cache->psp_s)
 	cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
       else if (psp_val == cache->psp_ns)
 	cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine psp alias."));
+	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
+	}
 
-      /* Use MSP_S as default stack pointer.  */
-      if (cache->active_sp_regnum == ARM_SP_REGNUM)
-	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
+      /* Identify what sp is alias for (msp_s, msp_ns, psp_s or psp_ns).  */
+      if (msp_val == cache->sp)
+	cache->active_sp_regnum = cache->active_msp_regnum;
+      else if (psp_val == cache->sp)
+	cache->active_sp_regnum = cache->active_psp_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine sp alias."));
+	  cache->active_sp_regnum = cache->active_msp_regnum;
+	}
     }
   else if (tdep->is_m)
     {
-      arm_cache_init_sp (tdep->m_profile_msp_regnum, &cache->msp_s, cache, frame);
-      arm_cache_init_sp (tdep->m_profile_psp_regnum, &cache->psp_s, cache, frame);
+      cache->msp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
+      cache->psp_s
+	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
+
+      /* Identify what sp is alias for (msp or psp).  */
+      if (cache->msp_s == cache->sp)
+	cache->active_sp_regnum = tdep->m_profile_msp_regnum;
+      else if (cache->psp_s == cache->sp)
+	cache->active_sp_regnum = tdep->m_profile_psp_regnum;
+      else
+	{
+	  warning (_("Invalid state, unable to determine sp alias."));
+	  cache->active_sp_regnum = tdep->m_profile_msp_regnum;
+	}
     }
   else
-    arm_cache_init_sp (ARM_SP_REGNUM, &cache->msp_s, cache, frame);
+    {
+      cache->msp_s
+	= get_frame_register_unsigned (frame, ARM_SP_REGNUM);
+
+      cache->active_sp_regnum = ARM_SP_REGNUM;
+    }
 }
 
 /* Return the requested stack pointer value (in REGNUM), taking into
-- 
2.25.1

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

[Torbjörn SVENSSON]

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 unqiue 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/dwarf2/frame.h | 20 +++++++++++++++++--
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index 3f884abe1d5..bff3b706e7e 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
 }
 
 \f
+struct dwarf2_frame_fn_data
+{
+  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
+		       int regnum);
+  void *data;
+  struct dwarf2_frame_fn_data* next;
+};
+
 struct dwarf2_frame_cache
 {
   /* DWARF Call Frame Address.  */
@@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
     }
 }
 
+void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
+				fn_prev_register fn)
+{
+  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->fn == fn)
+      return fn_data->data;
+
+  return nullptr;
+}
+
+void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
+				     void **this_cache,
+				     fn_prev_register fn, 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, fn);
+  if (data)
+    return data;
+
+  /* No object found, lets create a new instance.  */
+  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
+  fn_data->fn = fn;
+  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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
+
+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,
+				       void **this_cache,
+				       fn_prev_register fn);
+
 #endif /* dwarf2-frame.h */
-- 
2.25.1

[PATCH v2 4/4] gdb/arm: Use new dwarf2 function cache

[Torbjörn SVENSSON]

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 c011b2aa973..59cd0964d96 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3953,6 +3953,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)
@@ -3961,6 +3973,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)
     {
@@ -4000,33 +4054,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;
@@ -4034,17 +4073,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 v2 3/4] gdb: dwarf2 generic implementation for caching function data

[Torbjorn SVENSSON]

Hi,

On 2022-11-18 16:52, Torbjörn SVENSSON wrote:
> 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 unqiue 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>   2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 3f884abe1d5..bff3b706e7e 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>   }
>   
>   \f
> +struct dwarf2_frame_fn_data
> +{
> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);
> +  void *data;
> +  struct dwarf2_frame_fn_data* next;
> +};
> +
>   struct dwarf2_frame_cache
>   {
>     /* DWARF Call Frame Address.  */
> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>       }
>   }
>   
> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register fn)
> +{
> +  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->fn == fn)
> +      return fn_data->data;
> +
> +  return nullptr;
> +}
> +
> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +				     void **this_cache,
> +				     fn_prev_register fn, 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, fn);
> +  if (data)
> +    return data;
> +
> +  /* No object found, lets create a new instance.  */
> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
> +  fn_data->fn = fn;
> +  fn_data->data = frame_obstack_zalloc (size);

Since these 2 blocks (fn_data and fn_data->fn) are allocated on the 
obstack, do we need to release them in dwarf2_frame_dealloc_cache() or 
just leave them to the generic garbage collection?

Kind regards,
Torbjörn

> +  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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
> +
> +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,
> +				       void **this_cache,
> +				       fn_prev_register fn);
> +
>   #endif /* dwarf2-frame.h */

Re: [PATCH v2 2/4] gdb/arm: Ensure that stack pointers are in sync

[Luis Machado]

On 11/18/22 15:52, Torbjörn SVENSSON wrote:
> Without this patch, sp might be secure, but msp or psp is non-secure
> (this state can not happen in the hardware).

Maybe expand to make it clear we're patching things so we correctly sync msp/psp to the active sp.

> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
>   gdb/arm-tdep.c | 86 ++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 124a94dc87d..c011b2aa973 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -324,20 +324,6 @@ reconstruct_t_bit(struct gdbarch *gdbarch, CORE_ADDR lr, ULONGEST psr)
>     return psr;
>   }
>   
> -/* Initialize stack pointers, and flag the active one.  */
> -
> -static inline void
> -arm_cache_init_sp (int regnum, CORE_ADDR* member,
> -				      struct arm_prologue_cache *cache,
> -				      frame_info_ptr frame)
> -{
> -  CORE_ADDR val = get_frame_register_unsigned (frame, regnum);
> -  if (val == cache->sp)
> -    cache->active_sp_regnum = regnum;
> -
> -  *member = val;
> -}
> -
>   /* Initialize CACHE fields for which zero is not adequate (CACHE is
>      expected to have been ZALLOC'ed before calling this function).  */
>   
> @@ -362,34 +348,78 @@ arm_cache_init (struct arm_prologue_cache *cache, frame_info_ptr frame)
>   
>     if (tdep->have_sec_ext)
>       {
> -      CORE_ADDR msp_val = get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> -      CORE_ADDR psp_val = get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> -
> -      arm_cache_init_sp (tdep->m_profile_msp_s_regnum, &cache->msp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_s_regnum, &cache->psp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_msp_ns_regnum, &cache->msp_ns, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_ns_regnum, &cache->psp_ns, cache, frame);
> -
> +      const CORE_ADDR msp_val
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_regnum);
> +      const CORE_ADDR psp_val
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_regnum);
> +
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
> +      cache->msp_ns
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_ns_regnum);
> +      cache->psp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
> +      cache->psp_ns
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_ns_regnum);
> +
> +      /* Identify what msp is alias for (msp_s or msp_ns).  */
>         if (msp_val == cache->msp_s)
>   	cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
>         else if (msp_val == cache->msp_ns)
>   	cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine msp alias."));
> +	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;

It seems we're assuming msp is msp_s, so it would be nice to put that in the warning as well.

warning (_("Invalid state, unable to determine msp alias, assuming msp_s."));

> +	}
> +
> +      /* Identify what psp is alias for (psp_s or psp_ns).  */
>         if (psp_val == cache->psp_s)
>   	cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
>         else if (psp_val == cache->psp_ns)
>   	cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine psp alias."));
> +	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
> +	}
>   
> -      /* Use MSP_S as default stack pointer.  */
> -      if (cache->active_sp_regnum == ARM_SP_REGNUM)
> -	  cache->active_sp_regnum = tdep->m_profile_msp_s_regnum;
> +      /* Identify what sp is alias for (msp_s, msp_ns, psp_s or psp_ns).  */
> +      if (msp_val == cache->sp)
> +	cache->active_sp_regnum = cache->active_msp_regnum;
> +      else if (psp_val == cache->sp)
> +	cache->active_sp_regnum = cache->active_psp_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine sp alias."));
> +	  cache->active_sp_regnum = cache->active_msp_regnum;
> +	}

Same here. "assuming msp".

>       }
>     else if (tdep->is_m)
>       {
> -      arm_cache_init_sp (tdep->m_profile_msp_regnum, &cache->msp_s, cache, frame);
> -      arm_cache_init_sp (tdep->m_profile_psp_regnum, &cache->psp_s, cache, frame);
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_msp_s_regnum);
> +      cache->psp_s
> +	= get_frame_register_unsigned (frame, tdep->m_profile_psp_s_regnum);
> +
> +      /* Identify what sp is alias for (msp or psp).  */
> +      if (cache->msp_s == cache->sp)
> +	cache->active_sp_regnum = tdep->m_profile_msp_regnum;
> +      else if (cache->psp_s == cache->sp)
> +	cache->active_sp_regnum = tdep->m_profile_psp_regnum;
> +      else
> +	{
> +	  warning (_("Invalid state, unable to determine sp alias."));
> +	  cache->active_sp_regnum = tdep->m_profile_msp_regnum;

Same here.

> +	}
>       }
>     else
> -    arm_cache_init_sp (ARM_SP_REGNUM, &cache->msp_s, cache, frame);
> +    {
> +      cache->msp_s
> +	= get_frame_register_unsigned (frame, ARM_SP_REGNUM);
> +
> +      cache->active_sp_regnum = ARM_SP_REGNUM;
> +    }
>   }
>   
>   /* Return the requested stack pointer value (in REGNUM), taking into

Otherwise this looks OK to me.

Re: [PATCH v2 1/4] gdb/arm: Update active msp/psp when switching stack

[Luis Machado]

On 11/18/22 15:52, Torbjörn SVENSSON wrote:
> For targets with secext, msp and psp can be seen as an alias for one
> of msp_s, msp_ns, psp_s or psp_ns. When switching active sp, the
> coresponding msp/psp needs to be switched too.

coresponding -> corresponding.

> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
>   gdb/arm-tdep.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 7cb3f5f3050..124a94dc87d 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -504,8 +504,23 @@ arm_cache_switch_prev_sp (struct arm_prologue_cache *cache,
>     gdb_assert (arm_is_alternative_sp_register (tdep, sp_regnum));
>   
>     if (tdep->have_sec_ext)
> -    gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
> -		&& sp_regnum != tdep->m_profile_psp_regnum);
> +    {
> +      gdb_assert (sp_regnum != tdep->m_profile_msp_regnum
> +		  && sp_regnum != tdep->m_profile_psp_regnum);
> +
> +      if (sp_regnum == tdep->m_profile_msp_s_regnum
> +	  || sp_regnum == tdep->m_profile_psp_s_regnum)
> +	{
> +	  cache->active_msp_regnum = tdep->m_profile_msp_s_regnum;
> +	  cache->active_psp_regnum = tdep->m_profile_psp_s_regnum;
> +	}
> +      else if (sp_regnum == tdep->m_profile_msp_ns_regnum
> +	       || sp_regnum == tdep->m_profile_psp_ns_regnum)
> +	{
> +	  cache->active_msp_regnum = tdep->m_profile_msp_ns_regnum;
> +	  cache->active_psp_regnum = tdep->m_profile_psp_ns_regnum;
> +	}
> +    }
>   
>     cache->active_sp_regnum = sp_regnum;
>   }

This is OK with the commit message adjusted.

Thanks,
Luis

Re: [PATCH v2 4/4] gdb/arm: Use new dwarf2 function cache

[Luis Machado]

Hi,

On 11/18/22 15:52, Torbjörn SVENSSON wrote:
> 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 c011b2aa973..59cd0964d96 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3953,6 +3953,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.  */

coresponding -> corresponding

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

Given SP is the cfa, do we need to cache it here?

>   static struct value *
>   arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>   			  int regnum)
> @@ -3961,6 +3973,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);

We fetch ARM_SP_REGNUM in both legs of the conditional. How about moving it outside of the conditional blocks?

> +
> +	  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)
>       {
> @@ -4000,33 +4054,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;
> @@ -4034,17 +4073,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;
>   	}

As we've discussed off-list, I think we can reduce the number of get_frame_register_unsigned calls we do for each call to arm_dwarf2_prev_register by using some conditionals.

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

[Luis Machado]

Hi,

On 11/18/22 15:52, Torbjörn SVENSSON wrote:
> 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 unqiue cache object.

unqiue -> unique

> 
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>   2 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
> index 3f884abe1d5..bff3b706e7e 100644
> --- a/gdb/dwarf2/frame.c
> +++ b/gdb/dwarf2/frame.c
> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>   }
>   
>   \f
> +struct dwarf2_frame_fn_data
> +{
> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);
> +  void *data;
> +  struct dwarf2_frame_fn_data* next;
> +};
> +

I'm wondering if we really need to have a function pointer here. Isn't the cache supposed to be frame-wide and not
function-specific?

If we don't need it, the cache just becomes an opaque data pointer.

>   struct dwarf2_frame_cache
>   {
>     /* DWARF Call Frame Address.  */
> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>       }
>   }
>   
> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register fn)
> +{
> +  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->fn == fn)
> +      return fn_data->data;
> +
> +  return nullptr;
> +}
> +
> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
> +				     void **this_cache,
> +				     fn_prev_register fn, 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, fn);
> +  if (data)
> +    return data;
> +
> +  /* No object found, lets create a new instance.  */
> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
> +  fn_data->fn = fn;
> +  fn_data->data = frame_obstack_zalloc (size);
> +  fn_data->next = cache->fn_data;
> +  cache->fn_data = fn_data;
> +
> +  return fn_data->data;
> +}

And if we only have a data pointer, we can return a reference to it through the argument, and then DWARF can cache it.

We could even have a destructor/cleanup that can get called once the frames are destroyed.

> +
>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
> +
> +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,
> +				       void **this_cache,
> +				       fn_prev_register fn);
> +
>   #endif /* dwarf2-frame.h */

As we've discussed before, I think the cache idea is nice if we have to deal with targets with multiple CFA's (in our case, we have either 4 SP's or 2 SP's, plus aliases).

DWARF doesn't seem to support this at the moment, and the function HOW for DWARF is not smart enough to remember a previously-fetched value. So it seems we have room
for some improvement, unless there is enough reason elsewhere about why we shouldn't have a cache.

It would be nice to have some opinions from others, so we can potentially shape this in a way that makes it useful for the general case.

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

[Torbjorn SVENSSON]

Hi,

I've had a long discussion with Luis on IRC regarding the points 
mentioned here, but I'll reply to the list now in order to get more eyes 
on the topic.


On 2022-11-21 22:16, Luis Machado wrote:
> Hi,
> 
> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>> 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 unqiue cache object.
> 
> unqiue -> unique
> 
>>
>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>> index 3f884abe1d5..bff3b706e7e 100644
>> --- a/gdb/dwarf2/frame.c
>> +++ b/gdb/dwarf2/frame.c
>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, 
>> CORE_ADDR pc,
>>   }
>>   \f
>> +struct dwarf2_frame_fn_data
>> +{
>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>> +               int regnum);
>> +  void *data;
>> +  struct dwarf2_frame_fn_data* next;
>> +};
>> +
> 
> I'm wondering if we really need to have a function pointer here. Isn't 
> the cache supposed to be frame-wide and not
> function-specific?
> 
> If we don't need it, the cache just becomes an opaque data pointer.
> 
>>   struct dwarf2_frame_cache
>>   {
>>     /* DWARF Call Frame Address.  */
>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr 
>> this_frame, void **this_cache,
>>       }
>>   }
>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void 
>> **this_cache,
>> +                fn_prev_register fn)
>> +{
>> +  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->fn == fn)
>> +      return fn_data->data;
>> +
>> +  return nullptr;
>> +}
>> +
>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>> +                     void **this_cache,
>> +                     fn_prev_register fn, 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, fn);
>> +  if (data)
>> +    return data;
>> +
>> +  /* No object found, lets create a new instance.  */
>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>> +  fn_data->fn = fn;
>> +  fn_data->data = frame_obstack_zalloc (size);
>> +  fn_data->next = cache->fn_data;
>> +  cache->fn_data = fn_data;
>> +
>> +  return fn_data->data;
>> +}
> 
> And if we only have a data pointer, we can return a reference to it 
> through the argument, and then DWARF can cache it.
> 
> We could even have a destructor/cleanup that can get called once the 
> frames are destroyed.

I don't think we can do that without introducing a lot more changes to 
the common code. My changes are designed in a way that would only have 
an impact on arm (as they are the only users for the functionality right 
now) and not for every target out there that GDB supports. If going for 
a simpler solution, it would mean that every target needs to be 
re-tested in order to get the confirmation that the implementation would 
not break some other target.


> 
>> +
>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>> +
>> +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,
>> +                       void **this_cache,
>> +                       fn_prev_register fn);
>> +
>>   #endif /* dwarf2-frame.h */
> 
> As we've discussed before, I think the cache idea is nice if we have to 
> deal with targets with multiple CFA's (in our case, we have either 4 
> SP's or 2 SP's, plus aliases).
> 
> DWARF doesn't seem to support this at the moment, and the function HOW 
> for DWARF is not smart enough to remember a previously-fetched value. So 
> it seems we have room
> for some improvement, unless there is enough reason elsewhere about why 
> we shouldn't have a cache.


This patch does not provide a cache or anything, it just provides a way 
for the callback function to save some additional data between calls for 
the same frame.
The code above is generic in the way that it has one data object per 
function and frame. The reason for this implementation is that it's 
rather easy to ensure that the data object is okay for the function that 
uses it without any inter-dependencies with some other function that 
might be called for some other register on the same frame. You could 
even consider having a shared function to be a callback function. In the 
case of a shared function, that would mean that the object would be 
large and public and then it would simply make more sense to make the 
dwarf2 object public and extend it instead.
My approach ensures that each callback function has its own data and the 
data structure is "private" to the function. It's possible to share the 
struct for the data object between 2 functions, but it's not possible to 
share the instance of the struct between 2 functions.


> It would be nice to have some opinions from others, so we can 
> potentially shape this in a way that makes it useful for the general case.

Yes. Please give me some more feedback to work on!

Kind regards,
Torbjörn

Re: [PATCH v2 4/4] gdb/arm: Use new dwarf2 function cache

[Torbjorn SVENSSON]

Hi,

I've had a long discussion with Luis on IRC regarding the points 
mentioned here, but I'll reply to the list now in order to get more eyes 
on the topic.

On 2022-11-21 22:04, Luis Machado wrote:
> Hi,
> 
> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>> 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 c011b2aa973..59cd0964d96 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3953,6 +3953,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.  */
> 
> coresponding -> corresponding
> 
>> +  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;
>> +};
>> +
> 
> Given SP is the cfa, do we need to cache it here?

As I said off-list, it's not the value of sp, msp etc, it's the value of 
the inner frame, so what is actually "cached" here is basically the 
state of the frame, not the values.
The cache could be simplified in a few ways, but before doing the 
polishing, I would like to know if it's an acceptable way to implement 
the fix for the performance issue.

> 
>>   static struct value *
>>   arm_dwarf2_prev_register (frame_info_ptr this_frame, void **this_cache,
>>                 int regnum)
>> @@ -3961,6 +3973,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);
> 
> We fetch ARM_SP_REGNUM in both legs of the conditional. How about moving 
> it outside of the conditional blocks?

Sure. Before doing more here, I would like to get the agreement that 
this is a good approach to the problem.

>> +
>> +      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)
>>       {
>> @@ -4000,33 +4054,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;
>> @@ -4034,17 +4073,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;
>>       }
> 
> As we've discussed off-list, I think we can reduce the number of 
> get_frame_register_unsigned calls we do for each call to 
> arm_dwarf2_prev_register by using some conditionals.

Likely, but let's focus on the dwarf2 part of the patch first and do the 
polishing after, okay?

Kind regards,
Torbjörn

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

[Tomas Vanek]

Hi Torbjorn,

On 29/11/2022 16:19, Torbjorn SVENSSON wrote:
> Hi,
>
> I've had a long discussion with Luis on IRC regarding the points 
> mentioned here, but I'll reply to the list now in order to get more 
> eyes on the topic.
>
>
> On 2022-11-21 22:16, Luis Machado wrote:
>> Hi,
>>
>> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>>> 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 unqiue cache object.
>>
>> unqiue -> unique
>>
>>>
>>> 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 | 48 
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>>> index 3f884abe1d5..bff3b706e7e 100644
>>> --- a/gdb/dwarf2/frame.c
>>> +++ b/gdb/dwarf2/frame.c
>>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, 
>>> CORE_ADDR pc,
>>>   }
>>>   \f
>>> +struct dwarf2_frame_fn_data
>>> +{
>>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>>> +               int regnum);
>>> +  void *data;
>>> +  struct dwarf2_frame_fn_data* next;
>>> +};
>>> +
>>
>> I'm wondering if we really need to have a function pointer here. 
>> Isn't the cache supposed to be frame-wide and not
>> function-specific?
>>
>> If we don't need it, the cache just becomes an opaque data pointer.
>>
>>>   struct dwarf2_frame_cache
>>>   {
>>>     /* DWARF Call Frame Address.  */
>>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr 
>>> this_frame, void **this_cache,
>>>       }
>>>   }
>>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void 
>>> **this_cache,
>>> +                fn_prev_register fn)
>>> +{
>>> +  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->fn == fn)
>>> +      return fn_data->data;
>>> +
>>> +  return nullptr;
>>> +}
>>> +
>>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>>> +                     void **this_cache,
>>> +                     fn_prev_register fn, 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, fn);
>>> +  if (data)
>>> +    return data;
>>> +
>>> +  /* No object found, lets create a new instance.  */
>>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>>> +  fn_data->fn = fn;
>>> +  fn_data->data = frame_obstack_zalloc (size);
>>> +  fn_data->next = cache->fn_data;
>>> +  cache->fn_data = fn_data;
>>> +
>>> +  return fn_data->data;
>>> +}
>>
>> And if we only have a data pointer, we can return a reference to it 
>> through the argument, and then DWARF can cache it.
>>
>> We could even have a destructor/cleanup that can get called once the 
>> frames are destroyed.
>
> I don't think we can do that without introducing a lot more changes to 
> the common code. My changes are designed in a way that would only have 
> an impact on arm (as they are the only users for the functionality 
> right now) and not for every target out there that GDB supports. If 
> going for a simpler solution, it would mean that every target needs to 
> be re-tested in order to get the confirmation that the implementation 
> would not break some other target.
>
>
>>
>>> +
>>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>>> +
>>> +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,
>>> +                       void **this_cache,
>>> +                       fn_prev_register fn);
>>> +
>>>   #endif /* dwarf2-frame.h */
>>
>> As we've discussed before, I think the cache idea is nice if we have 
>> to deal with targets with multiple CFA's (in our case, we have either 
>> 4 SP's or 2 SP's, plus aliases).
>>
>> DWARF doesn't seem to support this at the moment, and the function 
>> HOW for DWARF is not smart enough to remember a previously-fetched 
>> value. So it seems we have room
>> for some improvement, unless there is enough reason elsewhere about 
>> why we shouldn't have a cache.
>
>
> This patch does not provide a cache or anything, it just provides a 
> way for the callback function to save some additional data between 
> calls for the same frame.
> The code above is generic in the way that it has one data object per 
> function and frame. The reason for this implementation is that it's 
> rather easy to ensure that the data object is okay for the function 
> that uses it without any inter-dependencies with some other function 
> that might be called for some other register on the same frame. You 
> could even consider having a shared function to be a callback 
> function. In the case of a shared function, that would mean that the 
> object would be large and public and then it would simply make more 
> sense to make the dwarf2 object public and extend it instead.
> My approach ensures that each callback function has its own data and 
> the data structure is "private" to the function. It's possible to 
> share the struct for the data object between 2 functions, but it's not 
> possible to share the instance of the struct between 2 functions.

Sorry, the per-function-pointers looks like an overkill to me.
Maybe I'm just an old school programmer and don't like associative arrays...
- frame unwinders use a generic pointer and ensuring the proper type 
cast is fully in responsibility of the implementation.
- we need just to replicate the similar functionality for architecture 
dependent handling of dwarf2 frames
- functions assigned to a dwarf2 frame by how = DWARF2_FRAME_REG_FN are 
never isolated functions from different parts
of code: a gdbarch can set only one initializer by 
dwarf2_frame_set_init_reg() and it sets all functions
- if we ever have more than one function assigned in one dwarf2 frame, 
seems me likely that all functions would prefer a single cache over 
isolated ones

>
>
>> It would be nice to have some opinions from others, so we can 
>> potentially shape this in a way that makes it useful for the general 
>> case.
>
> Yes. Please give me some more feedback to work on!
>
> Kind regards,
> Torbjörn

regards
     Tomas

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

[Torbjorn SVENSSON]

Hi,

On 2022-11-29 17:24, Tomas Vanek wrote:
> Hi Torbjorn,
> 
> On 29/11/2022 16:19, Torbjorn SVENSSON wrote:
>> Hi,
>>
>> I've had a long discussion with Luis on IRC regarding the points 
>> mentioned here, but I'll reply to the list now in order to get more 
>> eyes on the topic.
>>
>>
>> On 2022-11-21 22:16, Luis Machado wrote:
>>> Hi,
>>>
>>> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>>>> 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 unqiue cache object.
>>>
>>> unqiue -> unique
>>>
>>>>
>>>> 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 | 48 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>>>> index 3f884abe1d5..bff3b706e7e 100644
>>>> --- a/gdb/dwarf2/frame.c
>>>> +++ b/gdb/dwarf2/frame.c
>>>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, 
>>>> CORE_ADDR pc,
>>>>   }
>>>>   \f
>>>> +struct dwarf2_frame_fn_data
>>>> +{
>>>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>>>> +               int regnum);
>>>> +  void *data;
>>>> +  struct dwarf2_frame_fn_data* next;
>>>> +};
>>>> +
>>>
>>> I'm wondering if we really need to have a function pointer here. 
>>> Isn't the cache supposed to be frame-wide and not
>>> function-specific?
>>>
>>> If we don't need it, the cache just becomes an opaque data pointer.
>>>
>>>>   struct dwarf2_frame_cache
>>>>   {
>>>>     /* DWARF Call Frame Address.  */
>>>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr 
>>>> this_frame, void **this_cache,
>>>>       }
>>>>   }
>>>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void 
>>>> **this_cache,
>>>> +                fn_prev_register fn)
>>>> +{
>>>> +  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->fn == fn)
>>>> +      return fn_data->data;
>>>> +
>>>> +  return nullptr;
>>>> +}
>>>> +
>>>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>>>> +                     void **this_cache,
>>>> +                     fn_prev_register fn, 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, fn);
>>>> +  if (data)
>>>> +    return data;
>>>> +
>>>> +  /* No object found, lets create a new instance.  */
>>>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>>>> +  fn_data->fn = fn;
>>>> +  fn_data->data = frame_obstack_zalloc (size);
>>>> +  fn_data->next = cache->fn_data;
>>>> +  cache->fn_data = fn_data;
>>>> +
>>>> +  return fn_data->data;
>>>> +}
>>>
>>> And if we only have a data pointer, we can return a reference to it 
>>> through the argument, and then DWARF can cache it.
>>>
>>> We could even have a destructor/cleanup that can get called once the 
>>> frames are destroyed.
>>
>> I don't think we can do that without introducing a lot more changes to 
>> the common code. My changes are designed in a way that would only have 
>> an impact on arm (as they are the only users for the functionality 
>> right now) and not for every target out there that GDB supports. If 
>> going for a simpler solution, it would mean that every target needs to 
>> be re-tested in order to get the confirmation that the implementation 
>> would not break some other target.
>>
>>
>>>
>>>> +
>>>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>>>> +
>>>> +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,
>>>> +                       void **this_cache,
>>>> +                       fn_prev_register fn);
>>>> +
>>>>   #endif /* dwarf2-frame.h */
>>>
>>> As we've discussed before, I think the cache idea is nice if we have 
>>> to deal with targets with multiple CFA's (in our case, we have either 
>>> 4 SP's or 2 SP's, plus aliases).
>>>
>>> DWARF doesn't seem to support this at the moment, and the function 
>>> HOW for DWARF is not smart enough to remember a previously-fetched 
>>> value. So it seems we have room
>>> for some improvement, unless there is enough reason elsewhere about 
>>> why we shouldn't have a cache.
>>
>>
>> This patch does not provide a cache or anything, it just provides a 
>> way for the callback function to save some additional data between 
>> calls for the same frame.
>> The code above is generic in the way that it has one data object per 
>> function and frame. The reason for this implementation is that it's 
>> rather easy to ensure that the data object is okay for the function 
>> that uses it without any inter-dependencies with some other function 
>> that might be called for some other register on the same frame. You 
>> could even consider having a shared function to be a callback 
>> function. In the case of a shared function, that would mean that the 
>> object would be large and public and then it would simply make more 
>> sense to make the dwarf2 object public and extend it instead.
>> My approach ensures that each callback function has its own data and 
>> the data structure is "private" to the function. It's possible to 
>> share the struct for the data object between 2 functions, but it's not 
>> possible to share the instance of the struct between 2 functions.
> 
> Sorry, the per-function-pointers looks like an overkill to me.
> Maybe I'm just an old school programmer and don't like associative 
> arrays...
> - frame unwinders use a generic pointer and ensuring the proper type 
> cast is fully in responsibility of the implementation.
> - we need just to replicate the similar functionality for architecture 
> dependent handling of dwarf2 frames
> - functions assigned to a dwarf2 frame by how = DWARF2_FRAME_REG_FN are 
> never isolated functions from different parts
> of code: a gdbarch can set only one initializer by 
> dwarf2_frame_set_init_reg() and it sets all functions
> - if we ever have more than one function assigned in one dwarf2 frame, 
> seems me likely that all functions would prefer a single cache over 
> isolated ones


Based on the points above, can please answer the below questions?

a) Why is there a function pointer per register if it's always going to 
be a single one? It would have made more sense to simply have a single 
bool telling if the function should be called or not if your suggestion 
would make sense.

b) If you want a shared cache object, then why is all the cache types 
private to the compile unit? You can't have something shared across 2 
compile units that are just defined in the .c file.


Don't get me wrong, I'm not against defining the cache types in the 
public namespace. By doing so, it will have a large impact on the GDB 
code base and it will require a lot more testing than the change I'm 
proposing.
- Is it work the extra risk?
- Who can actually do the testing on all the targets to make sure that 
nothing broke?

Kind regards,
Torbjörn


> 
>>
>>
>>> It would be nice to have some opinions from others, so we can 
>>> potentially shape this in a way that makes it useful for the general 
>>> case.
>>
>> Yes. Please give me some more feedback to work on!
>>
>> Kind regards,
>> Torbjörn
> 
> regards
>      Tomas

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

[Luis Machado]

On 11/30/22 10:16, Torbjorn SVENSSON wrote:
> Hi,
> 
> On 2022-11-29 17:24, Tomas Vanek wrote:
>> Hi Torbjorn,
>>
>> On 29/11/2022 16:19, Torbjorn SVENSSON wrote:
>>> Hi,
>>>
>>> I've had a long discussion with Luis on IRC regarding the points mentioned here, but I'll reply to the list now in order to get more eyes on the topic.
>>>
>>>
>>> On 2022-11-21 22:16, Luis Machado wrote:
>>>> Hi,
>>>>
>>>> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>>>>> 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 unqiue cache object.
>>>>
>>>> unqiue -> unique
>>>>
>>>>>
>>>>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>>>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>>>>> index 3f884abe1d5..bff3b706e7e 100644
>>>>> --- a/gdb/dwarf2/frame.c
>>>>> +++ b/gdb/dwarf2/frame.c
>>>>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>>>>>   }
>>>>>   \f
>>>>> +struct dwarf2_frame_fn_data
>>>>> +{
>>>>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>>>>> +               int regnum);
>>>>> +  void *data;
>>>>> +  struct dwarf2_frame_fn_data* next;
>>>>> +};
>>>>> +
>>>>
>>>> I'm wondering if we really need to have a function pointer here. Isn't the cache supposed to be frame-wide and not
>>>> function-specific?
>>>>
>>>> If we don't need it, the cache just becomes an opaque data pointer.
>>>>
>>>>>   struct dwarf2_frame_cache
>>>>>   {
>>>>>     /* DWARF Call Frame Address.  */
>>>>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>>>>>       }
>>>>>   }
>>>>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
>>>>> +                fn_prev_register fn)
>>>>> +{
>>>>> +  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->fn == fn)
>>>>> +      return fn_data->data;
>>>>> +
>>>>> +  return nullptr;
>>>>> +}
>>>>> +
>>>>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>>>>> +                     void **this_cache,
>>>>> +                     fn_prev_register fn, 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, fn);
>>>>> +  if (data)
>>>>> +    return data;
>>>>> +
>>>>> +  /* No object found, lets create a new instance.  */
>>>>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>>>>> +  fn_data->fn = fn;
>>>>> +  fn_data->data = frame_obstack_zalloc (size);
>>>>> +  fn_data->next = cache->fn_data;
>>>>> +  cache->fn_data = fn_data;
>>>>> +
>>>>> +  return fn_data->data;
>>>>> +}
>>>>
>>>> And if we only have a data pointer, we can return a reference to it through the argument, and then DWARF can cache it.
>>>>
>>>> We could even have a destructor/cleanup that can get called once the frames are destroyed.
>>>
>>> I don't think we can do that without introducing a lot more changes to the common code. My changes are designed in a way that would only have an impact on arm (as they are the only users for the functionality right now) and not for every target out there that GDB supports. If going for a simpler solution, it would mean that every target needs to be re-tested in order to get the confirmation that the implementation would not break some other target.
>>>
>>>
>>>>
>>>>> +
>>>>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>>>>> +
>>>>> +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,
>>>>> +                       void **this_cache,
>>>>> +                       fn_prev_register fn);
>>>>> +
>>>>>   #endif /* dwarf2-frame.h */
>>>>
>>>> As we've discussed before, I think the cache idea is nice if we have to deal with targets with multiple CFA's (in our case, we have either 4 SP's or 2 SP's, plus aliases).
>>>>
>>>> DWARF doesn't seem to support this at the moment, and the function HOW for DWARF is not smart enough to remember a previously-fetched value. So it seems we have room
>>>> for some improvement, unless there is enough reason elsewhere about why we shouldn't have a cache.
>>>
>>>
>>> This patch does not provide a cache or anything, it just provides a way for the callback function to save some additional data between calls for the same frame.
>>> The code above is generic in the way that it has one data object per function and frame. The reason for this implementation is that it's rather easy to ensure that the data object is okay for the function that uses it without any inter-dependencies with some other function that might be called for some other register on the same frame. You could even consider having a shared function to be a callback function. In the case of a shared function, that would mean that the object would be large and public and then it would simply make more sense to make the dwarf2 object public and extend it instead.
>>> My approach ensures that each callback function has its own data and the data structure is "private" to the function. It's possible to share the struct for the data object between 2 functions, but it's not possible to share the instance of the struct between 2 functions.
>>
>> Sorry, the per-function-pointers looks like an overkill to me.
>> Maybe I'm just an old school programmer and don't like associative arrays...
>> - frame unwinders use a generic pointer and ensuring the proper type cast is fully in responsibility of the implementation.
>> - we need just to replicate the similar functionality for architecture dependent handling of dwarf2 frames
>> - functions assigned to a dwarf2 frame by how = DWARF2_FRAME_REG_FN are never isolated functions from different parts
>> of code: a gdbarch can set only one initializer by dwarf2_frame_set_init_reg() and it sets all functions
>> - if we ever have more than one function assigned in one dwarf2 frame, seems me likely that all functions would prefer a single cache over isolated ones
> 
> 
> Based on the points above, can please answer the below questions?
> 
> a) Why is there a function pointer per register if it's always going to be a single one? It would have made more sense to simply have a single bool telling if the function should be called or not if your suggestion would make sense.
> 
> b) If you want a shared cache object, then why is all the cache types private to the compile unit? You can't have something shared across 2 compile units that are just defined in the .c file.
> 
> 
> Don't get me wrong, I'm not against defining the cache types in the public namespace. By doing so, it will have a large impact on the GDB code base and it will require a lot more testing than the change I'm proposing.
> - Is it work the extra risk?
> - Who can actually do the testing on all the targets to make sure that nothing broke?
> 

What I see is an area of gdb where we can make some general improvements to help all targets. Testing wouldn't be that much more complicated, specially if we create a new HOW and leave the old one as-is.

> Kind regards,
> Torbjörn
> 
> 
>>
>>>
>>>
>>>> It would be nice to have some opinions from others, so we can potentially shape this in a way that makes it useful for the general case.
>>>
>>> Yes. Please give me some more feedback to work on!
>>>
>>> Kind regards,
>>> Torbjörn
>>
>> regards
>>      Tomas

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

[Luis Machado]

On 11/30/22 10:16, Torbjorn SVENSSON wrote:
> Hi,
> 
> On 2022-11-29 17:24, Tomas Vanek wrote:
>> Hi Torbjorn,
>>
>> On 29/11/2022 16:19, Torbjorn SVENSSON wrote:
>>> Hi,
>>>
>>> I've had a long discussion with Luis on IRC regarding the points mentioned here, but I'll reply to the list now in order to get more eyes on the topic.
>>>
>>>
>>> On 2022-11-21 22:16, Luis Machado wrote:
>>>> Hi,
>>>>
>>>> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>>>>> 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 unqiue cache object.
>>>>
>>>> unqiue -> unique
>>>>
>>>>>
>>>>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>>>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>>>>> index 3f884abe1d5..bff3b706e7e 100644
>>>>> --- a/gdb/dwarf2/frame.c
>>>>> +++ b/gdb/dwarf2/frame.c
>>>>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, CORE_ADDR pc,
>>>>>   }
>>>>>   \f
>>>>> +struct dwarf2_frame_fn_data
>>>>> +{
>>>>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>>>>> +               int regnum);
>>>>> +  void *data;
>>>>> +  struct dwarf2_frame_fn_data* next;
>>>>> +};
>>>>> +
>>>>
>>>> I'm wondering if we really need to have a function pointer here. Isn't the cache supposed to be frame-wide and not
>>>> function-specific?
>>>>
>>>> If we don't need it, the cache just becomes an opaque data pointer.
>>>>
>>>>>   struct dwarf2_frame_cache
>>>>>   {
>>>>>     /* DWARF Call Frame Address.  */
>>>>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
>>>>>       }
>>>>>   }
>>>>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
>>>>> +                fn_prev_register fn)
>>>>> +{
>>>>> +  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->fn == fn)
>>>>> +      return fn_data->data;
>>>>> +
>>>>> +  return nullptr;
>>>>> +}
>>>>> +
>>>>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>>>>> +                     void **this_cache,
>>>>> +                     fn_prev_register fn, 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, fn);
>>>>> +  if (data)
>>>>> +    return data;
>>>>> +
>>>>> +  /* No object found, lets create a new instance.  */
>>>>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>>>>> +  fn_data->fn = fn;
>>>>> +  fn_data->data = frame_obstack_zalloc (size);
>>>>> +  fn_data->next = cache->fn_data;
>>>>> +  cache->fn_data = fn_data;
>>>>> +
>>>>> +  return fn_data->data;
>>>>> +}
>>>>
>>>> And if we only have a data pointer, we can return a reference to it through the argument, and then DWARF can cache it.
>>>>
>>>> We could even have a destructor/cleanup that can get called once the frames are destroyed.
>>>
>>> I don't think we can do that without introducing a lot more changes to the common code. My changes are designed in a way that would only have an impact on arm (as they are the only users for the functionality right now) and not for every target out there that GDB supports. If going for a simpler solution, it would mean that every target needs to be re-tested in order to get the confirmation that the implementation would not break some other target.
>>>
>>>
>>>>
>>>>> +
>>>>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>>>>> +
>>>>> +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,
>>>>> +                       void **this_cache,
>>>>> +                       fn_prev_register fn);
>>>>> +
>>>>>   #endif /* dwarf2-frame.h */
>>>>
>>>> As we've discussed before, I think the cache idea is nice if we have to deal with targets with multiple CFA's (in our case, we have either 4 SP's or 2 SP's, plus aliases).
>>>>
>>>> DWARF doesn't seem to support this at the moment, and the function HOW for DWARF is not smart enough to remember a previously-fetched value. So it seems we have room
>>>> for some improvement, unless there is enough reason elsewhere about why we shouldn't have a cache.
>>>
>>>
>>> This patch does not provide a cache or anything, it just provides a way for the callback function to save some additional data between calls for the same frame.
>>> The code above is generic in the way that it has one data object per function and frame. The reason for this implementation is that it's rather easy to ensure that the data object is okay for the function that uses it without any inter-dependencies with some other function that might be called for some other register on the same frame. You could even consider having a shared function to be a callback function. In the case of a shared function, that would mean that the object would be large and public and then it would simply make more sense to make the dwarf2 object public and extend it instead.
>>> My approach ensures that each callback function has its own data and the data structure is "private" to the function. It's possible to share the struct for the data object between 2 functions, but it's not possible to share the instance of the struct between 2 functions.
>>
>> Sorry, the per-function-pointers looks like an overkill to me.
>> Maybe I'm just an old school programmer and don't like associative arrays...
>> - frame unwinders use a generic pointer and ensuring the proper type cast is fully in responsibility of the implementation.
>> - we need just to replicate the similar functionality for architecture dependent handling of dwarf2 frames
>> - functions assigned to a dwarf2 frame by how = DWARF2_FRAME_REG_FN are never isolated functions from different parts
>> of code: a gdbarch can set only one initializer by dwarf2_frame_set_init_reg() and it sets all functions
>> - if we ever have more than one function assigned in one dwarf2 frame, seems me likely that all functions would prefer a single cache over isolated ones
> 
> 
> Based on the points above, can please answer the below questions?

I'll try to clarify some of them from my personal perspective.

> 
> a) Why is there a function pointer per register if it's always going to be a single one? It would have made more sense to simply have a single bool telling if the function should be called or not if your suggestion would make sense.

That's correct. Each register (dwarf register column) gets to define a HOW, so you can have different functions for different registers. It may not be useful to have multiple different functions for this purpose though.

> 
> b) If you want a shared cache object, then why is all the cache types private to the compile unit? You can't have something shared across 2 compile units that are just defined in the .c file.

There is probably some historic reason for doing it that way. My guess is that we want to isolate the dwarf unwinding machinery as much as possible from the rest of the code. Having other code influencing the dwarf
unwinding logic may not be desirable. But then we have ways to do it, like the callback HOW.

With that said, I agree with Tomas that we should have a single data pointer/cache per frame. If we need to touch a bit more generic code, that is fine as long as it is done in a flexible future-proof way.

We already have a good mechanism for storing register values, and that's the trad_frame_saved_reg structure that we use for most of the prologue unwinder/analyzer code. That structure allocates things on the
frame obstack, so memory release is done automatically when the frame data gets flushed.

A getter function is needed so we can return the trad_frame_saved_reg array contained in the opaque (to the arch-specific code) dwarf2_frame_cache struct.

With the trad_frame_saved_reg array in hand, the arch-specific dwarf2 unwinder can cache some useful data for registers that don't have dwarf id's.

> 
> 
> Don't get me wrong, I'm not against defining the cache types in the public namespace. By doing so, it will have a large impact on the GDB code base and it will require a lot more testing than the change I'm proposing.

If we define a new HOW for this, the changes will be easier to test. Code using the non-cached version of the callback HOW will behave the same as before.

We could potentially have a single callback HOW and teach gdb how to detect that an arch-specific cache is being used. But I think that is beyond the scope of fixing the performance issue here.

> - Is it work the extra risk?
> - Who can actually do the testing on all the targets to make sure that nothing broke?
> 
> Kind regards,
> Torbjörn
> 
> 
>>
>>>
>>>
>>>> It would be nice to have some opinions from others, so we can potentially shape this in a way that makes it useful for the general case.
>>>
>>> Yes. Please give me some more feedback to work on!
>>>
>>> Kind regards,
>>> Torbjörn
>>
>> regards
>>      Tomas

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

[Torbjorn SVENSSON]

Ping! :)

Can someone (Simon?, Andrew?, Pedro?, Joel?) take a look at the proposed 
patch?
I would like to conclude something for the GDB13 release...

Kind regards,
Torbjörn


On 2022-11-21 22:16, Luis Machado wrote:
> Hi,
> 
> On 11/18/22 15:52, Torbjörn SVENSSON wrote:
>> 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 unqiue cache object.
> 
> unqiue -> unique
> 
>>
>> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>   gdb/dwarf2/frame.h | 20 +++++++++++++++++--
>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
>> index 3f884abe1d5..bff3b706e7e 100644
>> --- a/gdb/dwarf2/frame.c
>> +++ b/gdb/dwarf2/frame.c
>> @@ -831,6 +831,14 @@ dwarf2_fetch_cfa_info (struct gdbarch *gdbarch, 
>> CORE_ADDR pc,
>>   }
>>   \f
>> +struct dwarf2_frame_fn_data
>> +{
>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>> +               int regnum);
>> +  void *data;
>> +  struct dwarf2_frame_fn_data* next;
>> +};
>> +
> 
> I'm wondering if we really need to have a function pointer here. Isn't 
> the cache supposed to be frame-wide and not
> function-specific?
> 
> If we don't need it, the cache just becomes an opaque data pointer.
> 
>>   struct dwarf2_frame_cache
>>   {
>>     /* DWARF Call Frame Address.  */
>> @@ -862,6 +870,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 +1231,44 @@ dwarf2_frame_prev_register (frame_info_ptr 
>> this_frame, void **this_cache,
>>       }
>>   }
>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void 
>> **this_cache,
>> +                fn_prev_register fn)
>> +{
>> +  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->fn == fn)
>> +      return fn_data->data;
>> +
>> +  return nullptr;
>> +}
>> +
>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>> +                     void **this_cache,
>> +                     fn_prev_register fn, 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, fn);
>> +  if (data)
>> +    return data;
>> +
>> +  /* No object found, lets create a new instance.  */
>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>> +  fn_data->fn = fn;
>> +  fn_data->data = frame_obstack_zalloc (size);
>> +  fn_data->next = cache->fn_data;
>> +  cache->fn_data = fn_data;
>> +
>> +  return fn_data->data;
>> +}
> 
> And if we only have a data pointer, we can return a reference to it 
> through the argument, and then DWARF can cache it.
> 
> We could even have a destructor/cleanup that can get called once the 
> frames are destroyed.
> 
>> +
>>   /* 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 06c8a10c178..444afd9f8eb 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,18 @@ 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.  */
>> +
>> +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,
>> +                       void **this_cache,
>> +                       fn_prev_register fn);
>> +
>>   #endif /* dwarf2-frame.h */
> 
> As we've discussed before, I think the cache idea is nice if we have to 
> deal with targets with multiple CFA's (in our case, we have either 4 
> SP's or 2 SP's, plus aliases).
> 
> DWARF doesn't seem to support this at the moment, and the function HOW 
> for DWARF is not smart enough to remember a previously-fetched value. So 
> it seems we have room
> for some improvement, unless there is enough reason elsewhere about why 
> we shouldn't have a cache.
> 
> It would be nice to have some opinions from others, so we can 
> potentially shape this in a way that makes it useful for the general case.

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

[Tom Tromey]

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

> +struct dwarf2_frame_fn_data
> +{

New type should have a comment.

> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);

Shouldn't this use the fn_prev_register typedef?

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

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

> +
> +/* 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.

Tom

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

[Tom Tromey]

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

>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>> +  fn_data->fn = fn;
>> +  fn_data->data = frame_obstack_zalloc (size);

Torbjorn> Since these 2 blocks (fn_data and fn_data->fn) are allocated on the
Torbjorn> obstack, do we need to release them in dwarf2_frame_dealloc_cache() or
Torbjorn> just leave them to the generic garbage collection?

They can just be left.  The obstack as a whole is destroyed at once.
However, you probably should consider the lifetime management of the
data that's attached to the frame.  That said, it's fine if the answer
is that they should be allocated on the obstack as well.

Tom

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

[Torbjorn SVENSSON]

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

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

[Torbjorn SVENSSON]

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

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

[Joel Brobecker]

On Wed, Dec 28, 2022 at 05:16:03PM +0100, Torbjorn SVENSSON via Gdb-patches 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.

See for instance is_fixed_point_type's declaration in gdbtypes.h:

    /* Return True if TYPE is a TYPE_CODE_FIXED_POINT or if TYPE is
       a range type whose base type is a TYPE_CODE_FIXED_POINT.  */
    extern bool is_fixed_point_type (struct type *type);

... where we provide a description of what the function does, including
information about all the parameters. And then the corresponding
implementation in gdbtypes.c:

    /* See gdbtypes.h.  */

    bool
    is_fixed_point_type (struct type *type)
    {

The goal here is to provide the function's documentation at
the API level, rather than at the implementation level, for
all situations where it is declared like so.

For situations where the function is declared static,
we put the function's documentation next to the implementation,
as there isn't always a declaration.

The "See xxx.h" comment is there to confirm where the function's
documentation can be found.

-- 
Joel

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

[Tom Tromey]

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

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

> +struct dwarf2_frame_fn_data
> +{

This struct should have some kind of introductory comment, as should the
fields.

> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
> +		       int regnum);

Seems like this should use the fn_prev_register typedef.  (But see below.)

> +  void *data;
> +  struct dwarf2_frame_fn_data* next;

Wrong "*" placement.

> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
> +				fn_prev_register fn)

Wrong formatting.

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

Wrong formatting.

> +{
> +  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, fn);
> +  if (data)
> +    return data;
> +
> +  /* No object found, lets create a new instance.  */
> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
> +  fn_data->fn = fn;
> +  fn_data->data = frame_obstack_zalloc (size);
> +  fn_data->next = cache->fn_data;
> +  cache->fn_data = fn_data;
> +
> +  return fn_data->data;

This API seems a bit weird to me.

It seems like the 'fn' parameter is never really used.  It's maybe just
a sort of cookie.  But if so, I think it would be better to just use a
'const void *' or 'const char *' or something like that.  (A string is
nice because then it can also be seen in the debugger and give a clue
where it came from.)

Ok, I dug up the follow-up patch and indeed it is just a cookie.  I
think naming it as such and changing the type would make this more
clear.

Also in the follow-up I see that it calls dwarf2_frame_fn_data first.
So if you're going to go that route, then it seems that
dwarf2_frame_allocate_fn_data does not need to find an existing
object -- it can just assert there isn't one.

> +
> +/* 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,
> +				       void **this_cache,
> +				       fn_prev_register fn);

IMO both of these could use a longer comment.  From this it's impossible
to tell what the point of them is.

thanks,
Tom

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

[Torbjorn SVENSSON]

On 2023-01-18 19:47, 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.
> 
>> 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.
> 
>> +struct dwarf2_frame_fn_data
>> +{
> 
> This struct should have some kind of introductory comment, as should the
> fields.

Fixed in v3.

> 
>> +  struct value *(*fn) (frame_info_ptr this_frame, void **this_cache,
>> +		       int regnum);
> 
> Seems like this should use the fn_prev_register typedef.  (But see below.)
> 
>> +  void *data;
>> +  struct dwarf2_frame_fn_data* next;
> 
> Wrong "*" placement.

Fixed in v3.

> 
>> +void *dwarf2_frame_get_fn_data (frame_info_ptr this_frame, void **this_cache,
>> +				fn_prev_register fn)
> 
> Wrong formatting.
> 
>> +void *dwarf2_frame_allocate_fn_data (frame_info_ptr this_frame,
>> +				     void **this_cache,
>> +				     fn_prev_register fn, unsigned long size)
> 
> Wrong formatting.

I think the format is correct, but it looks strange in the mail as the 
'+' sign is "eaten" by the first tab. Looking in my editor for the 
source code, it looks correct.

>> +{
>> +  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, fn);
>> +  if (data)
>> +    return data;
>> +
>> +  /* No object found, lets create a new instance.  */
>> +  fn_data = FRAME_OBSTACK_ZALLOC (struct dwarf2_frame_fn_data);
>> +  fn_data->fn = fn;
>> +  fn_data->data = frame_obstack_zalloc (size);
>> +  fn_data->next = cache->fn_data;
>> +  cache->fn_data = fn_data;
>> +
>> +  return fn_data->data;
> 
> This API seems a bit weird to me.
> 
> It seems like the 'fn' parameter is never really used.  It's maybe just
> a sort of cookie.  But if so, I think it would be better to just use a
> 'const void *' or 'const char *' or something like that.  (A string is
> nice because then it can also be seen in the debugger and give a clue
> where it came from.)
> 
> Ok, I dug up the follow-up patch and indeed it is just a cookie.  I
> think naming it as such and changing the type would make this more
> clear.

Yes, it's the cookie/key to identify the right object for the 
prev_register function. In v3, I've renamed the variable (and the 
member) to be called "cookie" instead if that's better. The "fn" name 
came from the struct where it was originally defined.
The cookie/key could be a string, but then it should be auto-generated 
when calling the functions rather than letting the user type it. The 
reason is that if the user types it, it's less clear what the 
consequences of reusing it will be. Also, if we go for a string, it 
would consume more memory than having a function pointer (not that 
memory should be an issue, but anyway...).
If there is a function pointer, doesn't GDB lookup the symbol for the 
function pointer if there are debug symbols in the application?

> 
> Also in the follow-up I see that it calls dwarf2_frame_fn_data first.
> So if you're going to go that route, then it seems that
> dwarf2_frame_allocate_fn_data does not need to find an existing
> object -- it can just assert there isn't one.

What happens if there are 2 prev_register functions that both wants some 
custom data? With the approach I have (looping over the list and 
returning the matching data for the cookie/key) would allow us to reuse 
the existing object rather than create a new one. I could change the 
block that checks if there was a match and returns it to an assert, but 
what would the benefit be? Instead of reusing the existing object, we 
would crash GDB. Is this better?
> 
>> +
>> +/* 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,
>> +				       void **this_cache,
>> +				       fn_prev_register fn);
> 
> IMO both of these could use a longer comment.  From this it's impossible
> to tell what the point of them is.

Improved in v3.

> 
> thanks,
> Tom

Kind regards,
Torbjörn + Yvan