Re: [RFA] Reverse Debugging, 4/5

Michael Snyder
2008-10-06T22:18:34Z

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