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, 4/5


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




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