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

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

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.

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


>+/* 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 ...

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

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

> struct inferior_status *
>-save_inferior_status (int restore_stack_info)
>+save_inferior_status ()

(void), please.


Thanks,
Ulrich

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