RE: [PATCH 04/10] btrace: Handle stepping and goto for auxiliary instructions.

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

I should have fixed all comments.

Thanks,
Felix

On 6/4/19 2:35 PM, Metzger, Markus T wrote:

Hello Felix,

> @@ -2378,13 +2380,26 @@ record_btrace_single_step_forward (struct 
> thread_info
> *tp)
>    if (record_btrace_replay_at_breakpoint (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.  */
>    start = *replay;

Please preserve the order of comment and code.

> +
> +  /* 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.  If we're stepping a
> +     BTRACE_INSN_AUX instruction, print the ptwrite string and skip 
> + the

Should this say 'print the aux data'?

> +     instruction.  */
>    do
>      {
>        unsigned int steps;
> 
> +      /* If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
> +	 data and skip the instruction.  If we are at the end of the trace, jump
> +	 back to the instruction at which we started.  */
> +      bfun = &replay->btinfo->functions[replay->call_index];
> +      next_insn = bfun->insn[replay->insn_index + 1];

The below btrace_insn_next () already advances REPLAY to the next instruction.  You may use btrace_insn_next (replay) to get that instruction.

> +
> +      if (next_insn.iclass == BTRACE_INSN_AUX)
> +	printf_unfiltered (
> +	    "[%s]\n", btinfo->aux_data[next_insn.aux_data_index].c_str ());
> +
>        /* We will bail out here if we continue stepping after reaching the end
>  	 of the execution history.  */
>        steps = btrace_insn_next (replay, 1); @@ -2394,7 +2409,8 @@ 
> record_btrace_single_step_forward (struct thread_info
> *tp)
>  	  return btrace_step_no_history ();
>  	}
>      }
> -  while (btrace_insn_get (replay) == NULL);
> +  while (btrace_insn_get (replay) == NULL
> +	 || next_insn.iclass == BTRACE_INSN_AUX);

Since we now call btrace_insn_get (replay) inside the loop, we should rewrite this.

> @@ -2452,6 +2470,22 @@ record_btrace_single_step_backward (struct 
> thread_info *tp)
>    if (record_btrace_replay_at_breakpoint (tp))
>      return btrace_step_stopped ();
> 
> +  /* Check if we're stepping a BTRACE_INSN_AUX instruction and skip 
> + it.  */  bfun = &replay->btinfo->functions[replay->call_index];
> +  prev_insn = bfun->insn[replay->insn_index];

Same here.

> +
> +  if (prev_insn.iclass == BTRACE_INSN_AUX)
> +    {
> +      printf_unfiltered ("[%s]\n",
> +			 btinfo->aux_data[prev_insn.aux_data_index].c_str ());
> +      unsigned int steps = btrace_insn_prev (replay, 1);
> +      if (steps == 0)
> +	{
> +	  *replay = start;
> +	  return btrace_step_no_history ();
> +	}
> +    }
> +
>    return btrace_step_spurious ();
>  }
> 
> @@ -2853,25 +2887,27 @@ 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);
> +  const struct btrace_insn *insn = btrace_insn_get (&it);

You need to check FOUND before dereferencing IT.

> 
> -  /* 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) || (insn == NULL) || (insn->iclass == 
> + BTRACE_INSN_AUX))
>      error (_("No such instruction."));
> 
>    record_btrace_set_replay (tp, &it);
> --
> 2.20.1

Thanks,
Markus.
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