[v8,04/10] btrace: Handle stepping and goto for auxiliary instructions.
Commit Message
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
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
> -----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
@@ -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);