[RFA] Reverse Debugging, 4/5

[RFA] Reverse Debugging, 4/5

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: infcmd.txt --]
[-- Type: text/plain, Size: 5906 bytes --]


This patch (#4), adds special handling for the "finish" command
when executing in reverse.  The new code can't actually be 
invoked until patch #5 is added, and so has no user visible
effect.

Tested under RHEL native with no change in results.

2008-09-30  Michael Snyder  <msnyder@vmware.com>

	* breakpoint.h (breakpoint_silence): Export.
	* breakpoint.c (breakpoint_silence): New function.
	* infcmd.c (finish_command): Check for reverse exec direction.
	(finish_backward): New function, handle finish cmd in reverse.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.352
diff -u -p -r1.352 breakpoint.c
--- breakpoint.c	16 Sep 2008 18:55:01 -0000	1.352
+++ breakpoint.c	30 Sep 2008 23:53:41 -0000
@@ -7741,6 +7741,13 @@ breakpoint_clear_ignore_counts (void)
     b->ignore_count = 0;
 }
 
+void
+breakpoint_silence (struct breakpoint *b)
+{
+  /* Silence the breakpoint.  */
+  b->silent = 1;
+}
+
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.79
diff -u -p -r1.79 breakpoint.h
--- breakpoint.h	22 Sep 2008 15:26:53 -0000	1.79
+++ breakpoint.h	30 Sep 2008 23:53:41 -0000
@@ -884,4 +884,7 @@ extern int breakpoints_always_inserted_m
    in our opinion won't ever trigger.  */
 extern void breakpoint_retire_moribund (void);
 
+/* Tell a breakpoint to be quiet.  */
+extern void breakpoint_silence (struct breakpoint *);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.212
diff -u -p -r1.212 infcmd.c
--- infcmd.c	22 Sep 2008 15:20:08 -0000	1.212
+++ infcmd.c	30 Sep 2008 23:53:41 -0000
@@ -1369,6 +1369,8 @@ finish_command_continuation_free_arg (vo
 /* "finish": Set a temporary breakpoint at the place the selected
    frame will return to, then continue.  */
 
+static void finish_backwards (struct symbol *, struct thread_info *);
+
 static void
 finish_command (char *arg, int from_tty)
 {
@@ -1412,13 +1414,6 @@ finish_command (char *arg, int from_tty)
 
   clear_proceed_status ();
 
-  sal = find_pc_line (get_frame_pc (frame), 0);
-  sal.pc = get_frame_pc (frame);
-
-  breakpoint = set_momentary_breakpoint (sal, get_frame_id (frame), bp_finish);
-
-  old_chain = make_cleanup_delete_breakpoint (breakpoint);
-
   /* Find the function we will return from.  */
 
   function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
@@ -1427,10 +1422,29 @@ finish_command (char *arg, int from_tty)
      source.  */
   if (from_tty)
     {
-      printf_filtered (_("Run till exit from "));
+      if (target_get_execution_direction () == EXEC_REVERSE)
+	printf_filtered ("Run back to call of ");
+      else
+	printf_filtered ("Run till exit from ");
+
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
     }
 
+  if (target_get_execution_direction () == EXEC_REVERSE)
+    {
+      /* Split off at this point.  */
+      finish_backwards (function, tp);
+      return;
+    }
+
+  sal = find_pc_line (get_frame_pc (frame), 0);
+  sal.pc = get_frame_pc (frame);
+
+  breakpoint = set_momentary_breakpoint (sal, get_frame_id (frame), 
+					 bp_finish);
+
+  old_chain = make_cleanup_delete_breakpoint (breakpoint);
+
   tp->proceed_to_finish = 1;	/* We want stop_registers, please...  */
   make_cleanup_restore_integer (&suppress_stop_observer);
   suppress_stop_observer = 1;
@@ -1514,6 +1528,66 @@ It stopped at a breakpoint that has sinc
 Type \"info stack\" or \"info registers\" for more information.\n"));
     }
 }
+
+static void
+finish_backwards (struct symbol *function, struct thread_info *tp)
+{
+  struct symtab_and_line sal;
+  struct breakpoint *breakpoint;
+  struct cleanup *old_chain;
+  CORE_ADDR func_addr;
+  int back_up;
+
+  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);
+
+  /* TODO: Let's not worry about async until later.  */
+
+  /* We don't need a return value.  */
+  tp->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 ())
+    {
+      /* Set breakpoint and continue.  */
+      breakpoint =
+	set_momentary_breakpoint (sal,
+				  get_frame_id (get_selected_frame (NULL)),
+				  bp_breakpoint);
+      /* Tell the breakpoint to keep quiet.  We won't be done
+         until we've done another reverse single-step.  */
+      breakpoint_silence (breakpoint);
+      old_chain = make_cleanup_delete_breakpoint (breakpoint);
+      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
+      /* We will be stopped when proceed returns.  */
+      back_up = bpstat_find_breakpoint (tp->stop_bpstat, breakpoint) != NULL;
+      do_cleanups (old_chain);
+    }
+  else
+    back_up = 1;
+  if (back_up)
+    {
+      /* 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...) */
+      tp->step_range_start = tp->step_range_end = 1;
+      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
+    }
+  return;
+}
+
 \f
 static void
 environment_info (char *var, int from_tty)

Re: [RFA] Reverse Debugging, 4/5

[Pedro Alves]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain;   charset="utf-8", Size: 3815 bytes --]

On Wednesday 01 October 2008 20:19:28, Michael Snyder wrote:
>  
> +static void finish_backwards (struct symbol *, struct thread_info *);
> +

Minor nit.  If you put the new function here, you don't
need the forward declaration.

>    function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
> @@ -1427,10 +1422,29 @@ finish_command (char *arg, int from_tty)
>       source.  */
>    if (from_tty)
>      {
> -      printf_filtered (_("Run till exit from "));
> +      if (target_get_execution_direction () == EXEC_REVERSE)
> +       printf_filtered ("Run back to call of ");
> +      else
> +       printf_filtered ("Run till exit from ");
> +
>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
>      }


i18n.

> +
> +static void
> +finish_backwards (struct symbol *function, struct thread_info *tp)
> +{
> +  struct symtab_and_line sal;
> +  struct breakpoint *breakpoint;
> +  struct cleanup *old_chain;
> +  CORE_ADDR func_addr;
> +  int back_up;
> +
> +  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.");

Can't this happen in some circunstances, like the user hitting
finish after stepi through
no-debug-info-at-all-not-even-minimal-symbols code?

> +
> +  sal = find_pc_line (func_addr, 0);
> +
> +  /* TODO: Let's not worry about async until later.  */

If you don't want to think about it now, could you error out
in the async+reverse case ?

> +
> +  /* We don't need a return value.  */
> +  tp->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 ())
> +    {
> +      /* Set breakpoint and continue.  */
> +      breakpoint =
> +       set_momentary_breakpoint (sal,
> +                                 get_frame_id (get_selected_frame (NULL)),
> +                                 bp_breakpoint);
> +      /* Tell the breakpoint to keep quiet.  We won't be done
> +         until we've done another reverse single-step.  */
> +      breakpoint_silence (breakpoint);
> +      old_chain = make_cleanup_delete_breakpoint (breakpoint);
> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
> +      /* We will be stopped when proceed returns.  */
> +      back_up = bpstat_find_breakpoint (tp->stop_bpstat, breakpoint) != NULL;
> +      do_cleanups (old_chain);
> +    }
> +  else
> +    back_up = 1;
> +  if (back_up)
> +    {
> +      /* 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...) */
> +      tp->step_range_start = tp->step_range_end = 1;
> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);

Would calling step_1 or step_once instead work here?  It would
probably avoid the kludge.

I'm no reverse guru, but your comments made sense to me.


> +    }
> +  return;
> +}
> +
>  \f
>  static void
>  environment_info (char *var, int from_tty)



-- 
Pedro Alves
\x16º&Öéj×!zÊÞ¶êççÝ9îX¬µªÜ†\a[¥«\…ë

Re: [RFA] Reverse Debugging, 4/5

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 01 October 2008 20:19:28, Michael Snyder wrote:
>> +static void finish_backwards (struct symbol *, struct thread_info *);
>> +
> 
> Minor nit.  If you put the new function here, you don't
> need the forward declaration.

OK.

>>    function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
>> @@ -1427,10 +1422,29 @@ finish_command (char *arg, int from_tty)
>>       source.  */
>>    if (from_tty)
>>      {
>> -      printf_filtered (_("Run till exit from "));
>> +      if (target_get_execution_direction () == EXEC_REVERSE)
>> +       printf_filtered ("Run back to call of ");
>> +      else
>> +       printf_filtered ("Run till exit from ");
>> +
>>        print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
>>      }
> 
> i18n.

OK.

>> +
>> +static void
>> +finish_backwards (struct symbol *function, struct thread_info *tp)
>> +{
>> +  struct symtab_and_line sal;
>> +  struct breakpoint *breakpoint;
>> +  struct cleanup *old_chain;
>> +  CORE_ADDR func_addr;
>> +  int back_up;
>> +
>> +  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.");
> 
> Can't this happen in some circunstances, like the user hitting
> finish after stepi through
> no-debug-info-at-all-not-even-minimal-symbols code?

Ummmm... maybe.  Probably, I suppose.
There are certainly circumstances where finish doesn't work
in the forward direction...


> 
>> +
>> +  sal = find_pc_line (func_addr, 0);
>> +
>> +  /* TODO: Let's not worry about async until later.  */
> 
> If you don't want to think about it now, could you error out
> in the async+reverse case ?

Good idea.

>> +
>> +  /* We don't need a return value.  */
>> +  tp->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 ())
>> +    {
>> +      /* Set breakpoint and continue.  */
>> +      breakpoint =
>> +       set_momentary_breakpoint (sal,
>> +                                 get_frame_id (get_selected_frame (NULL)),
>> +                                 bp_breakpoint);
>> +      /* Tell the breakpoint to keep quiet.  We won't be done
>> +         until we've done another reverse single-step.  */
>> +      breakpoint_silence (breakpoint);
>> +      old_chain = make_cleanup_delete_breakpoint (breakpoint);
>> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
>> +      /* We will be stopped when proceed returns.  */
>> +      back_up = bpstat_find_breakpoint (tp->stop_bpstat, breakpoint) != NULL;
>> +      do_cleanups (old_chain);
>> +    }
>> +  else
>> +    back_up = 1;
>> +  if (back_up)
>> +    {
>> +      /* 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...) */
>> +      tp->step_range_start = tp->step_range_end = 1;
>> +      proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
> 
> Would calling step_1 or step_once instead work here?  It would
> probably avoid the kludge.

I'll have to get back to you on that.

Thanks for the comments.

Re: [RFA] Reverse Debugging, 4/5

[Joel Brobecker]

> 	* breakpoint.c (breakpoint_silence): New function.
> 	* infcmd.c (finish_command): Check for reverse exec direction.

You're going to hate me, now :).  This is another instance where
I think we can break the code a little differently:

  1. finish_command_backwards (I would have prefered
     "reverse_finish_command but :-P)

  2. finish_command_forward

  3. finish_command:
     {
       [do all the stuff about checking for args, etc]
       if (target_get_exec_dir () == EXEC_REVERSE)
         finish_command_backwards ()
       else
         finish_command_forward ()
     }

That way, we just split the finish_command code into two parts
without moving some of the code, and it's clear that the two paths
are completely distinct.  The "branch-off" approach (that we used
for Ada but that I'm trying to avoid like the plague now) does obscure
the structure of your program.

> +void
> +breakpoint_silence (struct breakpoint *b)
> +{
> +  /* Silence the breakpoint.  */
> +  b->silent = 1;

Minor nit: This name brings little meaning when I see it being called.
Can we change it to "make_breakpoint_silent"?  That way, the comment
in the body becomes useless and can be removed.

> +  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.");

Internal error? I understand that it should probably never happen
in this context, but how about making it a simple error instead.
If we trip this check, it's true that something went wrong, but
let's just abort the command and let the user try to continue
rather than asking the user whether we should abort the whole
session.

> +  /* TODO: Let's not worry about async until later.  */

Should we add a check now, though, and error out if async was requested?

> +      /* (Kludgy way of letting wait_for_inferior know...) */
> +      tp->step_range_start = tp->step_range_end = 1;

AARGH! More special meaning to these addresses. We really ought to
clean these up and put some specific flags in the structure, one day.
I don't know why we're trying so hard to resume these fields.

-- 
Joel

Re: [RFA] Reverse Debugging, 4/5

[Michael Snyder]

Joel Brobecker wrote:
>>       * breakpoint.c (breakpoint_silence): New function.
>>       * infcmd.c (finish_command): Check for reverse exec direction.
> 
> You're going to hate me, now :).  This is another instance where
> I think we can break the code a little differently:
> 
>   1. finish_command_backwards (I would have prefered
>      "reverse_finish_command but :-P)

If this were the function that directly implemented
the reverse-finish command, I would have named it like that.

There's actually another function that implements the
reverse-finish command.  This function is a helper,
one that just does part of the job.

> 
>   2. finish_command_forward
> 
>   3. finish_command:
>      {
>        [do all the stuff about checking for args, etc]
>        if (target_get_exec_dir () == EXEC_REVERSE)
>          finish_command_backwards ()
>        else
>          finish_command_forward ()
>      }
> 
> That way, we just split the finish_command code into two parts
> without moving some of the code, and it's clear that the two paths
> are completely distinct.  The "branch-off" approach (that we used
> for Ada but that I'm trying to avoid like the plague now) does obscure
> the structure of your program.

OK, I'm not totally opposed to the idea.  ;-)


>> +void
>> +breakpoint_silence (struct breakpoint *b)
>> +{
>> +  /* Silence the breakpoint.  */
>> +  b->silent = 1;
> 
> Minor nit: This name brings little meaning when I see it being called.
> Can we change it to "make_breakpoint_silent"?  That way, the comment
> in the body becomes useless and can be removed.

Yeah, ok.



>> +  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.");
> 
> Internal error? I understand that it should probably never happen
> in this context, but how about making it a simple error instead.
> If we trip this check, it's true that something went wrong, but
> let's just abort the command and let the user try to continue
> rather than asking the user whether we should abort the whole
> session.
> 
>> +  /* TODO: Let's not worry about async until later.  */
> 
> Should we add a check now, though, and error out if async was requested?

Yeah.  Will do.


>> +      /* (Kludgy way of letting wait_for_inferior know...) */
>> +      tp->step_range_start = tp->step_range_end = 1;
> 
> AARGH! More special meaning to these addresses. We really ought to
> clean these up and put some specific flags in the structure, one day.
> I don't know why we're trying so hard to resume these fields.

OK, in this instance, I did not add the special meaning.
I was only following what practice was already there.  ;-)

Re: [RFA] Reverse Debugging, 4/5

[Joel Brobecker]

> There's actually another function that implements the
> reverse-finish command.  This function is a helper,
> one that just does part of the job.

I think I got that part, perhaps in my attempt to save my fingers
(they REALLY hurt from too much typing right now :-(), I didn't
express what I wanted to say all that well. I hope you got the
idea.

> >>+      /* (Kludgy way of letting wait_for_inferior know...) */
> >>+      tp->step_range_start = tp->step_range_end = 1;
> >
> >AARGH! More special meaning to these addresses. We really ought to
> >clean these up and put some specific flags in the structure, one day.
> >I don't know why we're trying so hard to resume these fields.
> 
> OK, in this instance, I did not add the special meaning.
> I was only following what practice was already there.  ;-)

right - I was just complaining, not saying that you were responsible
nor asking you to fix it. No problemo (and thanks for bearing with
my whining ;-).

-- 
Joel

Re: [RFA] Reverse Debugging, 4/5

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Michael> +void
Michael> +breakpoint_silence (struct breakpoint *b)
Michael> +{
Michael> +  /* Silence the breakpoint.  */
Michael> +  b->silent = 1;

Joel> Minor nit: This name brings little meaning when I see it being called.
Joel> Can we change it to "make_breakpoint_silent"?  That way, the comment
Joel> in the body becomes useless and can be removed.

The Python breakpoint wrapper directly sets or clears b->silent.
Perhaps we could make this a generic setter in anticipation of that
patch?

Tom

Re: [RFA] Reverse Debugging, 4/5

[Michael Snyder]

Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Michael> +void
> Michael> +breakpoint_silence (struct breakpoint *b)
> Michael> +{
> Michael> +  /* Silence the breakpoint.  */
> Michael> +  b->silent = 1;
> 
> Joel> Minor nit: This name brings little meaning when I see it being called.
> Joel> Can we change it to "make_breakpoint_silent"?  That way, the comment
> Joel> in the body becomes useless and can be removed.
> 
> The Python breakpoint wrapper directly sets or clears b->silent.
> Perhaps we could make this a generic setter in anticipation of that
> patch?

Erm, yeah, I started to say "I'm open to it", but
hey, I just want to get my patch in!  Surely it will
be easy enough to change this to a generic setter
later?

Re: [RFA] Reverse Debugging, 4/5

[Tom Tromey]

Michael> Erm, yeah, I started to say "I'm open to it", but
Michael> hey, I just want to get my patch in!  Surely it will
Michael> be easy enough to change this to a generic setter
Michael> later?

Yeah, it is no problem.

Tom

Re: [RFA] Reverse Debugging, 4/5

[Joel Brobecker]

> Erm, yeah, I started to say "I'm open to it", but hey, I just want to
> get my patch in!  Surely it will be easy enough to change this to a
> generic setter later?

I don't think I'm the type to let best be the enemy of good, but
in this case, changing the function name wouldn't delay your patch
all that much. So I would insist in that case that you make the
change now rather than later - that way, there's no risk of
forgetting. Or are you concerned that choosing the name might
turn into a long thread?

This being said, I would be totally open to not using the function
at all, and setting b->silent directly.  This is a well-documented
standalone flag and I'm personally fine with accessing it directly.
Ideally, I would love for GDB to only use opaque types, but we can
always create the accessor routines if/when we decide to make the
breakpoint structure opaque (opinions welcome).

-- 
Joel