Re: [RFA] Reverse Debugging, 4/5

[Michael Snyder <msnyder@vmware.com>]

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