This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Reverse Debugging, 3/5


Pedro Alves wrote:
Hi Michael,

Haven't read the other patches yet, but I'll go ahead and give
some comments on this one.

On Wednesday 01 October 2008 20:18:35, Michael Snyder wrote:
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.322
retrieving revision 1.322.2.2
diff -u -p -r1.322 -r1.322.2.2
--- infrun.c    22 Sep 2008 15:26:53 -0000      1.322
+++ infrun.c    30 Sep 2008 23:50:51 -0000      1.322.2.2
@@ -1193,11 +1193,17 @@ proceed (CORE_ADDR addr, enum target_sig


   if (addr == (CORE_ADDR) -1)
     {
-      if (pc == stop_pc && breakpoint_here_p (pc))
+      if (pc == stop_pc && breakpoint_here_p (pc)
+         && target_get_execution_direction () != EXEC_REVERSE)

Hmmm, so EXEC_ERROR is accepted here. What exactly is
EXEC_ERROR useful for? Will there be a target that can't go
either direction? :-)

No, silly... ;-)


Shouldn't failing to find ones
direction always be an error (hence an error call from inside
target_get_execution_direction, or something alike).

Targets that don't implement reverse return EXEC_ERROR, rather than EXEC_FORWARD. It was an early interface design decision, and I'm not sure if I can remember the justification after over 2 years, but I made it consciously -- it seemed to simplify things.


 /* The PTID we'll do a target_wait on.*/
@@ -2141,6 +2149,12 @@ handle_inferior_event (struct execution_
       ecs->event_thread->stop_signal = ecs->ws.value.sig;
       break;

+    case TARGET_WAITKIND_NO_HISTORY:
+      /* Reverse execution: target ran out of history info.  */
+      print_stop_reason (NO_HISTORY, 0);
+      stop_stepping (ecs);
+      return;
+
       /* We had an event in the inferior, but we are not interested
          in handling it at this level. The lower layers have already
          done what needs to be done, if anything.
@@ -2861,6 +2875,17 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
            keep_going (ecs);
            return;
          }

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

Split new line before the operator, not after:

OK


       case BPSTAT_WHAT_CHECK_SHLIBS:
@@ -3026,10 +3051,25 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
       && stop_pc < ecs->event_thread->step_range_end)
     {
       if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
+       fprintf_unfiltered (gdb_stdlog, "infrun: stepping inside range [0x%s-0x%s]\n",
                            paddr_nz (ecs->event_thread->step_range_start),
                            paddr_nz (ecs->event_thread->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

unless

OK


+        keep going back to the call point).  */
+      if (stop_pc == ecs->event_thread->step_range_start &&
+         stop_pc != ecs->stop_func_start &&
+         target_get_execution_direction () == EXEC_REVERSE)
+       {
+         ecs->event_thread->stop_step = 1;
+         print_stop_reason (END_STEPPING_RANGE, 0);
+         stop_stepping (ecs);
+       }

+      else
+       {
+         keep_going (ecs);
+       }

Unneeded braces.

Don't you think it's more readable if the if block and the else block match?

       return;
     }

@@ -3116,10 +3156,28 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (

       if (ecs->event_thread->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_caller (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
+            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)
+           {
+             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);
+           }
+         else
+           insert_step_resume_breakpoint_at_caller (get_current_frame ());
+
          keep_going (ecs);
          return;
        }
@@ -3176,9 +3234,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME (
          return;
        }

-      /* Set a breakpoint at callee's return address (the address at
-         which the caller will resume).  */
-      insert_step_resume_breakpoint_at_caller (get_current_frame ());
+      if (target_get_execution_direction () == EXEC_REVERSE)
+       {
+         /* Set a breakpoint at callee's start address.
+            From there we can step once and be back in the caller.  */
+         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);
+       }
+      else
+       {
+         /* Set a breakpoint at callee's return address (the address
+            at which the caller will resume).  */
+         insert_step_resume_breakpoint_at_caller (get_current_frame ());
+       }

Unneeded braces.

Oh come on -- I know they're syntactic null, but they serve to keep the comment together with the code it refers to.

       keep_going (ecs);
       return;
     }
@@ -3344,6 +3414,28 @@ step_into_function (struct execution_con
     ecs->stop_func_start = gdbarch_skip_prologue
                             (current_gdbarch, ecs->stop_func_start);

+  if (target_get_execution_direction () == EXEC_REVERSE)
+    {
+      stop_func_sal = find_pc_line (stop_pc, 0);
+
+      /* OK, we're just gonna keep stepping here.  */
+      if (stop_func_sal.pc == stop_pc)
+       {
+         /* We're there already.  Just stop stepping now.  */
+         ecs->event_thread->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.  */
+      ecs->event_thread->step_range_start = stop_func_sal.pc;
+      ecs->event_thread->step_range_end   = stop_func_sal.end;

Somethings fishy with the whitespace. ^

I just like things to line up when possible! ;-)


+      keep_going (ecs);
+      return;
+    }
+  /* else... */
   stop_func_sal = find_pc_line (ecs->stop_func_start, 0);
   /* Use the step_resume_break to step until the end of the prologue,
      even if that involves jumps (as it seems to on the vax under
@@ -3712,6 +3804,10 @@ print_stop_reason (enum inferior_stop_re
       annotate_signal_string_end ();
       ui_out_text (uiout, ".\n");
       break;
+    case NO_HISTORY:
+      /* Reverse execution: target ran out of history info.  */
+      ui_out_text (uiout, "\nNo more reverse-execution history.\n");
+      break;
     default:
       internal_error (__FILE__, __LINE__,
                      _("print_stop_reason: unrecognized enum value"));

Otherwise, I can't see anything wrong with it...

Thanks for reviewing.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]