diff mbox series

[v8,04/10] btrace: Handle stepping and goto for auxiliary instructions.

Message ID 20230321154626.448816-5-felix.willgerodt@intel.com
State New
Headers
Series Extensions for PTWRITE |

Commit Message

Willgerodt, Felix March 21, 2023, 3:46 p.m. UTC 
  Print the auxiliary data when stepping. Don't allow to goto an auxiliary
instruction.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/record-btrace.c | 67 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 13 deletions(-)

Comments

Simon Marchi March 24, 2023, 2:09 p.m. UTC | #1 
On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> Print the auxiliary data when stepping. Don't allow to goto an auxiliary
> instruction.
> 
> This patch is in preparation for the new ptwrite feature, which is based on
> auxiliary instructions.

Hi Felix,

Just a few minor comments.  No need to repost just for that.

> @@ -2388,12 +2392,27 @@ record_btrace_single_step_forward (struct thread_info *tp)
>  	 of the execution history.  */
>        steps = btrace_insn_next (replay, 1);
>        if (steps == 0)
> +        {
> +          *replay = start;
> +          return btrace_step_no_history ();
> +        }

These lines just (wrongfully) replaces the indentation with all spaces,
when in reality these lines don't need to be touched.

> +
> +      const struct btrace_insn *insn = btrace_insn_get (replay);
> +      if (insn == nullptr)
> +	continue;
> +
> +      /* If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
> +	 data and skip the instruction.  */
> +      if (insn->iclass == BTRACE_INSN_AUX)
>  	{
> -	  *replay = start;
> -	  return btrace_step_no_history ();
> +	  gdb_printf ("[%s]\n",
> +		      btinfo->aux_data.at (insn->aux_data_index).c_str ());

I was a bit thrown off by the use of std::vector::at :P (I think it was
used in the previous patch too).  Did you use it specifically for the
behavior when the access is out of bounds?  If not, using operator[]
would probably be better (and more usual).

> @@ -2859,25 +2895,30 @@ record_btrace_target::goto_record_end ()
>  /* The goto_record method of target record-btrace.  */
>  
>  void
> -record_btrace_target::goto_record (ULONGEST insn)
> +record_btrace_target::goto_record (ULONGEST insn_number)
>  {
>    struct thread_info *tp;
>    struct btrace_insn_iterator it;
>    unsigned int number;
>    int found;
>  
> -  number = insn;
> +  number = insn_number;
>  
>    /* Check for wrap-arounds.  */
> -  if (number != insn)
> +  if (number != insn_number)
>      error (_("Instruction number out of range."));
>  
>    tp = require_btrace_thread ();
>  
>    found = btrace_find_insn_by_number (&it, &tp->btrace, number);
>  
> -  /* Check if the instruction could not be found or is a gap.  */
> -  if (found == 0 || btrace_insn_get (&it) == NULL)
> +  /* Check if the instruction could not be found or is a gap or an
> +     auxilliary instruction.  */
> +  if (found == 0)
> +    error (_("No such instruction."));
> +
> +  const struct btrace_insn *insn = btrace_insn_get (&it);
> +  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
>      error (_("No such instruction."));

Just wondering, would it be useful for the user to print a more specific
error message ("Can't go to an auxiliary instruction") if they try to
goto an aux instruction?

Simon
Terekhov, Mikhail via Gdb-patches March 31, 2023, 10:58 a.m. UTC | #2 
> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 15:10
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary
> instructions.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > Print the auxiliary data when stepping. Don't allow to goto an auxiliary
> > instruction.
> >
> > This patch is in preparation for the new ptwrite feature, which is based on
> > auxiliary instructions.
> 
> Hi Felix,
> 
> Just a few minor comments.  No need to repost just for that.
> 
> > @@ -2388,12 +2392,27 @@ record_btrace_single_step_forward (struct
> thread_info *tp)
> >  	 of the execution history.  */
> >        steps = btrace_insn_next (replay, 1);
> >        if (steps == 0)
> > +        {
> > +          *replay = start;
> > +          return btrace_step_no_history ();
> > +        }
> 
> These lines just (wrongfully) replaces the indentation with all spaces,
> when in reality these lines don't need to be touched.

Will fix that! Thanks.

> > +
> > +      const struct btrace_insn *insn = btrace_insn_get (replay);
> > +      if (insn == nullptr)
> > +	continue;
> > +
> > +      /* If we're stepping a BTRACE_INSN_AUX instruction, print the
> auxiliary
> > +	 data and skip the instruction.  */
> > +      if (insn->iclass == BTRACE_INSN_AUX)
> >  	{
> > -	  *replay = start;
> > -	  return btrace_step_no_history ();
> > +	  gdb_printf ("[%s]\n",
> > +		      btinfo->aux_data.at (insn->aux_data_index).c_str ());
> 
> I was a bit thrown off by the use of std::vector::at :P (I think it was
> used in the previous patch too).  Did you use it specifically for the
> behavior when the access is out of bounds?  If not, using operator[]
> would probably be better (and more usual).

Yes this was intentionally. I think I added it based on a previous review.

> > @@ -2859,25 +2895,30 @@ record_btrace_target::goto_record_end ()
> >  /* The goto_record method of target record-btrace.  */
> >
> >  void
> > -record_btrace_target::goto_record (ULONGEST insn)
> > +record_btrace_target::goto_record (ULONGEST insn_number)
> >  {
> >    struct thread_info *tp;
> >    struct btrace_insn_iterator it;
> >    unsigned int number;
> >    int found;
> >
> > -  number = insn;
> > +  number = insn_number;
> >
> >    /* Check for wrap-arounds.  */
> > -  if (number != insn)
> > +  if (number != insn_number)
> >      error (_("Instruction number out of range."));
> >
> >    tp = require_btrace_thread ();
> >
> >    found = btrace_find_insn_by_number (&it, &tp->btrace, number);
> >
> > -  /* Check if the instruction could not be found or is a gap.  */
> > -  if (found == 0 || btrace_insn_get (&it) == NULL)
> > +  /* Check if the instruction could not be found or is a gap or an
> > +     auxilliary instruction.  */
> > +  if (found == 0)
> > +    error (_("No such instruction."));
> > +
> > +  const struct btrace_insn *insn = btrace_insn_get (&it);
> > +  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
> >      error (_("No such instruction."));
> 
> Just wondering, would it be useful for the user to print a more specific
> error message ("Can't go to an auxiliary instruction") if they try to
> goto an aux instruction?
> 

I like the suggestion. I will split it up to keep the current message for gaps:

  const struct btrace_insn *insn = btrace_insn_get (&it);
  if (insn == NULL)
    error (_("No such instruction."));
  if (insn->iclass == BTRACE_INSN_AUX)
    error (_("Can't go to an auxiliary instruction."));

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox series

Patch

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 9ec7eb1b415..22687e889ec 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2378,9 +2378,13 @@  record_btrace_single_step_forward (struct thread_info *tp)
     return btrace_step_stopped ();
 
   /* Skip gaps during replay.  If we end up at a gap (at the end of the trace),
-     jump back to the instruction at which we started.  */
+     jump back to the instruction at which we started.  If we're stepping a
+     BTRACE_INSN_AUX instruction, print the auxiliary data and skip the
+     instruction.  */
+
   start = *replay;
-  do
+
+  for (;;)
     {
       unsigned int steps;
 
@@ -2388,12 +2392,27 @@  record_btrace_single_step_forward (struct thread_info *tp)
 	 of the execution history.  */
       steps = btrace_insn_next (replay, 1);
       if (steps == 0)
+        {
+          *replay = start;
+          return btrace_step_no_history ();
+        }
+
+      const struct btrace_insn *insn = btrace_insn_get (replay);
+      if (insn == nullptr)
+	continue;
+
+      /* If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
+	 data and skip the instruction.  */
+      if (insn->iclass == BTRACE_INSN_AUX)
 	{
-	  *replay = start;
-	  return btrace_step_no_history ();
+	  gdb_printf ("[%s]\n",
+		      btinfo->aux_data.at (insn->aux_data_index).c_str ());
+	  continue;
 	}
+
+      /* We have an instruction, we are done.  */
+      break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Determine the end of the instruction trace.  */
   btrace_insn_end (&end, btinfo);
@@ -2424,9 +2443,12 @@  record_btrace_single_step_backward (struct thread_info *tp)
 
   /* If we can't step any further, we reached the end of the history.
      Skip gaps during replay.  If we end up at a gap (at the beginning of
-     the trace), jump back to the instruction at which we started.  */
+     the trace), jump back to the instruction at which we started.
+     If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
+     data and skip the instruction.  */
   start = *replay;
-  do
+
+  for (;;)
     {
       unsigned int steps;
 
@@ -2436,8 +2458,22 @@  record_btrace_single_step_backward (struct thread_info *tp)
 	  *replay = start;
 	  return btrace_step_no_history ();
 	}
+
+      const struct btrace_insn *insn = btrace_insn_get (replay);
+      if (insn == nullptr)
+	continue;
+
+      /* Check if we're stepping a BTRACE_INSN_AUX instruction and skip it.  */
+      if (insn->iclass == BTRACE_INSN_AUX)
+	{
+	  gdb_printf ("[%s]\n",
+		      btinfo->aux_data.at (insn->aux_data_index).c_str ());
+	  continue;
+	}
+
+      /* We have an instruction, we are done.  */
+      break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Check if we're stepping a breakpoint.
 
@@ -2859,25 +2895,30 @@  record_btrace_target::goto_record_end ()
 /* The goto_record method of target record-btrace.  */
 
 void
-record_btrace_target::goto_record (ULONGEST insn)
+record_btrace_target::goto_record (ULONGEST insn_number)
 {
   struct thread_info *tp;
   struct btrace_insn_iterator it;
   unsigned int number;
   int found;
 
-  number = insn;
+  number = insn_number;
 
   /* Check for wrap-arounds.  */
-  if (number != insn)
+  if (number != insn_number)
     error (_("Instruction number out of range."));
 
   tp = require_btrace_thread ();
 
   found = btrace_find_insn_by_number (&it, &tp->btrace, number);
 
-  /* Check if the instruction could not be found or is a gap.  */
-  if (found == 0 || btrace_insn_get (&it) == NULL)
+  /* Check if the instruction could not be found or is a gap or an
+     auxilliary instruction.  */
+  if (found == 0)
+    error (_("No such instruction."));
+
+  const struct btrace_insn *insn = btrace_insn_get (&it);
+  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
     error (_("No such instruction."));
 
   record_btrace_set_replay (tp, &it);