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

Doug Evans
2008-12-04T22:32:53Z

On Thu, Dec 4, 2008 at 7:31 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Doug Evans wrote:
>
>> /* Everything's ready, push all the info needed to restore the
>> caller (and identify the dummy-frame) onto the dummy-frame
>> stack. */
>>- dummy_frame_push (caller_regcache, &dummy_id);
>>- discard_cleanups (caller_regcache_cleanup);
>>+ dummy_frame = dummy_frame_push (caller_state, &dummy_id);
>>+ /* Do this before calling make_cleanup_pop_dummy_frame. */
>>+ discard_cleanups (caller_state_cleanup);
>>+ dummy_frame_cleanup = make_cleanup_pop_dummy_frame (dummy_frame);
>
> This will cause any random error during proceed to pop the dummy
> frame -- but the frame could still be on the call chain, couldn't it?
>
> I think we should only (explicitly) pop the dummy frame either after
> successful completion of the call, or when we do an explicit unwind.

Agreed. I'll fix that.

>>+ if (! ptid_equal (this_thread_ptid, inferior_thread ()->ptid))
>>+ {
>>+ /* We've switched threads. This can happen if another thread gets a
>>+ signal or breakpoint while our thread was running.
>>+ There's no point in restoring the inferior status,
>>+ we're in a different thread. */
>>+ discard_cleanups (inf_status_cleanup);
>>+ discard_inferior_status (inf_status);
>>+ /* Keep the dummy frame record, if the user switches back to the
>>+ thread with the hand-call, we'll need it. */
>>+ error (_("\
>>+The current thread has changed while making a function call from GDB.\n\
>>+The state of the function call has been lost.\n\
>>+It may be recoverable by changing back to the original thread\n\
>>+and examining the state."));
>>+ }
>
> The various error messages in this function all use different wording to
> express the same fact:
> - Evaluation of the expression containing the function (%s) will be
> abandoned. [signal case]
> - When the function (%s) is done executing, GDB will silently
> stop (instead of continuing to evaluate the expression containing
> the function call). [breakpoint case]
> - The state of the function call has been lost.
> It may be recoverable by changing back to the original thread
> and examining the state. [your new case]
>
> It would be preferable to use the same wording. Maybe Eli has some
> thoughts here ...
>
> Also, to provide additional information, it might be nice to distinguish
> between the cases:
> The program being debugged was signaled in another thread ...
> and
> The program being debugged stopped in another thread ...

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.]

>>+/* Two structures are used to record inferior state.
>>
>>+ inferior_program_state contains state about the program itself like its
>>+ registers and any signal it received when it last stopped.
>>+ This state must be restored regardless of how the inferior function call
>>+ ends (either successfully, or after it hits a breakpoint or signal)
>>+ if the program is to properly continue where it left off.
>>+
>>+ inferior_status contains state regarding gdb's control of the inferior
>>+ itself like stepping control. It also contains session state like the
>>+ user's currently selected frame.
>>+ This state is only restored upon successful completion of the
>>+ inferior function call.
>
> 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.]

>> if (stop_stack_dummy)
>> {
>>- /* Pop the empty frame that contains the stack dummy. POP_FRAME
>>- ends with a setting of the current frame, so we can use that
>>- next. */
>>- frame_pop (get_current_frame ());
>>- /* Set stop_pc to what it was before we called the function.
>>- Can't rely on restore_inferior_status because that only gets
>>- called if we don't stop in the called function. */
>>- stop_pc = read_pc ();
>>- select_frame (get_current_frame ());
>>+ /* Pop the empty frame that contains the stack dummy.
>>+ This also restores all inferior state prior to the call.
>>+ If the current frame is not a stack dummy, do nothing and warn
>>+ the user. */
>>+ struct frame_info *frame = get_current_frame ();
>>+ if (get_frame_type (frame) == DUMMY_FRAME)
>>+ {
>>+ dummy_frame_pop (get_frame_id (frame));
>>+ }
>>+ else
>>+ {
>>+ /* We avoid calling the frame a dummy frame as it has little
>>+ meaning to the user. */
>>+ printf_filtered (_("\
>>+Stopped after an inferior function call, but not in the expected stack frame.\n\
>>+Proceed with caution.\n"));
>>+ }
>
> I don't quite see how this can ever happen; stop_stack_dummy should be
> true only for a bp_call_dummy breakpoint, which is only recognized if
> the current frame ID matches the dummy frame ID. So for the above check
> to trigger would require that we're in a frame with dummy frame ID but
> not a dummy frame ...

Ya. I was reluctant to add an assert so I went with what's there.
I'll change the code to assert the frame is a dummy frame.

>>+struct inferior_program_state
>> {
>> enum target_signal stop_signal;
>> CORE_ADDR stop_pc;
>>+ struct regcache *registers;
>>+};
>
> Isn't stop_pc redundant? We should be able just set stop_pc to
> regcache_read_pc (registers) after restoring the registers, just
> like the code above did ...

Too much timid cut-n-paste-n-tweak and not enough bold rewriting?
stop_pc/registers is in the original inferior_status so I kept them.
In the original code, is there a case when stop_pc != registers.pc?

>> struct inferior_status *
>>-save_inferior_status (int restore_stack_info)
>>+save_inferior_status ()
>
> (void), please.

Righto.

And thanks for the review.