diff mbox series

[v2,02/11] gdbserver: use inheritance to define tracepoint contexts

Message ID 20241230-upstream-gdbserver-regcache-v2-2-020a9514fcf0@intel.com
State New
Headers
Series gdbserver: refactor regcache |

Commit Message

Tankut Baris Aktemur Dec. 30, 2024, 10:49 a.m. UTC 
  Use inheritance in the definition of tracepoint contexts.  This is a
refactoring that aims to improve the code.  No behavior should be
altered.
---
 gdbserver/tracepoint.cc | 107 ++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 59 deletions(-)

Comments

Simon Marchi Jan. 6, 2025, 9:46 p.m. UTC | #1 
On 2024-12-30 05:49, Tankut Baris Aktemur wrote:
> Use inheritance in the definition of tracepoint contexts.  This is a
> refactoring that aims to improve the code.  No behavior should be
> altered.
> ---
>  gdbserver/tracepoint.cc | 107 ++++++++++++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 59 deletions(-)
> 
> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index 81104b02c8321fcb4cb247a6166be10ba04aea8c..fc0586a5c5ace6edc1fc72d24511c6f36e97f759 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -1274,7 +1274,7 @@ static char *tracing_stop_note;
>  
>  /* Functions local to this file.  */
>  
> -/* Base "class" for tracepoint type specific data to be passed down to
> +/* Base class for tracepoint type specific data to be passed down to
>     collect_data_at_tracepoint.  */
>  struct tracepoint_hit_ctx
>  {
> @@ -1285,23 +1285,13 @@ struct tracepoint_hit_ctx
>  
>  /* Fast/jump tracepoint specific data to be passed down to
>     collect_data_at_tracepoint.  */
> -struct fast_tracepoint_ctx
> +struct fast_tracepoint_ctx : public tracepoint_hit_ctx
>  {
> -  struct tracepoint_hit_ctx base;
> -
> -  struct regcache regcache;
> -  int regcache_initted;
> -  unsigned char *regspace;
> -
> -  unsigned char *regs;
> -  struct tracepoint *tpoint;
> -};
> -
> -/* Static tracepoint specific data to be passed down to
> -   collect_data_at_tracepoint.  */
> -struct static_tracepoint_ctx
> -{
> -  struct tracepoint_hit_ctx base;
> +  fast_tracepoint_ctx (unsigned char *_regs)
> +    : regcache_initted (0), regs (_regs)

 - explicit
 - It's fine to name the parameter `regs` too
 - Initialize `regcache_initted` inline
 - I think that `tracepoint_hit_ctx` should have a constructor that
   takes the type, instead of each variant initializing the `type`
   member

> +  {
> +    type = fast_tracepoint;
> +  }
>  
>    /* The regcache corresponding to the registers state at the time of
>       the tracepoint hit.  Initialized lazily, from REGS.  */
> @@ -1314,25 +1304,41 @@ struct static_tracepoint_ctx
>    unsigned char *regspace;
>  
>    /* The register buffer as passed on by lttng/ust.  */
> -  struct registers *regs;

Something seems wrong, this field just disappears in the refactor.  It
is only used if HAVE_UST is defined, which never happens nowadays
because it used a version of lttng-ust that has been deprecated for a
loooong time (the 0.x series).  So everything in HAVE_UST just bitrots.
It might be possible to update all this code to use lttng-ust 2.x (1.x
never existed), but I don't think it's going to happen unless somebody
specifically asks for it.

I would suggest starting with removing support for UST from gdbserver
and rebasing you series, it will end up making this patch simpler.  If
we ever want to resurrect support for UST support and port to 2.x, we
can get the code from the git history.

> +  unsigned char *regs;
> +
> +  /* The GDB tracepoint matching the probed marker that was "hit".  */
> +  struct tracepoint *tpoint;
> +};
> +
> +/* Static tracepoint specific data to be passed down to
> +   collect_data_at_tracepoint.  */
> +struct static_tracepoint_ctx : public fast_tracepoint_ctx

I will have to dig deeper, but I'm not sure it conceptually makes sense
for static_tracepoint_ctx to inherit fast_tracepoint_ctx.  Are static
tracepoints always based on fast tracepoints?

(I have to stop here for now.)

Simon
diff mbox series

Patch

diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 81104b02c8321fcb4cb247a6166be10ba04aea8c..fc0586a5c5ace6edc1fc72d24511c6f36e97f759 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -1274,7 +1274,7 @@  static char *tracing_stop_note;
 
 /* Functions local to this file.  */
 
-/* Base "class" for tracepoint type specific data to be passed down to
+/* Base class for tracepoint type specific data to be passed down to
    collect_data_at_tracepoint.  */
 struct tracepoint_hit_ctx
 {
@@ -1285,23 +1285,13 @@  struct tracepoint_hit_ctx
 
 /* Fast/jump tracepoint specific data to be passed down to
    collect_data_at_tracepoint.  */
-struct fast_tracepoint_ctx
+struct fast_tracepoint_ctx : public tracepoint_hit_ctx
 {
-  struct tracepoint_hit_ctx base;
-
-  struct regcache regcache;
-  int regcache_initted;
-  unsigned char *regspace;
-
-  unsigned char *regs;
-  struct tracepoint *tpoint;
-};
-
-/* Static tracepoint specific data to be passed down to
-   collect_data_at_tracepoint.  */
-struct static_tracepoint_ctx
-{
-  struct tracepoint_hit_ctx base;
+  fast_tracepoint_ctx (unsigned char *_regs)
+    : regcache_initted (0), regs (_regs)
+  {
+    type = fast_tracepoint;
+  }
 
   /* The regcache corresponding to the registers state at the time of
      the tracepoint hit.  Initialized lazily, from REGS.  */
@@ -1314,25 +1304,41 @@  struct static_tracepoint_ctx
   unsigned char *regspace;
 
   /* The register buffer as passed on by lttng/ust.  */
-  struct registers *regs;
+  unsigned char *regs;
+
+  /* The GDB tracepoint matching the probed marker that was "hit".  */
+  struct tracepoint *tpoint;
+};
+
+/* Static tracepoint specific data to be passed down to
+   collect_data_at_tracepoint.  */
+struct static_tracepoint_ctx : public fast_tracepoint_ctx
+{
+  static_tracepoint_ctx (unsigned char *_regs,
+			 const char *_fmt, va_list *_args)
+    : fast_tracepoint_ctx (_regs), fmt (_fmt), args (_args)
+  {
+    type = static_tracepoint;
+  }
 
   /* The "printf" formatter and the args the user passed to the marker
      call.  We use this to be able to collect "static trace data"
      ($_sdata).  */
   const char *fmt;
   va_list *args;
-
-  /* The GDB tracepoint matching the probed marker that was "hit".  */
-  struct tracepoint *tpoint;
 };
 
 #else
 
 /* Static tracepoint specific data to be passed down to
    collect_data_at_tracepoint.  */
-struct trap_tracepoint_ctx
+struct trap_tracepoint_ctx : public tracepoint_hit_ctx
 {
-  struct tracepoint_hit_ctx base;
+  trap_tracepoint_ctx (struct regcache *_regcache)
+    : regcache (_regcache)
+  {
+    type = trap_tracepoint;
+  }
 
   struct regcache *regcache;
 };
@@ -4345,7 +4351,6 @@  tracepoint_finished_step (thread_info *tinfo, CORE_ADDR stop_pc)
   struct tracepoint *tpoint;
   struct wstep_state *wstep;
   struct wstep_state **wstep_link;
-  struct trap_tracepoint_ctx ctx;
 
   /* Pull in fast tracepoint trace frames from the inferior lib buffer into
      our buffer.  */
@@ -4375,8 +4380,7 @@  tracepoint_finished_step (thread_info *tinfo, CORE_ADDR stop_pc)
 	       target_pid_to_str (tinfo->id).c_str (),
 	       wstep->tp_number, paddress (wstep->tp_address));
 
-  ctx.base.type = trap_tracepoint;
-  ctx.regcache = get_thread_regcache (tinfo);
+  trap_tracepoint_ctx ctx (get_thread_regcache (tinfo));
 
   while (wstep != NULL)
     {
@@ -4398,8 +4402,7 @@  tracepoint_finished_step (thread_info *tinfo, CORE_ADDR stop_pc)
       ++wstep->current_step;
 
       /* Collect data.  */
-      collect_data_at_step ((struct tracepoint_hit_ctx *) &ctx,
-			    stop_pc, tpoint, wstep->current_step);
+      collect_data_at_step (&ctx, stop_pc, tpoint, wstep->current_step);
 
       if (wstep->current_step >= tpoint->step_count)
 	{
@@ -4530,14 +4533,12 @@  tracepoint_was_hit (thread_info *tinfo, CORE_ADDR stop_pc)
 {
   struct tracepoint *tpoint;
   int ret = 0;
-  struct trap_tracepoint_ctx ctx;
 
   /* Not tracing, don't handle.  */
   if (!tracing)
     return 0;
 
-  ctx.base.type = trap_tracepoint;
-  ctx.regcache = get_thread_regcache (tinfo);
+  trap_tracepoint_ctx ctx (get_thread_regcache (tinfo));
 
   for (tpoint = tracepoints; tpoint; tpoint = tpoint->next)
     {
@@ -4556,10 +4557,8 @@  tracepoint_was_hit (thread_info *tinfo, CORE_ADDR stop_pc)
 
 	  /* Test the condition if present, and collect if true.  */
 	  if (!tpoint->cond
-	      || (condition_true_at_tracepoint
-		  ((struct tracepoint_hit_ctx *) &ctx, tpoint)))
-	    collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-					stop_pc, tpoint);
+	      || (condition_true_at_tracepoint (&ctx, tpoint)))
+	    collect_data_at_tracepoint (&ctx, stop_pc, tpoint);
 
 	  if (stopping_tracepoint
 	      || trace_buffer_is_full
@@ -4696,7 +4695,8 @@  get_context_regcache (struct tracepoint_hit_ctx *ctx)
 
   if (ctx->type == fast_tracepoint)
     {
-      struct fast_tracepoint_ctx *fctx = (struct fast_tracepoint_ctx *) ctx;
+      auto fctx = static_cast<fast_tracepoint_ctx *> (ctx);
+
       if (!fctx->regcache_initted)
 	{
 	  fctx->regcache_initted = 1;
@@ -4709,8 +4709,7 @@  get_context_regcache (struct tracepoint_hit_ctx *ctx)
 #ifdef HAVE_UST
   if (ctx->type == static_tracepoint)
     {
-      struct static_tracepoint_ctx *sctx
-	= (struct static_tracepoint_ctx *) ctx;
+      auto sctx = static_cast <static_tracepoint_ctx *> (ctx);
 
       if (!sctx->regcache_initted)
 	{
@@ -4730,7 +4729,7 @@  get_context_regcache (struct tracepoint_hit_ctx *ctx)
 #else
   if (ctx->type == trap_tracepoint)
     {
-      struct trap_tracepoint_ctx *tctx = (struct trap_tracepoint_ctx *) ctx;
+      auto tctx = static_cast<trap_tracepoint_ctx *> (ctx);
       regcache = tctx->regcache;
     }
 #endif
@@ -4881,7 +4880,7 @@  condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 #ifdef IN_PROCESS_AGENT
   if (tpoint->compiled_cond)
     {
-      struct fast_tracepoint_ctx *fctx = (struct fast_tracepoint_ctx *) ctx;
+      auto fctx = static_cast<fast_tracepoint_ctx *> (ctx);
       err = ((condfn) (uintptr_t) (tpoint->compiled_cond)) (fctx->regs, &value);
     }
   else
@@ -5763,7 +5762,6 @@  IP_AGENT_EXPORT_FUNC void gdb_collect (struct tracepoint *tpoint,
 IP_AGENT_EXPORT_FUNC void
 gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
 {
-  struct fast_tracepoint_ctx ctx;
   const struct target_desc *ipa_tdesc;
 
   /* Don't do anything until the trace run is completely set up.  */
@@ -5771,9 +5769,8 @@  gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
     return;
 
   ipa_tdesc = get_ipa_tdesc (ipa_tdesc_idx);
-  ctx.base.type = fast_tracepoint;
-  ctx.regs = regs;
-  ctx.regcache_initted = 0;
+  fast_tracepoint_ctx ctx (regs);
+
   /* Wrap the regblock in a register cache (in the stack, we don't
      want to malloc here).  */
   ctx.regspace = (unsigned char *) alloca (ipa_tdesc->registers_size);
@@ -5797,11 +5794,10 @@  gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
 
       /* Test the condition if present, and collect if true.  */
       if (ctx.tpoint->cond == NULL
-	  || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-					   ctx.tpoint))
+	  || condition_true_at_tracepoint (&ctx, ctx.tpoint))
 	{
-	  collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-				      ctx.tpoint->address, ctx.tpoint);
+	  collect_data_at_tracepoint (&ctx, ctx.tpoint->address,
+				      ctx.tpoint);
 
 	  /* Note that this will cause original insns to be written back
 	     to where we jumped from, but that's OK because we're jumping
@@ -6651,7 +6647,6 @@  gdb_probe (const struct marker *mdata, void *probe_private,
 	   const char *fmt, va_list *args)
 {
   struct tracepoint *tpoint;
-  struct static_tracepoint_ctx ctx;
   const struct target_desc *ipa_tdesc;
 
   /* Don't do anything until the trace run is completely set up.  */
@@ -6662,11 +6657,7 @@  gdb_probe (const struct marker *mdata, void *probe_private,
     }
 
   ipa_tdesc = get_ipa_tdesc (ipa_tdesc_idx);
-  ctx.base.type = static_tracepoint;
-  ctx.regcache_initted = 0;
-  ctx.regs = regs;
-  ctx.fmt = fmt;
-  ctx.args = args;
+  static_tracepoint_ctx ctx (regs, fmt, args)
 
   /* Wrap the regblock in a register cache (in the stack, we don't
      want to malloc here).  */
@@ -6702,11 +6693,9 @@  gdb_probe (const struct marker *mdata, void *probe_private,
 
   /* Test the condition if present, and collect if true.  */
   if (tpoint->cond == NULL
-      || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-				       tpoint))
+      || condition_true_at_tracepoint (&ctx, tpoint))
     {
-      collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-				  tpoint->address, tpoint);
+      collect_data_at_tracepoint (&ctx, tpoint->address, tpoint);
 
       if (stopping_tracepoint
 	  || trace_buffer_is_full
@@ -6737,7 +6726,7 @@  static void
 collect_ust_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 				struct traceframe *tframe)
 {
-  struct static_tracepoint_ctx *umd = (struct static_tracepoint_ctx *) ctx;
+  auto umd = static_cast<static_tracepoint_ctx *> (ctx);
   unsigned char *bufspace;
   int size;
   va_list copy;