Re: [rfc] [0/7] infrun cleanup

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Pedro Alves wrote:
> On Sunday 07 December 2008 01:28:38, Pedro Alves wrote:
> > On Sunday 07 December 2008 00:15:20, Ulrich Weigand wrote:
> > 
> > > I'd appreciate any feedback on this approach (in particular if 
> > > you've done things differently in your attempt) ...
> > 
> > Eh, no, a lot of dejavu here.  :-)
> > 
> > > Overall patch set tested on amd64-linux with no regressions.
> > 
> > Thanks much for doing this.  I've commented on one.  I'll
> > look at the rest tomorrow.
> > 
> 
> It all looks good to me.

Thanks for your feedback!

> The only bit that concerns me, is:
> 
> > Within handle_inferior_event (and its subroutines), every path that
> > ends in
> > 
> >    stop_stepping (ecs);
> >    return;
> > 
> > is replaced by
> > 
> >    return 0;
> > 
> > and likewise every path that ends in
> > 
> >    prepare_to_wait (ecs);
> >    return
> > 
> > is replaced by 
> > 
> >    return 1;
> 
> I'm not so sure this makes things clearer than what's there
> currently.  One now has to remember what "return 0" or "return 1" means,
> while previously, calls to prepare_to_wait/stop_stepping made
> it quite explicit.
> 
> We also lost the debug message that hinted us that we're going
> to need to wait for another target event ("infrun: prepare_to_wait"), or
> that we're done ("infrun: stop_stepping").  Perhaps leave the
> stop_stopping/prepare_to_wait functions, for the debug output, and
> for clarity?

I agree that the return 0/1 is not quite optimal.  On the other hand,
I feel it would be nice to get rid of those nearly-empty functions
(the debug messages seem to me mostly redundant, as the callers of
stop_stepping / prepare_to_wait typically already have their own,
more specific debug message ...).

In the context of some further cleanup and splitting handle_inferior_event
into multiple more independent parts, I had been wondering whether it
might be a good idea to use a enum (like enum inferior_stop_reason) 
instead of the boolean: handle_inferior_event (and its hypothetical
subroutines) would return enum values to indicate *why* the inferior
stopped, including a new STILL_RUNNING value to indicate that it
in fact hasn't yet stopped.

In this set-up you'd have statements like

   return STILL_RUNNING;

or

   return END_STEPPING_RANGE;

or (another potential new value)

   return HIT_BREAKPOINT;

within handle_inferior_event; and its caller would be rewritten like

  do
    {
      ptid = target_wait (...);
      stop_reason = handle_inferior_event (ptid, ...);
    }
  while (stop_reason == STILL_RUNNING);

It might be feasible to use the stop_reason in the future to merge
some of the print_stop_reason stuff into normal_stop and reduce the
amount of duplicate checks; or even to replace some of the "global"
output variables like stopped_by_random_signal or tp->stop_step.

What do you think?


> There are a couple of comments left behind that should be cleaned up,
> if we remove those functions, e.g.,
> 
> /* Print why the inferior has stopped. We always print something when
>    the inferior exits, or receives a signal. The rest of the cases are
>    dealt with later on in normal_stop() and print_it_typical().  Ideally
>    there should be a call to this function from handle_inferior_event()
>    each time stop_stepping() is called.*/

I thought I had updated that one?

>   /* Refresh prev_pc value just prior to resuming.  This used to be
>      done in stop_stepping, however, setting prev_pc there did not handle
>      scenarios such as inferior function calls or returning from
>      a function via the return command.  In those cases, the prev_pc
>      ...

I left that because it specifically refers to how things were done
in the past, when we still had that function ...  Maybe it could be
marked as "the former stop_stepping" or so.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com