Re: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement

["Ulrich Weigand" <uweigand@de.ibm.com>]

Doug Evans wrote:

> Another issue is that if proceed() does error out, users won't see the
> evaluation-has-been-abandoned message.  That could be fixed by
> invoking proceed in a TRY_CATCH.  [For reference sake, that's not the
> only reason I'm wondering about using TRY_CATCH here.]

I think using a TRY_CATCH here would really make a lot of sense ...

> > Hmmm, that's not quite what what the code actually does.  The state is also
> > restored if some error is thrown during the proceed call, for example.
> > I'm not sure whether this is really the right thing to do ...
> 
> Right, thanks.
> Though it's not clear to me whether "this" in your last sentence is
> intending to suggest that the comment is correct or the code is.
> 
> For education's sake, is there an instance where one would want to
> restore inferior_status when the inferior function call prematurely
> stops (for whatever reason)? [assuming unwindonsignal == off.]

I wasn't completely clear on this myself, so this triggered me to think
again about the whole question of what to restore when ...

Let's look at the various bits of information that are saved and
restored, and why they need to be.  Expanding a bit on the split
your patch already introduced, it seems to me that we have:

Inferior thread state (what you call inferior_program_state):
  current regcache
  stop_pc  (redundant with regcache)
  tp->stop_signal

"proceed" state (mostly what you call inferior_status ...):
  "Output arguments" of proceed:
    tp->stop_bpstat
    tp->stop_step
    stop_stack_dummy
    stopped_by_random_signal
    breakpoint_proceeded
  "Input arguments" to proceed:
    stop_after_trap
    inf->stop_soon
    tp->proceed_to_finish
    tp->step_over_calls
    tp->step_range_start
    tp->step_range_end
    tp->step_frame_id
  Internal proceed control flow:
    tp->trap_expected  (probably does not need to be saved at all)

User-selected state  (... except for this, which seems somewhat distinct):
  selected frame
  implicitly: current thread


Now amongst those, the inferior thread state absolutely must be restored
at some point, or else we cannot continue running that thread.

The proceed state needs to be restored whenever call_function_by_hand
returns successfully.  This is particularly clear for the "output"
arguments: the caller of call_function_by_hand may still be interested
in why the last prior call to proceed returned, and should not be
distracted by the intervening call to proceed in call_function_by_hand
(e.g. if as part of the execution of a breakpoint command list an
inferior function happens to be called).  The case for saving and
restoring the "input" arguments is less compelling; I don't really
see where an inferior call could intervene between the setting of
those flags and the proceed call for which they are intended.  On
the other hand, it cannot hurt to save/restore them either.

In any case, there is no need (and potential harm if the current
thread has changed!) to restore any of this state in the case
where call_function_by_hand throws an error; we get thrown back up
to the user interface, where any proceed input/output state is no
longer interesting.

As to the user-selected state, this also needs to be preserved 
whenever call_function_by_hand returns successfully.  If it throws
an error, however, the selected frame and thread should *not* be
restored but set to the frame/thread in which the error occured.


Now let's look at the various places where we might want to 
restore state:

1. An error can occur during stack frame preparation

2. An error can occur during proceed

3. After proceed finishes normally, we are not where we expect to be
   (inferior terminated, wrong thread, got signaled, hit breakpoint)

4. After proceed finishes normally, we are at the stack dummy where
   we expected to be

5. At some point after a prior call_function_by_hand returned an
   error, we run into the old stack dummy


In the case of 1. the registers of the current thread may have been
changed, and we need to restore them.  At this point, really nothing
else could have changed anyway.

In the case of 2. or 3., it seems to me we should not restore 
*anything*.  We should leave the current thread and selected frame 
to indicate where the error has happened, and throw control back up
to the user interface layer.  Likewise, the inferior thread state
should be left unmodified to allow the user to inspect the location
where the problem occured (of course, the dummy frame needs to remain
pushed to allow a potential restore at some later point).  The "proceed"
state is irrelevant at this point, and should not be touched.

In the case of 4., we need to restore *everything*.  The inferior thread
state was already restored by the implicit pop of the dummy frame.
This leaves the "proceed" state as well as the selected frame to
be restored (the current thread must be correct at this point).

In the case of 5., only the inferior state needs to be restored;
this is what popping the dummy frame already does.


To accomplish that, it seems that the simplest way to do so would
be to install a cleanup to restore the inferior thread state while
preparing the frame; once this is done, the cleanup is removed and
responsibility for restoring the inferior state is moved into the (at
this point freshly pushed) dummy frame.  (This is just what the code
already does.)

The "proceed" state (and the selected frame) on the other hand should
not be installed via a cleanup at all, I think.  They should simply be
explicitly restored only in the case 4. as above.


Does this make sense?  I hope I haven't overlooked anything here ...

Bye,
Ulrich

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