Re: [RFA] Reverse debugging, part 2/3: core interface

[Daniel Jacobowitz <drow at false dot org>]

On Mon, Apr 17, 2006 at 04:42:02PM -0700, Michael Snyder wrote:
> @@ -1371,6 +1384,62 @@
>        do_cleanups (old_chain);
>      }
>  }
> +
> +static void
> +finish_backwards (struct symbol *function)
> +{
> +  struct symtab_and_line sal;
> +  struct breakpoint *breakpoint;
> +  struct cleanup *old_chain;
> +  CORE_ADDR func_addr;
> +
> +  if (find_pc_partial_function (get_frame_pc (get_current_frame ()),
> +				NULL, &func_addr, NULL) == 0)
> +    internal_error (__FILE__, __LINE__, 
> +		    "Finish: couldn't find function.");
> +
> +  sal = find_pc_line (func_addr, 0);
> +
> +  /* Let's cheat and not worry about async until later.  */

:-(

What problems does it present?  Need to use completions?

Async operation has been getting a bit musty.  I don't think it's fair
to insist you straighten that out before this goes in.  But, it would
be nice to issue an error here, instead of doing something quite
possibly wrong.

> +
> +  /* We don't need a return value.  */
> +  proceed_to_finish = 0;
> +  /* Special case: if we're sitting at the function entry point, 
> +     then all we need to do is take a reverse singlestep.  We
> +     don't need to set a breakpoint, and indeed it would do us
> +     no good to do so.
> +
> +     Note that this can only happen at frame #0, since there's
> +     no way that a function up the stack can have a return address
> +     that's equal to its entry point.  */
> +
> +  if (sal.pc != read_pc ())

Above you used get_frame_pc, here you use read_pc.  More importantly...

> +    {
> +      /* Set breakpoint and continue.  */
> +      breakpoint = 
> +	set_momentary_breakpoint (sal, 
> +				  get_frame_id (get_selected_frame (NULL)),
> +				  bp_breakpoint);

...above you used get_current_frame, here you used get_selected_frame.
Which is it?  I think that it should be the selected frame in all three
cases, by analogy with finish, and there should probably be a similar
error for cases without a caller.

> +  if (bpstat_find_breakpoint (stop_bpstat, breakpoint) != NULL)
> +    {
> +      /* If in fact we hit the step-resume breakpoint (and not
> +	 some other breakpoint), then we're almost there -- 
> +	 we just need to back up by one more single-step.  */
> +      /* (Kludgy way of letting wait_for_inferior know...) */
> +      step_range_start = step_range_end = 1;
> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
> +    }

Does this do the right thing if there was already an active breakpoint
at the function entry point?

> +	if (stop_pc == ecs->stop_func_start &&
> +	    target_get_execution_direction () == EXEC_REVERSE)

&& at the beginning of lines.  (Same applies to '='; I saw that
somewhere else in these patches, don't remember where now.)

> +	  {
> +	    /* We are stepping over a function call in reverse, and
> +	       just hit the step-resume breakpoint at the start
> +	       address of the function.  Go back to single-stepping,
> +	       which should take us back to the function call.  */
> +	    ecs->another_trap = 1;
> +	    keep_going (ecs);
> +	    return;
> +	  }
>  	break;

If we've hit a step resume breakpoint, this says, then delete it,
continue, and expect another trap.  Where's the other trap come from,
i.e. what makes us go back to stepping?  I'm sure this bit works,
I just can't see why.

> @@ -2312,7 +2331,22 @@
>  	 fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
>  			    paddr_nz (step_range_start),
>  			    paddr_nz (step_range_end));
> -      keep_going (ecs);
> +
> +      /* When stepping backward, stop at beginning of line range
> +	 (unles it's the function entry point, in which case 
> +	 keep going back to the call point).  */
> +      if (stop_pc == step_range_start &&
> +	  stop_pc != ecs->stop_func_start &&
> +	  target_get_execution_direction () == EXEC_REVERSE)
> +	{
> +	  stop_step = 1;
> +	  print_stop_reason (END_STEPPING_RANGE, 0);
> +	  stop_stepping (ecs);
> +	}
> +      else
> +	{
> +	  keep_going (ecs);
> +	}
>        return;
>      }
>  

Why do we automatically step out of functions, again?  I know we
discussed this - is it because this is the equivalent of prologue
skipping, in reverse?  A better comment here might be nice.

This looks like it could be nasty with recursion; we might step out of
a bunch of recursive calls for a tiny function.  But it looks like we
have the same issue in both directions, so let's ignore for now.

> @@ -2393,10 +2427,33 @@
>  
>        if (step_over_calls == STEP_OVER_ALL)
>  	{
> -	  /* We're doing a "next", set a breakpoint at callee's return
> -	     address (the address at which the caller will
> -	     resume).  */
> -	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
> +	  /* We're doing a "next".
> +
> +	     Normal (forward) execution: set a breakpoint at the
> +	     callee's return address (the address at which the caller
> +	     will resume).
> +
> +	     Reverse (backward) execution.  set the step-resume

Typo; probably "execution: set..."

> +	     breakpoint at the start of the function that we just
> +	     stepped into (backwards), and continue to there.  When we
> +	     get there, we'll need to single-step back to the
> +	     caller.  */
> +
> +	  if (target_get_execution_direction () == EXEC_REVERSE)
> +	    {
> +	      /* FIXME: I'm not sure if we've handled the frame for
> +		 recursion.  */
> +
> +	      struct symtab_and_line sr_sal;
> +	      init_sal (&sr_sal);
> +	      sr_sal.pc = ecs->stop_func_start;
> +	      insert_step_resume_breakpoint_at_sal (sr_sal, null_frame_id);
> +	    }

This does seem like a reasonably important FIXME.  Any reason it
shouldn't be get_current_frame()?  Same in the other occurance of this
FIXME below.

> @@ -2585,17 +2656,38 @@
>  
>    if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)

This (pre-existing) check is pretty bogus... the epilogue doesn't need
to be at the end of the function any more.

>      {
> -      /* If this is the last line of the function, don't keep stepping
> -         (it would probably step us out of the function).
> -         This is particularly necessary for a one-line function,
> -         in which after skipping the prologue we better stop even though
> -         we will be in mid-line.  */
> -      if (debug_infrun)
> -	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped to a different function\n");
> -      stop_step = 1;
> -      print_stop_reason (END_STEPPING_RANGE, 0);
> -      stop_stepping (ecs);
> -      return;

So this makes some sense for the one-line function case...

> +      else
> +	{
> +	  /* If we stepped backward into the last line of a function,
> +	     then we've presumably stepped thru a return.  We want to
> +	     keep stepping backward until we reach the beginning of
> +	     the new line.  */
> +	  step_range_start = ecs->sal.pc;
> +	  step_range_end = ecs->sal.end;
> +	  step_frame_id = get_frame_id (get_current_frame ());
> +	  ecs->current_line = ecs->sal.line;
> +	  ecs->current_symtab = ecs->sal.symtab;
> +	  /* Adjust for prologue, in case of a one-line function.  */
> +	  if (in_prologue (step_range_start, ecs->stop_func_start))
> +	    step_range_start = SKIP_PROLOGUE (step_range_start);
> +	  keep_going (ecs);
> +	  return;
> +	}
>      }
>    step_range_start = ecs->sal.pc;
>    step_range_end = ecs->sal.end;

... but I'm much more skeptical of this.  I don't think it does what
you want it to; you could arrive backwards in a function at many
points.  Can't you check for "stepped backwards into a new function"
independently of "the last line"?  I'm thinking step_frame_id
here.  When that changes, either we've stepped backwards into a
function called by this one (which will show up as a function call in
the normal way), or backwards into the caller of this one.

I'm not entirely sure we want to step to the beginning of the line in
this case either.  A better analogue might be stepping forwards out of
a function; we don't continue to the next line of the calling function.
This is especially useful on lines with multiple function calls:

  foo (bar(), baz());

step takes you into bar, out of bar, into baz, out of baz, into foo.
Should reverse stepping do the same, and take you out of foo (stop),
into baz (stop), out of baz (stop), et cetera?

> @@ -2658,6 +2750,28 @@
>    if (s && s->language != language_asm)
>      ecs->stop_func_start = SKIP_PROLOGUE (ecs->stop_func_start);
>  
> +  if (target_get_execution_direction () == EXEC_REVERSE)
> +    {
> +      ecs->sal = find_pc_line (stop_pc, 0);
> +
> +      /* OK, we're just gonna keep stepping here.  */
> +      if (ecs->sal.pc == stop_pc)
> +	{
> +	  /* We're there already.  Just stop stepping now.  */
> +	  stop_step = 1;
> +	  print_stop_reason (END_STEPPING_RANGE, 0);
> +	  stop_stepping (ecs);
> +	  return;
> +	}
> +      /* Else just reset the step range and keep going.
> +	 No step-resume breakpoint, they don't work for
> +	 epilogues, which can have multiple entry paths.  */
> +      step_range_start = ecs->sal.pc;
> +      step_range_end   = ecs->sal.end;
> +      keep_going (ecs);
> +      return;
> +    }

[In step_into_function] Won't this stepping range move us all the way
through that line and to the previous line?

-- 
Daniel Jacobowitz
CodeSourcery