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

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

[Doug Evans]

Hi.  This patch was in progress when I wrote
http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
This patch subsumes that one.

There are two things this patch does:
1) properly pop dummy frames more often
2) make the inferior resumable after an inferior function call gets a
   signal without having to remember to do "signal 0"

1) properly pop dummy frames more often

Ulrich asked:
> I agree that it would be better if call_function_by_hand were to call
> dummy_frame_pop in this case.  However, why exactly is this a bug?

It's a bug because it's confusing to not pop the frame in the places
where one is able to.  Plus it's, well, unclean.
I recognize that the dummy frame stack can't be precisely managed because
the user can do things like:

set $orig_pc = $pc
set $orig_sp = $sp
break my_fun
call my_fun()
set $pc = $orig_pc
set $sp = $orig_sp
continue

[This doesn't always work, but you get the idea.]
At the "continue", the inferior function call no longer exists and
gdb's mechanisms for removing the dummy frame from dummy_frame_stack
will never trigger (until the program is resumed and cleanup_dummy_frames
is called).  But we can still keep things clean
and unconfusing for the common case.

2) make the inferior resumable after an inferior function call gets a
   signal without having to remember to do "signal 0"

If an inferior function call gets a signal (say SIGSEGV or SIGABRT),
one should be able to easily resume program execution after popping the stack.
It works today, but after manually popping the stack (e.g. with
"frame <dummy> ; return" where <dummy> is the dummy stack frame's number)
one has to remember to resume the program with "signal 0".
This is because stop_signal doesn't get restored.
Maybe there's a reason it shouldn't be, or maybe should be made an option,
but the current behaviour seems user-unfriendly for the normal case.

Restoring stop_signal also has the benefit that if an inferior function
call is made after getting a signal, and the inferior function call
doesn't complete (e.g. has a breakpoint and doesn't complete right away),
the user can resume the program (after popping the inf fun call off the
stack) with "continue" without having to remember what the signal was
and remember to use "signal N".

[BTW, IWBN if there was a command that did the equivalent of
"frame <dummy> ; return", named say "abandon", so that one didn't have
to go to the trouble of finding the dummy frame's stack number.
"abandon" would have the additional benefit that if the stack
was corrupted and one couldn't find the dummy frame, it would still
work since everything it needs is in dummy_frame_stack - it doesn't need
to examine the inferior's stack, except maybe for some sanity checking.
Continuing the inferior may still not be possible, but it does give the
user a more straightforward way to abandon an inferior function call
than exists today.]

The bulk of the patch goes into making (2) work in a clean way.
Right now the inferior state that can be restored is a collection of
inferior_status, regcache, and misc. things like stop_pc (see the
assignment of stop_pc in normal_stop() when stop_stack_dummy).
The patch organizes the state into two pieces: inferior program state
(registers, stop_pc, stop_signal) and gdb session state
(the rest of inferior_status).
Program state is recorded on the dummy frame stack and is always
restored, either when the inferior function call completes or
when the stack frame is manually popped.
inferior_status contains the things that only get restored
if either the inferior function call successfully completes or
it gets a signal and unwindonsignal is set.

P.S. I've removed one copy of the regcache.  Hopefully I've structured
things in a way that doesn't lose.

NOTES:
- this adds the unwindonsignal.exp testcase from before, plus a new
  testcase that verifies one can resume the inferior after abandoning
  an inferior function call that gets a signal

It's a big patch so presumably there are issues with it.
Comments?

[tested on i386-linux and x86_64-linux]

2008-11-18  Doug Evans  <dje@google.com>

	* dummy-frame.c (dummy_frame): Replace regcache member with
	caller_state.
	(dummy_frame_push): Replace caller_regcache arg with caller_state.
	Return pointer to created dummy frame.  All callers updated.
	(remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame,
	assert_dummy_present,pop_dummy_frame_below,lookup_dummy_id,
	dummy_frame_discard,do_pop_dummy_frame_cleanup,
	make_cleanup_pop_dummy_frame): New functions.
	(dummy_frame_pop): Rewrite.  Verify requested frame is in the
	dummy frame stack.  Restore program state.  Discard dummy frames below
	the one being deleted.
	(dummy_frame_sniffer): Update.
	* dummy-frame.h (dummy_frame_push): Update prototype.
	(dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
	* frame.c (frame_pop): dummy_frame_pop now does all the work for
	DUMMY_FRAMEs.
	* infcall.c (call_function_by_hand): Replace caller_regcache,
	caller_regcache_cleanup with caller_state, caller_state_cleanup.
	New locals dummy_frame, dummy_frame_cleanup.
	Ensure dummy frame is popped/discarded for all possible exit paths.
	* inferior.h (inferior_program_state): Declare (opaque type).
	(save_inferior_program_state,restore_inferior_program_state,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): Declare.
	(save_inferior_status): Update prototype.
	* infrun.c: #include "dummy-frame.h"
	(normal_stop): When stopped for the completion of an inferior function
	call, verify the expected stack frame kind and use dummy_frame_pop.
	(inferior_program_state): New struct.
	(save_inferior_program_state,restore_inferior_program_state,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): New functions.
	(save_inferior_status): Remove arg restore_stack_info.
	All callers updated.  Remove saving of state now saved by
	save_inferior_program_state.
	(restore_inferior_status): Remove restoration of state now done by
	restore_inferior_program_state.
	(discard_inferior_status): Remove freeing of registers, now done by
	discard_inferior_program_state.

	* gdb.base/call-signal-resume.exp: New file.
	* gdb.base/call-signals.c: New file.
	* gdb.base/unwindonsignal.exp: New file.

Index: dummy-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.c,v
retrieving revision 1.53
diff -u -p -u -p -r1.53 dummy-frame.c
--- dummy-frame.c	30 Oct 2008 20:35:30 -0000	1.53
+++ dummy-frame.c	18 Nov 2008 11:23:36 -0000
@@ -42,8 +42,8 @@ struct dummy_frame
   /* This frame's ID.  Must match the value returned by
      gdbarch_dummy_id.  */
   struct frame_id id;
-  /* The caller's regcache.  */
-  struct regcache *regcache;
+  /* The caller's state prior to the call.  */
+  struct inferior_program_state *caller_state;
 };
 
 static struct dummy_frame *dummy_frame_stack = NULL;
@@ -81,61 +81,183 @@ deprecated_pc_in_call_dummy (CORE_ADDR p
   return 0;
 }
 
-/* Push the caller's state, along with the dummy frame info, onto a
+/* Push the caller's state, along with the dummy frame info, onto the
    dummy-frame stack.  */
 
-void
-dummy_frame_push (struct regcache *caller_regcache,
+struct dummy_frame *
+dummy_frame_push (struct inferior_program_state *caller_state,
 		  const struct frame_id *dummy_id)
 {
   struct dummy_frame *dummy_frame;
 
   dummy_frame = XZALLOC (struct dummy_frame);
-  dummy_frame->regcache = caller_regcache;
+  dummy_frame->caller_state = caller_state;
   dummy_frame->id = (*dummy_id);
   dummy_frame->next = dummy_frame_stack;
   dummy_frame_stack = dummy_frame;
+
+  return dummy_frame;
 }
 
-/* Pop the dummy frame with ID dummy_id from the dummy-frame stack.  */
+/* Remove *DUMMY_PTR from the dummy frame stack.  */
 
-void
-dummy_frame_pop (struct frame_id dummy_id)
+static void
+remove_dummy_frame (struct dummy_frame **dummy_ptr)
+{
+  struct dummy_frame *dummy = *dummy_ptr;
+
+  *dummy_ptr = dummy->next;
+  discard_inferior_program_state (dummy->caller_state);
+  xfree (dummy);
+}
+
+/* Cleanup handler for dummy_frame_pop.  */
+
+static void
+do_dummy_frame_cleanup (void *arg)
 {
-  struct dummy_frame **dummy_ptr;
+  struct dummy_frame **dummy_ptr = arg;
+
+  remove_dummy_frame (dummy_ptr);
+}
+
+/* Pop *DUMMY_PTR, restoring program state to that before the
+   frame was created.  */
+
+static void
+pop_dummy_frame (struct dummy_frame **dummy_ptr)
+{
+  struct cleanup *cleanups;
+  struct dummy_frame *dummy;
+
+  cleanups = make_cleanup (do_dummy_frame_cleanup, dummy_ptr);
 
-  for (dummy_ptr = &dummy_frame_stack;
-       (*dummy_ptr) != NULL;
-       dummy_ptr = &(*dummy_ptr)->next)
+  restore_inferior_program_state ((*dummy_ptr)->caller_state);
+
+  /* restore_inferior_status frees inf_status,
+     all that remains is to pop *dummy_ptr */
+  discard_cleanups (cleanups);
+  dummy = *dummy_ptr;
+  *dummy_ptr = dummy->next;
+  xfree (dummy);
+
+  /* We've made right mess of GDB's local state, just discard
+     everything.  */
+  reinit_frame_cache ();
+}
+
+/* Assert that DUMMY is in the dummy frame stack.  */
+
+static void
+assert_dummy_present (struct dummy_frame *dummy)
+{
+  struct dummy_frame *d;
+  int found = 0;
+
+  for (d = dummy_frame_stack; d != NULL; d = d->next)
     {
-      struct dummy_frame *dummy = *dummy_ptr;
-      if (frame_id_eq (dummy->id, dummy_id))
+      if (d == dummy)
 	{
-	  *dummy_ptr = dummy->next;
-	  regcache_xfree (dummy->regcache);
-	  xfree (dummy);
+	  found = 1;
 	  break;
 	}
     }
+  gdb_assert (found);
 }
 
-/* There may be stale dummy frames, perhaps left over from when a longjump took us
-   out of a function that was called by the debugger.  Clean them up at least once
-   whenever we start a new inferior.  */
+/* Pop DUMMY, restore inferior state,
+   and remove all dummy frames below DUMMY from the stack.
+   If the frame isn't found, flag an internal error.  */
 
 static void
-cleanup_dummy_frames (struct target_ops *target, int from_tty)
+pop_dummy_frame_and_below (struct dummy_frame *dummy)
+{
+  assert_dummy_present (dummy);
+  while (dummy_frame_stack != dummy)
+    {
+      remove_dummy_frame (&dummy_frame_stack);
+    }
+  pop_dummy_frame (&dummy_frame_stack);
+}
+
+/* Look up DUMMY_ID.
+   Return NULL if not found.  */
+
+static struct dummy_frame *
+lookup_dummy_id (struct frame_id dummy_id)
 {
-  struct dummy_frame *dummy, *next;
+  struct dummy_frame *dummy;
 
-  for (dummy = dummy_frame_stack; dummy; dummy = next)
+  for (dummy = dummy_frame_stack; dummy != NULL; dummy = dummy->next)
     {
-      next = dummy->next;
-      regcache_xfree (dummy->regcache);
-      xfree (dummy);
+      if (frame_id_eq (dummy->id, dummy_id))
+	return dummy;
     }
 
-  dummy_frame_stack = NULL;
+  return NULL;
+}
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   This also removes all dummy frames below DUMMY_ID from the stack.
+   On return reinit_frame_cache has been called.
+
+   If the frame isn't found, flag an internal error.  */
+
+void
+dummy_frame_pop (struct frame_id dummy_id)
+{
+  struct dummy_frame *dummy;
+
+  dummy = lookup_dummy_id (dummy_id);
+  gdb_assert (dummy != NULL);
+
+  pop_dummy_frame_and_below (dummy);
+}
+
+/* Discard DUMMY and remove it from the dummy frame stack.
+   This also removes all dummy frames below DUMMY from the stack.
+
+   If the frame isn't found, flag an internal error.  */
+
+void
+dummy_frame_discard (struct dummy_frame *dummy)
+{
+  assert_dummy_present (dummy);
+  while (dummy_frame_stack != dummy)
+    {
+      remove_dummy_frame (&dummy_frame_stack);
+    }
+  remove_dummy_frame (&dummy_frame_stack);
+}
+
+/* Utility for make_cleanup_pop_dummy_frame.  */
+
+static void
+do_pop_dummy_frame_cleanup (void *dummy)
+{
+  pop_dummy_frame_and_below (dummy);
+}
+
+/* Schedule a cleanup to pop DUMMY_FRAME and restore inferior state.  */
+
+struct cleanup *
+make_cleanup_pop_dummy_frame (struct dummy_frame *dummy)
+{
+  return make_cleanup (do_pop_dummy_frame_cleanup, dummy);
+}
+
+/* There may be stale dummy frames, perhaps left over from when a longjump took
+   us out of a function that was called by the debugger.  Clean them up at
+   least once whenever we start a new inferior.  */
+
+static void
+cleanup_dummy_frames (struct target_ops *target, int from_tty)
+{
+  while (dummy_frame_stack != NULL)
+    {
+      remove_dummy_frame (&dummy_frame_stack);
+    }
 }
 
 /* Return the dummy frame cache, it contains both the ID, and a
@@ -178,7 +300,7 @@ dummy_frame_sniffer (const struct frame_
 	    {
 	      struct dummy_frame_cache *cache;
 	      cache = FRAME_OBSTACK_ZALLOC (struct dummy_frame_cache);
-	      cache->prev_regcache = dummyframe->regcache;
+	      cache->prev_regcache = get_inferior_program_state_regcache (dummyframe->caller_state);
 	      cache->this_id = this_id;
 	      (*this_prologue_cache) = cache;
 	      return 1;
@@ -215,7 +337,7 @@ dummy_frame_prev_register (struct frame_
   return reg_val;
 }
 
-/* Assuming that THIS frame is a dummy, return the ID of THIS frame.  That ID is
+/* Assuming that THIS_FRAME is a dummy, return its ID.  That ID is
    determined by examining the NEXT frame's unwound registers using
    the method dummy_id().  As a side effect, THIS dummy frame's
    dummy cache is located and and saved in THIS_PROLOGUE_CACHE.  */
Index: dummy-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.h,v
retrieving revision 1.23
diff -u -p -u -p -r1.23 dummy-frame.h
--- dummy-frame.h	8 Sep 2008 15:23:12 -0000	1.23
+++ dummy-frame.h	18 Nov 2008 11:23:36 -0000
@@ -23,8 +23,10 @@
 #include "frame.h"
 
 struct frame_info;
-struct regcache;
+struct inferior_program_state;
 struct frame_unwind;
+struct dummy_frame;
+struct cleanup;
 
 /* Push the information needed to identify, and unwind from, a dummy
    frame onto the dummy frame stack.  */
@@ -39,11 +41,27 @@ struct frame_unwind;
    be expanded so that it knowns the lower/upper extent of the dummy
    frame's code.  */
 
-extern void dummy_frame_push (struct regcache *regcache,
-			      const struct frame_id *dummy_id);
+extern struct dummy_frame *dummy_frame_push (struct inferior_program_state *caller_state,
+					     const struct frame_id *dummy_id);
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   This also removes all dummy frames below DUMMY_ID from the stack,
+   and calls reinit_frame_cache.
+
+   If the frame isn't found, flag an internal error.  */
 
 extern void dummy_frame_pop (struct frame_id dummy_id);
 
+/* Discard DUMMY and remove it from the dummy frame stack.
+   This also removes all dummy frames below DUMMY from the stack.  */
+
+extern void dummy_frame_discard (struct dummy_frame *dummy);
+
+/* Schedule a cleanup to pop DUMMY_FRAME.  */
+
+extern struct cleanup *make_cleanup_pop_dummy_frame (struct dummy_frame *);
+
 /* If the PC falls in a dummy frame, return a dummy frame
    unwinder.  */
 
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.255
diff -u -p -u -p -r1.255 frame.c
--- frame.c	24 Sep 2008 12:59:49 -0000	1.255
+++ frame.c	18 Nov 2008 11:23:36 -0000
@@ -536,6 +536,14 @@ frame_pop (struct frame_info *this_frame
   struct regcache *scratch;
   struct cleanup *cleanups;
 
+  if (get_frame_type (this_frame) == DUMMY_FRAME)
+    {
+      /* Popping a dummy frame involves restoring more than just registers.
+	 dummy_frame_pop does all the work.  */
+      dummy_frame_pop (get_frame_id (this_frame));
+      return;
+    }
+
   /* Ensure that we have a frame to pop to.  */
   prev_frame = get_prev_frame_1 (this_frame);
 
@@ -549,11 +557,6 @@ frame_pop (struct frame_info *this_frame
   scratch = frame_save_as_regcache (prev_frame);
   cleanups = make_cleanup_regcache_xfree (scratch);
 
-  /* If we are popping a dummy frame, clean up the associated
-     data as well.  */
-  if (get_frame_type (this_frame) == DUMMY_FRAME)
-    dummy_frame_pop (get_frame_id (this_frame));
-
   /* FIXME: cagney/2003-03-16: It should be possible to tell the
      target's register cache that it is about to be hit with a burst
      register transfer and that the sequence of register writes should
Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.106
diff -u -p -u -p -r1.106 infcall.c
--- infcall.c	18 Nov 2008 00:13:02 -0000	1.106
+++ infcall.c	18 Nov 2008 11:23:36 -0000
@@ -318,12 +318,14 @@ call_function_by_hand (struct value *fun
   struct cleanup *retbuf_cleanup;
   struct inferior_status *inf_status;
   struct cleanup *inf_status_cleanup;
+  struct inferior_program_state *caller_state;
+  struct cleanup *caller_state_cleanup;
+  struct dummy_frame *dummy_frame;
+  struct cleanup *dummy_frame_cleanup;
   CORE_ADDR funaddr;
   CORE_ADDR real_pc;
   struct type *ftype = check_typedef (value_type (function));
   CORE_ADDR bp_addr;
-  struct regcache *caller_regcache;
-  struct cleanup *caller_regcache_cleanup;
   struct frame_id dummy_id;
   struct cleanup *args_cleanup;
   struct frame_info *frame;
@@ -351,15 +353,16 @@ call_function_by_hand (struct value *fun
   /* A cleanup for the inferior status.  Create this AFTER the retbuf
      so that this can be discarded or applied without interfering with
      the regbuf.  */
-  inf_status = save_inferior_status (1);
+  inf_status = save_inferior_status ();
   inf_status_cleanup = make_cleanup_restore_inferior_status (inf_status);
 
-  /* Save the caller's registers so that they can be restored once the
+  /* Save the caller's registers and other state associated with the
+     inferior itself so that they can be restored once the
      callee returns.  To allow nested calls the registers are (further
      down) pushed onto a dummy frame stack.  Include a cleanup (which
      is tossed once the regcache has been pushed).  */
-  caller_regcache = frame_save_as_regcache (frame);
-  caller_regcache_cleanup = make_cleanup_regcache_xfree (caller_regcache);
+  caller_state = save_inferior_program_state ();
+  caller_state_cleanup = make_cleanup_restore_inferior_program_state (caller_state);
 
   /* Ensure that the initial SP is correctly aligned.  */
   {
@@ -642,8 +645,10 @@ call_function_by_hand (struct value *fun
   /* 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);
 
   /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP -
      If you're looking to implement asynchronous dummy-frames, then
@@ -705,6 +710,7 @@ call_function_by_hand (struct value *fun
 	 we'll crash as the inferior is no longer running.  */
       discard_cleanups (inf_status_cleanup);
       discard_inferior_status (inf_status);
+      dummy_frame_discard (dummy_frame);
       error (_("\
 The program being debugged exited while in a function called from GDB."));
     }
@@ -745,9 +751,10 @@ The program being debugged exited while 
 	    {
 	      /* The user wants the context restored. */
 
-	      /* We must get back to the frame we were before the
-		 dummy call. */
-	      frame_pop (get_current_frame ());
+	      /* We must get back to the frame we were before the dummy call.
+		 Plus we need to restore inferior status to that before the
+		 dummy call.  This is all handled by cleanups
+		 dummy_frame_cleanup and inf_status_cleanup.  */
 
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
@@ -761,14 +768,16 @@ Evaluation of the expression containing 
 	  else
 	    {
 	      /* The user wants to stay in the frame where we stopped
-                 (default).*/
-	      /* If we restored the inferior status (via the cleanup),
+		 (default).
+		 If we restored the inferior status (via the cleanup),
 		 we would print a spurious error message (Unable to
-		 restore previously selected frame), would write the
-		 registers from the inf_status (which is wrong), and
-		 would do other wrong things.  */
+		 restore previously selected frame), and
+		 would do other wrong things.
+		 Discarding inf_status_cleanup also discards
+		 dummy_frame_cleanup.  */
 	      discard_cleanups (inf_status_cleanup);
 	      discard_inferior_status (inf_status);
+
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
 	      error (_("\
@@ -782,14 +791,17 @@ Evaluation of the expression containing 
 
       if (!stop_stack_dummy)
 	{
-	  /* We hit a breakpoint inside the FUNCTION. */
-	  /* If we restored the inferior status (via the cleanup), we
+	  /* We hit a breakpoint inside the FUNCTION.
+	     If we restored the inferior status (via the cleanup), we
 	     would print a spurious error message (Unable to restore
 	     previously selected frame), would write the registers
 	     from the inf_status (which is wrong), and would do other
-	     wrong things.  */
+	     wrong things.
+	     Discarding inf_status_cleanup also discards
+	     dummy_frame_cleanup.  */
 	  discard_cleanups (inf_status_cleanup);
 	  discard_inferior_status (inf_status);
+
 	  /* The following error message used to say "The expression
 	     which contained the function call has been discarded."
 	     It is a hard concept to explain in a few words.  Ideally,
@@ -811,6 +823,9 @@ the function call)."), name);
 
   /* If we get here the called FUNCTION run to completion. */
 
+  /* The dummy frame has been popped so discard its cleanup.  */
+  discard_cleanups (dummy_frame_cleanup);
+
   /* On normal return, the stack dummy has been popped already.  */
   regcache_cpy_no_passthrough (retbuf, stop_registers);
 
Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.115
diff -u -p -u -p -r1.115 inferior.h
--- inferior.h	17 Nov 2008 16:37:34 -0000	1.115
+++ inferior.h	18 Nov 2008 11:23:36 -0000
@@ -40,24 +40,40 @@ struct ui_out;
 /* For struct frame_id.  */
 #include "frame.h"
 
-/* Structure in which to save the status of the inferior.  Create/Save
-   through "save_inferior_status", restore through
-   "restore_inferior_status".
-
-   This pair of routines should be called around any transfer of
-   control to the inferior which you don't want showing up in your
-   control variables.  */
+/* 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.
+
+   Call these routines around hand called functions, including function calls
+   in conditional breakpoints for example.  */
+
+struct inferior_program_state;
 struct inferior_status;
 
-extern struct inferior_status *save_inferior_status (int);
+extern struct inferior_program_state *save_inferior_program_state (void);
+extern struct inferior_status *save_inferior_status (void);
 
+extern void restore_inferior_program_state (struct inferior_program_state *);
 extern void restore_inferior_status (struct inferior_status *);
 
+extern struct cleanup *make_cleanup_restore_inferior_program_state (struct inferior_program_state *);
 extern struct cleanup *make_cleanup_restore_inferior_status (struct inferior_status *);
 
+extern void discard_inferior_program_state (struct inferior_program_state *);
 extern void discard_inferior_status (struct inferior_status *);
 
+extern struct regcache *get_inferior_program_state_regcache (struct inferior_program_state *);
+
 /* The -1 ptid, often used to indicate either an error condition
    or a "don't care" condition, i.e, "run all threads."  */
 extern ptid_t minus_one_ptid;
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.338
diff -u -p -u -p -r1.338 infrun.c
--- infrun.c	17 Nov 2008 18:50:22 -0000	1.338
+++ infrun.c	18 Nov 2008 11:23:36 -0000
@@ -45,7 +45,7 @@
 #include "language.h"
 #include "solib.h"
 #include "main.h"
-
+#include "dummy-frame.h"
 #include "gdb_assert.h"
 #include "mi/mi-common.h"
 #include "event-top.h"
@@ -4342,15 +4342,23 @@ Further execution is probably impossible
 
   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"));
+	}
     }
 
 done:
@@ -4746,10 +4754,86 @@ signals_info (char *signum_exp, int from
   printf_filtered (_("\nUse the \"handle\" command to change these tables.\n"));
 }
 \f
-struct inferior_status
+/* Inferior program state.
+   These are details related to the inferior itself, and don't include
+   things like what frame the user had selected or what gdb was doing
+   with the target at the time.
+   For inferior function calls these are things we want to restore
+   regardless of whether the function call successfully completes
+   or the dummy frame has to be manually popped.  */
+
+struct inferior_program_state
 {
   enum target_signal stop_signal;
   CORE_ADDR stop_pc;
+  struct regcache *registers;
+};
+
+struct inferior_program_state *
+save_inferior_program_state ()
+{
+  struct inferior_program_state *inf_state = XMALLOC (struct inferior_program_state);
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  inf_state->stop_signal = tp->stop_signal;
+  inf_state->stop_pc = stop_pc;
+
+  inf_state->registers = regcache_dup (get_current_regcache ());
+
+  return inf_state;
+}
+
+/* Restore inferior session state to INF_STATE.  */
+
+void
+restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  tp->stop_signal = inf_state->stop_signal;
+  stop_pc = inf_state->stop_pc;
+
+  /* The inferior can be gone if the user types "print exit(0)"
+     (and perhaps other times).  */
+  if (target_has_execution)
+    /* NB: The register write goes through to the target.  */
+    regcache_cpy (get_current_regcache (), inf_state->registers);
+  regcache_xfree (inf_state->registers);
+}
+
+static void
+do_restore_inferior_program_state_cleanup (void *state)
+{
+  restore_inferior_program_state (state);
+}
+
+struct cleanup *
+make_cleanup_restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  return make_cleanup (do_restore_inferior_program_state_cleanup, inf_state);
+}
+
+void
+discard_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  regcache_xfree (inf_state->registers);
+  xfree (inf_state);
+}
+
+struct regcache *
+get_inferior_program_state_regcache (struct inferior_program_state *inf_state)
+{
+  return inf_state->registers;
+}
+
+/* Session related state for inferior function calls.
+   These are the additional bits of state that need to be restored
+   when an inferior function call successfully completes.  */
+
+struct inferior_status
+{
   bpstat stop_bpstat;
   int stop_step;
   int stop_stack_dummy;
@@ -4763,32 +4847,23 @@ struct inferior_status
   int stop_after_trap;
   int stop_soon;
 
-  /* These are here because if call_function_by_hand has written some
-     registers and then decides to call error(), we better not have changed
-     any registers.  */
-  struct regcache *registers;
-
-  /* A frame unique identifier.  */
+  /* ID if the selected frame when the inferior function call was made.  */
   struct frame_id selected_frame_id;
 
   int breakpoint_proceeded;
-  int restore_stack_info;
   int proceed_to_finish;
 };
 
 /* Save all of the information associated with the inferior<==>gdb
-   connection.  INF_STATUS is a pointer to a "struct inferior_status"
-   (defined in inferior.h).  */
+   connection.  */
 
 struct inferior_status *
-save_inferior_status (int restore_stack_info)
+save_inferior_status ()
 {
   struct inferior_status *inf_status = XMALLOC (struct inferior_status);
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  inf_status->stop_signal = tp->stop_signal;
-  inf_status->stop_pc = stop_pc;
   inf_status->stop_step = tp->stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
@@ -4806,12 +4881,10 @@ save_inferior_status (int restore_stack_
   inf_status->stop_bpstat = tp->stop_bpstat;
   tp->stop_bpstat = bpstat_copy (tp->stop_bpstat);
   inf_status->breakpoint_proceeded = breakpoint_proceeded;
-  inf_status->restore_stack_info = restore_stack_info;
   inf_status->proceed_to_finish = tp->proceed_to_finish;
 
-  inf_status->registers = regcache_dup (get_current_regcache ());
-
   inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL));
+
   return inf_status;
 }
 
@@ -4836,14 +4909,14 @@ restore_selected_frame (void *args)
   return (1);
 }
 
+/* Restore inferior session state to INF_STATUS.  */
+
 void
 restore_inferior_status (struct inferior_status *inf_status)
 {
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  tp->stop_signal = inf_status->stop_signal;
-  stop_pc = inf_status->stop_pc;
   tp->stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
@@ -4856,24 +4929,11 @@ restore_inferior_status (struct inferior
   inf->stop_soon = inf_status->stop_soon;
   bpstat_clear (&tp->stop_bpstat);
   tp->stop_bpstat = inf_status->stop_bpstat;
+  inf_status->stop_bpstat = NULL;
   breakpoint_proceeded = inf_status->breakpoint_proceeded;
   tp->proceed_to_finish = inf_status->proceed_to_finish;
 
-  /* The inferior can be gone if the user types "print exit(0)"
-     (and perhaps other times).  */
-  if (target_has_execution)
-    /* NB: The register write goes through to the target.  */
-    regcache_cpy (get_current_regcache (), inf_status->registers);
-  regcache_xfree (inf_status->registers);
-
-  /* FIXME: If we are being called after stopping in a function which
-     is called from gdb, we should not be trying to restore the
-     selected frame; it just prints a spurious error message (The
-     message is useful, however, in detecting bugs in gdb (like if gdb
-     clobbers the stack)).  In fact, should we be restoring the
-     inferior status at all in that case?  .  */
-
-  if (target_has_stack && inf_status->restore_stack_info)
+  if (target_has_stack)
     {
       /* The point of catch_errors is that if the stack is clobbered,
          walking the stack might encounter a garbage pointer and
@@ -4885,7 +4945,6 @@ restore_inferior_status (struct inferior
 	/* Error in restoring the selected frame.  Select the innermost
 	   frame.  */
 	select_frame (get_current_frame ());
-
     }
 
   xfree (inf_status);
@@ -4908,10 +4967,9 @@ discard_inferior_status (struct inferior
 {
   /* See save_inferior_status for info on stop_bpstat. */
   bpstat_clear (&inf_status->stop_bpstat);
-  regcache_xfree (inf_status->registers);
   xfree (inf_status);
 }
-
+\f
 int
 inferior_has_forked (ptid_t pid, ptid_t *child_pid)
 {
Index: testsuite/gdb.base/call-signal-resume.exp
===================================================================
RCS file: testsuite/gdb.base/call-signal-resume.exp
diff -N testsuite/gdb.base/call-signal-resume.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signal-resume.exp	18 Nov 2008 11:23:37 -0000
@@ -0,0 +1,107 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping call-signal-resume.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested call-signal-resume.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+proc get_dummy_frame_number { } {
+  global gdb_prompt
+
+  send_gdb "bt\n"
+  gdb_expect {
+    -re "#(\[0-9\]*) *<function called from gdb>.*$gdb_prompt $"
+      {
+	return $expect_out(1,string)
+      }
+    -re "$gdb_prompt $"
+      {
+	return ""
+      }
+    timeout
+      {
+	return ""
+      }
+  }
+  return ""
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"call-signal-resume, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "call-signal-resume, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "call-signal-resume, inferior function call signaled"
+    }
+}
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "call-signal-resume, dummy stack frame number"
+    setup_xfail "*-*-*"
+} else {
+    pass "call-signal-resume, dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Resume execution, the program should successfully complete.
+gdb_test "continue" "Program exited normally."
+
+return 0
Index: testsuite/gdb.base/call-signals.c
===================================================================
RCS file: testsuite/gdb.base/call-signals.c
diff -N testsuite/gdb.base/call-signals.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signals.c	18 Nov 2008 11:23:37 -0000
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing handling of inferior function calls
+   in the presence of signals.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported,
+     so try that first.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+#ifdef SIGSEGV
+  kill (getpid (), SIGSEGV);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+  return 0;
+}
Index: testsuite/gdb.base/unwindonsignal.exp
===================================================================
RCS file: testsuite/gdb.base/unwindonsignal.exp
diff -N testsuite/gdb.base/unwindonsignal.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/unwindonsignal.exp	18 Nov 2008 11:23:37 -0000
@@ -0,0 +1,94 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping unwindonsignal.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested unwindonsignal.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+# Turn on unwindonsignal.
+gdb_test "set unwindonsignal on" \
+	"" \
+	"setting unwindonsignal"
+gdb_test "show unwindonsignal" \
+	"Unwinding of stack .* is on." \
+	"showing unwindonsignal"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"unwindonsignal, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "unwindonsignal, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, inferior function call signaled"
+    }
+}
+
+# Verify the stack got unwound.
+gdb_test "bt" \
+	"#0 *main \\(.*\\) at .*" \
+	"unwindonsignal, stack unwound"
+
+# Verify the dummy frame got removed from dummy_frame_stack.
+gdb_test_multiple "maint print dummy-frames" \
+	"unwindonsignal, dummy frame removed" {
+    -re "\[\r\n\]*.*stack=.*code=.*\[\r\n\]+$gdb_prompt $" {
+	fail "unwindonsignal, dummy frame removed"
+    }
+    -re "\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, dummy frame removed"
+    }
+}
+
+return 0

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

[Doug Evans]

On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
> Hi.  This patch was in progress when I wrote
> http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
> This patch subsumes that one.
>
> There are two things this patch does:
> 1) properly pop dummy frames more often
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
>
> 1) properly pop dummy frames more often
>
> Ulrich asked:
>> I agree that it would be better if call_function_by_hand were to call
>> dummy_frame_pop in this case.  However, why exactly is this a bug?
>
> It's a bug because it's confusing to not pop the frame in the places
> where one is able to.  Plus it's, well, unclean.
> I recognize that the dummy frame stack can't be precisely managed because
> the user can do things like:
>
> set $orig_pc = $pc
> set $orig_sp = $sp
> break my_fun
> call my_fun()
> set $pc = $orig_pc
> set $sp = $orig_sp
> continue
>
> [This doesn't always work, but you get the idea.]
> At the "continue", the inferior function call no longer exists and
> gdb's mechanisms for removing the dummy frame from dummy_frame_stack
> will never trigger (until the program is resumed and cleanup_dummy_frames
> is called).  But we can still keep things clean
> and unconfusing for the common case.

For completeness' sake,
To catch cases like these one could do things like compare $sp with
$dummy_frame_stack.*.regcache.sp every time execution stops.

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

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 10757 bytes --]

On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
> Hi.  This patch was in progress when I wrote
> http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
> This patch subsumes that one.
>
> There are two things this patch does:
> 1) properly pop dummy frames more often
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
>
> 1) properly pop dummy frames more often
>
> Ulrich asked:
>> I agree that it would be better if call_function_by_hand were to call
>> dummy_frame_pop in this case.  However, why exactly is this a bug?
>
> It's a bug because it's confusing to not pop the frame in the places
> where one is able to.  Plus it's, well, unclean.
> I recognize that the dummy frame stack can't be precisely managed because
> the user can do things like:
>
> set $orig_pc = $pc
> set $orig_sp = $sp
> break my_fun
> call my_fun()
> set $pc = $orig_pc
> set $sp = $orig_sp
> continue
>
> [This doesn't always work, but you get the idea.]
> At the "continue", the inferior function call no longer exists and
> gdb's mechanisms for removing the dummy frame from dummy_frame_stack
> will never trigger (until the program is resumed and cleanup_dummy_frames
> is called).  But we can still keep things clean
> and unconfusing for the common case.
>
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
>
> If an inferior function call gets a signal (say SIGSEGV or SIGABRT),
> one should be able to easily resume program execution after popping the stack.
> It works today, but after manually popping the stack (e.g. with
> "frame <dummy> ; return" where <dummy> is the dummy stack frame's number)
> one has to remember to resume the program with "signal 0".
> This is because stop_signal doesn't get restored.
> Maybe there's a reason it shouldn't be, or maybe should be made an option,
> but the current behaviour seems user-unfriendly for the normal case.
>
> Restoring stop_signal also has the benefit that if an inferior function
> call is made after getting a signal, and the inferior function call
> doesn't complete (e.g. has a breakpoint and doesn't complete right away),
> the user can resume the program (after popping the inf fun call off the
> stack) with "continue" without having to remember what the signal was
> and remember to use "signal N".
>
> [BTW, IWBN if there was a command that did the equivalent of
> "frame <dummy> ; return", named say "abandon", so that one didn't have
> to go to the trouble of finding the dummy frame's stack number.
> "abandon" would have the additional benefit that if the stack
> was corrupted and one couldn't find the dummy frame, it would still
> work since everything it needs is in dummy_frame_stack - it doesn't need
> to examine the inferior's stack, except maybe for some sanity checking.
> Continuing the inferior may still not be possible, but it does give the
> user a more straightforward way to abandon an inferior function call
> than exists today.]
>
> The bulk of the patch goes into making (2) work in a clean way.
> Right now the inferior state that can be restored is a collection of
> inferior_status, regcache, and misc. things like stop_pc (see the
> assignment of stop_pc in normal_stop() when stop_stack_dummy).
> The patch organizes the state into two pieces: inferior program state
> (registers, stop_pc, stop_signal) and gdb session state
> (the rest of inferior_status).
> Program state is recorded on the dummy frame stack and is always
> restored, either when the inferior function call completes or
> when the stack frame is manually popped.
> inferior_status contains the things that only get restored
> if either the inferior function call successfully completes or
> it gets a signal and unwindonsignal is set.
>
> P.S. I've removed one copy of the regcache.  Hopefully I've structured
> things in a way that doesn't lose.
>
> NOTES:
> - this adds the unwindonsignal.exp testcase from before, plus a new
>  testcase that verifies one can resume the inferior after abandoning
>  an inferior function call that gets a signal
>
> It's a big patch so presumably there are issues with it.
> Comments?
>
> [tested on i386-linux and x86_64-linux]
>
> 2008-11-18  Doug Evans  <dje@google.com>
>
>        * dummy-frame.c (dummy_frame): Replace regcache member with
>        caller_state.
>        (dummy_frame_push): Replace caller_regcache arg with caller_state.
>        Return pointer to created dummy frame.  All callers updated.
>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame,
>        assert_dummy_present,pop_dummy_frame_below,lookup_dummy_id,
>        dummy_frame_discard,do_pop_dummy_frame_cleanup,
>        make_cleanup_pop_dummy_frame): New functions.
>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>        dummy frame stack.  Restore program state.  Discard dummy frames below
>        the one being deleted.
>        (dummy_frame_sniffer): Update.
>        * dummy-frame.h (dummy_frame_push): Update prototype.
>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>        DUMMY_FRAMEs.
>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>        New locals dummy_frame, dummy_frame_cleanup.
>        Ensure dummy frame is popped/discarded for all possible exit paths.
>        * inferior.h (inferior_program_state): Declare (opaque type).
>        (save_inferior_program_state,restore_inferior_program_state,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_state_regcache): Declare.
>        (save_inferior_status): Update prototype.
>        * infrun.c: #include "dummy-frame.h"
>        (normal_stop): When stopped for the completion of an inferior function
>        call, verify the expected stack frame kind and use dummy_frame_pop.
>        (inferior_program_state): New struct.
>        (save_inferior_program_state,restore_inferior_program_state,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_state_regcache): New functions.
>        (save_inferior_status): Remove arg restore_stack_info.
>        All callers updated.  Remove saving of state now saved by
>        save_inferior_program_state.
>        (restore_inferior_status): Remove restoration of state now done by
>        restore_inferior_program_state.
>        (discard_inferior_status): Remove freeing of registers, now done by
>        discard_inferior_program_state.
>
>        * gdb.base/call-signal-resume.exp: New file.
>        * gdb.base/call-signals.c: New file.
>        * gdb.base/unwindonsignal.exp: New file.

ref: http://sourceware.org/ml/gdb-patches/2008-11/msg00454.html

Blech.  I went too far in trying to keep dummy_frame_stack accurate.
One can (theoretically) have multiple hand-function-calls outstanding
in multiple threads (and soon in multiple processes presumably).
Attached is a new version of the patch that goes back to having
dummy_frame_pop only popping the requested frame (and similarily for
dummy_frame_discard).

I wrote a testcase that calls functions in multiple threads (with
scheduler-locking on) by setting a breakpoint on the function being
called.  I think there's a bug in scheduler-locking support as when I
make the second call in the second thread, gdb makes the first thread
step over the breakpoint and then resume, and control returns to
call_function_by_hand with inferior_ptid changed to the first thread.
To protect call_function_by_hand from this it now flags an error if
the thread changes.
[I'll submit the testcase separately once I can get it to pass, unless
folks want it to see it.]

How's this?

2008-11-18  Doug Evans  <dje@google.com>

        * dummy-frame.c (dummy_frame): Replace regcache member with
        caller_state.
        (dummy_frame_push): Replace caller_regcache arg with caller_state.
        Return pointer to created dummy frame.  All callers updated.
        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
        lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
        do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
        functions.
        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
        dummy frame stack.  Restore program state.
        (cleanup_dummy_frames): Rewrite.
        (dummy_frame_sniffer): Update.
        * dummy-frame.h (dummy_frame_push): Update prototype.
        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
        * frame.c (frame_pop): dummy_frame_pop now does all the work for
        DUMMY_FRAMEs.
        * infcall.c (call_function_by_hand): Replace caller_regcache,
        caller_regcache_cleanup with caller_state, caller_state_cleanup.
        New locals dummy_frame, dummy_frame_cleanup, this_thread.
        Ensure dummy frame is popped/discarded for all possible exit paths.
        Detect program stopping in a different thread.
        * inferior.h (inferior_program_state): Declare (opaque type).
        (save_inferior_program_state,restore_inferior_program_state,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_state_regcache): Declare.
        (save_inferior_status): Update prototype.
        * infrun.c: #include "dummy-frame.h"
        (normal_stop): When stopped for the completion of an inferior function
        call, verify the expected stack frame kind and use dummy_frame_pop.
        (inferior_program_state): New struct.
        (save_inferior_program_state,restore_inferior_program_state,
        do_restore_inferior_program_state_cleanup,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_state_regcache): New functions.
        (inferior_status): Remove members stop_signal, stop_pc, registers,
        restore_stack_info.
        (save_inferior_status): Remove arg restore_stack_info.
        All callers updated.  Remove saving of state now saved by
        save_inferior_program_state.
        (restore_inferior_status): Remove restoration of state now done by
        restore_inferior_program_state.
        (discard_inferior_status): Remove freeing of registers, now done by
        discard_inferior_program_state.

        * gdb.base/call-signal-resume.exp: New file.
        * gdb.base/call-signals.c: New file.
        * gdb.base/unwindonsignal.exp: New file.

[-- Attachment #2: gdb-081119-sym-info-2.patch.txt --]
[-- Type: text/plain, Size: 3606 bytes --]

2008-11-19  Doug Evans  <dje@google.com>

	* printcmd.c (sym_info): Don't print the offset if it's zero.

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.137
diff -u -p -r1.137 printcmd.c
--- printcmd.c	18 Nov 2008 21:31:26 -0000	1.137
+++ printcmd.c	20 Nov 2008 06:09:30 -0000
@@ -1013,6 +1013,8 @@ sym_info (char *arg, int from_tty)
 	&& (msymbol = lookup_minimal_symbol_by_pc_section (sect_addr, osect)))
       {
 	const char *obj_name, *mapped, *sec_name, *msym_name;
+	char *loc_string;
+	struct cleanup *old_chain;
 
 	matches = 1;
 	offset = sect_addr - SYMBOL_VALUE_ADDRESS (msymbol);
@@ -1020,43 +1022,55 @@ sym_info (char *arg, int from_tty)
 	sec_name = osect->the_bfd_section->name;
 	msym_name = SYMBOL_PRINT_NAME (msymbol);
 
+	/* Don't print the offset if it is zero.
+	   We assume there's no need to handle i18n of "sym + offset".  */
+	if (offset)
+	  xasprintf (&loc_string, "%s + %u", msym_name, offset);
+	else
+	  xasprintf (&loc_string, "%s", msym_name);
+
+	/* Use a cleanup to free loc_string in case the user quits
+	   a pagination request inside printf_filtered.  */
+	old_chain = make_cleanup (xfree, loc_string);
+
 	gdb_assert (osect->objfile && osect->objfile->name);
 	obj_name = osect->objfile->name;
 
 	if (MULTI_OBJFILE_P ())
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "%s overlay section %s of %s\n"),
-			       msym_name, offset,
-			       mapped, sec_name, obj_name);
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of "
+	      printf_filtered (_("%s in load address range of "
 				 "section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+			       loc_string, sec_name, obj_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s of %s\n"),
-			       msym_name, offset, mapped, sec_name, obj_name);
+	      printf_filtered (_("%s in %s overlay section %s of %s\n"),
+			       loc_string, mapped, sec_name, obj_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s of %s\n"),
-			       msym_name, offset, sec_name, obj_name);
+	      printf_filtered (_("%s in section %s of %s\n"),
+			       loc_string, sec_name, obj_name);
 	else
 	  if (pc_in_unmapped_range (addr, osect))
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in load address range of %s overlay "
+	      printf_filtered (_("%s in load address range of %s overlay "
 				 "section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in load address range of section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in load address range of section %s\n"),
+			       loc_string, sec_name);
 	  else
 	    if (section_is_overlay (osect))
-	      printf_filtered (_("%s + %u in %s overlay section %s\n"),
-			       msym_name, offset, mapped, sec_name);
+	      printf_filtered (_("%s in %s overlay section %s\n"),
+			       loc_string, mapped, sec_name);
 	    else
-	      printf_filtered (_("%s + %u in section %s\n"),
-			       msym_name, offset, sec_name);
+	      printf_filtered (_("%s in section %s\n"),
+			       loc_string, sec_name);
+
+	do_cleanups (old_chain);
       }
   }
   if (matches == 0)

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

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 11120 bytes --]

On Thu, Nov 20, 2008 at 12:03 AM, Doug Evans <dje@google.com> wrote:
> On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
>> Hi.  This patch was in progress when I wrote
>> http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
>> This patch subsumes that one.
>>
>> There are two things this patch does:
>> 1) properly pop dummy frames more often
>> 2) make the inferior resumable after an inferior function call gets a
>>   signal without having to remember to do "signal 0"
>>
>> 1) properly pop dummy frames more often
>>
>> Ulrich asked:
>>> I agree that it would be better if call_function_by_hand were to call
>>> dummy_frame_pop in this case.  However, why exactly is this a bug?
>>
>> It's a bug because it's confusing to not pop the frame in the places
>> where one is able to.  Plus it's, well, unclean.
>> I recognize that the dummy frame stack can't be precisely managed because
>> the user can do things like:
>>
>> set $orig_pc = $pc
>> set $orig_sp = $sp
>> break my_fun
>> call my_fun()
>> set $pc = $orig_pc
>> set $sp = $orig_sp
>> continue
>>
>> [This doesn't always work, but you get the idea.]
>> At the "continue", the inferior function call no longer exists and
>> gdb's mechanisms for removing the dummy frame from dummy_frame_stack
>> will never trigger (until the program is resumed and cleanup_dummy_frames
>> is called).  But we can still keep things clean
>> and unconfusing for the common case.
>>
>> 2) make the inferior resumable after an inferior function call gets a
>>   signal without having to remember to do "signal 0"
>>
>> If an inferior function call gets a signal (say SIGSEGV or SIGABRT),
>> one should be able to easily resume program execution after popping the stack.
>> It works today, but after manually popping the stack (e.g. with
>> "frame <dummy> ; return" where <dummy> is the dummy stack frame's number)
>> one has to remember to resume the program with "signal 0".
>> This is because stop_signal doesn't get restored.
>> Maybe there's a reason it shouldn't be, or maybe should be made an option,
>> but the current behaviour seems user-unfriendly for the normal case.
>>
>> Restoring stop_signal also has the benefit that if an inferior function
>> call is made after getting a signal, and the inferior function call
>> doesn't complete (e.g. has a breakpoint and doesn't complete right away),
>> the user can resume the program (after popping the inf fun call off the
>> stack) with "continue" without having to remember what the signal was
>> and remember to use "signal N".
>>
>> [BTW, IWBN if there was a command that did the equivalent of
>> "frame <dummy> ; return", named say "abandon", so that one didn't have
>> to go to the trouble of finding the dummy frame's stack number.
>> "abandon" would have the additional benefit that if the stack
>> was corrupted and one couldn't find the dummy frame, it would still
>> work since everything it needs is in dummy_frame_stack - it doesn't need
>> to examine the inferior's stack, except maybe for some sanity checking.
>> Continuing the inferior may still not be possible, but it does give the
>> user a more straightforward way to abandon an inferior function call
>> than exists today.]
>>
>> The bulk of the patch goes into making (2) work in a clean way.
>> Right now the inferior state that can be restored is a collection of
>> inferior_status, regcache, and misc. things like stop_pc (see the
>> assignment of stop_pc in normal_stop() when stop_stack_dummy).
>> The patch organizes the state into two pieces: inferior program state
>> (registers, stop_pc, stop_signal) and gdb session state
>> (the rest of inferior_status).
>> Program state is recorded on the dummy frame stack and is always
>> restored, either when the inferior function call completes or
>> when the stack frame is manually popped.
>> inferior_status contains the things that only get restored
>> if either the inferior function call successfully completes or
>> it gets a signal and unwindonsignal is set.
>>
>> P.S. I've removed one copy of the regcache.  Hopefully I've structured
>> things in a way that doesn't lose.
>>
>> NOTES:
>> - this adds the unwindonsignal.exp testcase from before, plus a new
>>  testcase that verifies one can resume the inferior after abandoning
>>  an inferior function call that gets a signal
>>
>> It's a big patch so presumably there are issues with it.
>> Comments?
>>
>> [tested on i386-linux and x86_64-linux]
>>
>> 2008-11-18  Doug Evans  <dje@google.com>
>>
>>        * dummy-frame.c (dummy_frame): Replace regcache member with
>>        caller_state.
>>        (dummy_frame_push): Replace caller_regcache arg with caller_state.
>>        Return pointer to created dummy frame.  All callers updated.
>>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame,
>>        assert_dummy_present,pop_dummy_frame_below,lookup_dummy_id,
>>        dummy_frame_discard,do_pop_dummy_frame_cleanup,
>>        make_cleanup_pop_dummy_frame): New functions.
>>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>>        dummy frame stack.  Restore program state.  Discard dummy frames below
>>        the one being deleted.
>>        (dummy_frame_sniffer): Update.
>>        * dummy-frame.h (dummy_frame_push): Update prototype.
>>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>>        DUMMY_FRAMEs.
>>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>>        New locals dummy_frame, dummy_frame_cleanup.
>>        Ensure dummy frame is popped/discarded for all possible exit paths.
>>        * inferior.h (inferior_program_state): Declare (opaque type).
>>        (save_inferior_program_state,restore_inferior_program_state,
>>        make_cleanup_restore_inferior_program_state,
>>        discard_inferior_program_state,
>>        get_inferior_program_state_regcache): Declare.
>>        (save_inferior_status): Update prototype.
>>        * infrun.c: #include "dummy-frame.h"
>>        (normal_stop): When stopped for the completion of an inferior function
>>        call, verify the expected stack frame kind and use dummy_frame_pop.
>>        (inferior_program_state): New struct.
>>        (save_inferior_program_state,restore_inferior_program_state,
>>        make_cleanup_restore_inferior_program_state,
>>        discard_inferior_program_state,
>>        get_inferior_program_state_regcache): New functions.
>>        (save_inferior_status): Remove arg restore_stack_info.
>>        All callers updated.  Remove saving of state now saved by
>>        save_inferior_program_state.
>>        (restore_inferior_status): Remove restoration of state now done by
>>        restore_inferior_program_state.
>>        (discard_inferior_status): Remove freeing of registers, now done by
>>        discard_inferior_program_state.
>>
>>        * gdb.base/call-signal-resume.exp: New file.
>>        * gdb.base/call-signals.c: New file.
>>        * gdb.base/unwindonsignal.exp: New file.
>
> ref: http://sourceware.org/ml/gdb-patches/2008-11/msg00454.html
>
> Blech.  I went too far in trying to keep dummy_frame_stack accurate.
> One can (theoretically) have multiple hand-function-calls outstanding
> in multiple threads (and soon in multiple processes presumably).
> Attached is a new version of the patch that goes back to having
> dummy_frame_pop only popping the requested frame (and similarily for
> dummy_frame_discard).
>
> I wrote a testcase that calls functions in multiple threads (with
> scheduler-locking on) by setting a breakpoint on the function being
> called.  I think there's a bug in scheduler-locking support as when I
> make the second call in the second thread, gdb makes the first thread
> step over the breakpoint and then resume, and control returns to
> call_function_by_hand with inferior_ptid changed to the first thread.
> To protect call_function_by_hand from this it now flags an error if
> the thread changes.
> [I'll submit the testcase separately once I can get it to pass, unless
> folks want it to see it.]
>
> How's this?
>
> 2008-11-18  Doug Evans  <dje@google.com>
>
>        * dummy-frame.c (dummy_frame): Replace regcache member with
>        caller_state.
>        (dummy_frame_push): Replace caller_regcache arg with caller_state.
>        Return pointer to created dummy frame.  All callers updated.
>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
>        lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
>        do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
>        functions.
>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>        dummy frame stack.  Restore program state.
>        (cleanup_dummy_frames): Rewrite.
>        (dummy_frame_sniffer): Update.
>        * dummy-frame.h (dummy_frame_push): Update prototype.
>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>        DUMMY_FRAMEs.
>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>        New locals dummy_frame, dummy_frame_cleanup, this_thread.
>        Ensure dummy frame is popped/discarded for all possible exit paths.
>        Detect program stopping in a different thread.
>        * inferior.h (inferior_program_state): Declare (opaque type).
>        (save_inferior_program_state,restore_inferior_program_state,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_state_regcache): Declare.
>        (save_inferior_status): Update prototype.
>        * infrun.c: #include "dummy-frame.h"
>        (normal_stop): When stopped for the completion of an inferior function
>        call, verify the expected stack frame kind and use dummy_frame_pop.
>        (inferior_program_state): New struct.
>        (save_inferior_program_state,restore_inferior_program_state,
>        do_restore_inferior_program_state_cleanup,
>        make_cleanup_restore_inferior_program_state,
>        discard_inferior_program_state,
>        get_inferior_program_state_regcache): New functions.
>        (inferior_status): Remove members stop_signal, stop_pc, registers,
>        restore_stack_info.
>        (save_inferior_status): Remove arg restore_stack_info.
>        All callers updated.  Remove saving of state now saved by
>        save_inferior_program_state.
>        (restore_inferior_status): Remove restoration of state now done by
>        restore_inferior_program_state.
>        (discard_inferior_status): Remove freeing of registers, now done by
>        discard_inferior_program_state.
>
>        * gdb.base/call-signal-resume.exp: New file.
>        * gdb.base/call-signals.c: New file.
>        * gdb.base/unwindonsignal.exp: New file.
>

Ummm, this time with the correct patch attached.  Sorry!

[-- Attachment #2: gdb-081119-infcall-2.patch.txt --]
[-- Type: text/plain, Size: 39770 bytes --]

2008-11-18  Doug Evans  <dje@google.com>

	* dummy-frame.c (dummy_frame): Replace regcache member with
	caller_state.
	(dummy_frame_push): Replace caller_regcache arg with caller_state.
	Return pointer to created dummy frame.  All callers updated.
	(remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
	lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
	do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
	functions.
	(dummy_frame_pop): Rewrite.  Verify requested frame is in the
	dummy frame stack.  Restore program state.
	(cleanup_dummy_frames): Rewrite.
	(dummy_frame_sniffer): Update.
	* dummy-frame.h (dummy_frame_push): Update prototype.
	(dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
	* frame.c (frame_pop): dummy_frame_pop now does all the work for
	DUMMY_FRAMEs.
	* infcall.c (call_function_by_hand): Replace caller_regcache,
	caller_regcache_cleanup with caller_state, caller_state_cleanup.
	New locals dummy_frame, dummy_frame_cleanup, this_thread.
	Ensure dummy frame is popped/discarded for all possible exit paths.
	Detect program stopping in a different thread.
	* inferior.h (inferior_program_state): Declare (opaque type).
	(save_inferior_program_state,restore_inferior_program_state,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): Declare.
	(save_inferior_status): Update prototype.
	* infrun.c: #include "dummy-frame.h"
	(normal_stop): When stopped for the completion of an inferior function
	call, verify the expected stack frame kind and use dummy_frame_pop.
	(inferior_program_state): New struct.
	(save_inferior_program_state,restore_inferior_program_state,
	do_restore_inferior_program_state_cleanup,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): New functions.
	(inferior_status): Remove members stop_signal, stop_pc, registers,
	restore_stack_info.
	(save_inferior_status): Remove arg restore_stack_info.
	All callers updated.  Remove saving of state now saved by
	save_inferior_program_state.
	(restore_inferior_status): Remove restoration of state now done by
	restore_inferior_program_state.
	(discard_inferior_status): Remove freeing of registers, now done by
	discard_inferior_program_state.

	* gdb.base/call-signal-resume.exp: New file.
	* gdb.base/call-signals.c: New file.
	* gdb.base/unwindonsignal.exp: New file.

Index: dummy-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.c,v
retrieving revision 1.53
diff -u -p -r1.53 dummy-frame.c
--- dummy-frame.c	30 Oct 2008 20:35:30 -0000	1.53
+++ dummy-frame.c	20 Nov 2008 07:09:58 -0000
@@ -42,8 +42,8 @@ struct dummy_frame
   /* This frame's ID.  Must match the value returned by
      gdbarch_dummy_id.  */
   struct frame_id id;
-  /* The caller's regcache.  */
-  struct regcache *regcache;
+  /* The caller's state prior to the call.  */
+  struct inferior_program_state *caller_state;
 };
 
 static struct dummy_frame *dummy_frame_stack = NULL;
@@ -81,61 +81,181 @@ deprecated_pc_in_call_dummy (CORE_ADDR p
   return 0;
 }
 
-/* Push the caller's state, along with the dummy frame info, onto a
+/* Push the caller's state, along with the dummy frame info, onto the
    dummy-frame stack.  */
 
-void
-dummy_frame_push (struct regcache *caller_regcache,
+struct dummy_frame *
+dummy_frame_push (struct inferior_program_state *caller_state,
 		  const struct frame_id *dummy_id)
 {
   struct dummy_frame *dummy_frame;
 
   dummy_frame = XZALLOC (struct dummy_frame);
-  dummy_frame->regcache = caller_regcache;
+  dummy_frame->caller_state = caller_state;
   dummy_frame->id = (*dummy_id);
   dummy_frame->next = dummy_frame_stack;
   dummy_frame_stack = dummy_frame;
+
+  return dummy_frame;
 }
 
-/* Pop the dummy frame with ID dummy_id from the dummy-frame stack.  */
+/* Remove *DUMMY_PTR from the dummy frame stack.  */
 
-void
-dummy_frame_pop (struct frame_id dummy_id)
+static void
+remove_dummy_frame (struct dummy_frame **dummy_ptr)
+{
+  struct dummy_frame *dummy = *dummy_ptr;
+
+  *dummy_ptr = dummy->next;
+  discard_inferior_program_state (dummy->caller_state);
+  xfree (dummy);
+}
+
+/* Cleanup handler for dummy_frame_pop.  */
+
+static void
+do_dummy_frame_cleanup (void *arg)
 {
-  struct dummy_frame **dummy_ptr;
+  struct dummy_frame **dummy_ptr = arg;
+
+  remove_dummy_frame (dummy_ptr);
+}
+
+/* Pop *DUMMY_PTR, restoring program state to that before the
+   frame was created.  */
+
+static void
+pop_dummy_frame_from_ptr (struct dummy_frame **dummy_ptr)
+{
+  struct cleanup *cleanups;
+  struct dummy_frame *dummy;
+
+  cleanups = make_cleanup (do_dummy_frame_cleanup, dummy_ptr);
 
-  for (dummy_ptr = &dummy_frame_stack;
-       (*dummy_ptr) != NULL;
-       dummy_ptr = &(*dummy_ptr)->next)
+  restore_inferior_program_state ((*dummy_ptr)->caller_state);
+
+  /* restore_inferior_status frees inf_status,
+     all that remains is to pop *dummy_ptr */
+  discard_cleanups (cleanups);
+  dummy = *dummy_ptr;
+  *dummy_ptr = dummy->next;
+  xfree (dummy);
+
+  /* We've made right mess of GDB's local state, just discard
+     everything.  */
+  reinit_frame_cache ();
+}
+
+/* Return a pointer to DUMMY's entry in the dummy frame stack.
+   Returns NULL if DUMMY is not present.  */
+
+static struct dummy_frame **
+lookup_dummy (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  for (dp = &dummy_frame_stack; *dp != NULL; dp = &(*dp)->next)
     {
-      struct dummy_frame *dummy = *dummy_ptr;
-      if (frame_id_eq (dummy->id, dummy_id))
-	{
-	  *dummy_ptr = dummy->next;
-	  regcache_xfree (dummy->regcache);
-	  xfree (dummy);
-	  break;
-	}
+      if (*dp == dummy)
+	return dp;
     }
+
+  return NULL;
 }
 
-/* There may be stale dummy frames, perhaps left over from when a longjump took us
-   out of a function that was called by the debugger.  Clean them up at least once
-   whenever we start a new inferior.  */
+/* Look up DUMMY_ID.
+   Return NULL if not found.  */
 
-static void
-cleanup_dummy_frames (struct target_ops *target, int from_tty)
+static struct dummy_frame **
+lookup_dummy_id (struct frame_id dummy_id)
 {
-  struct dummy_frame *dummy, *next;
+  struct dummy_frame **dp;
 
-  for (dummy = dummy_frame_stack; dummy; dummy = next)
+  for (dp = &dummy_frame_stack; *dp != NULL; dp = &(*dp)->next)
     {
-      next = dummy->next;
-      regcache_xfree (dummy->regcache);
-      xfree (dummy);
+      if (frame_id_eq ((*dp)->id, dummy_id))
+	return dp;
     }
 
-  dummy_frame_stack = NULL;
+  return NULL;
+}
+
+/* Pop DUMMY, restore inferior state.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.  */
+
+static void
+pop_dummy_frame (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy (dummy);
+  gdb_assert (dp != NULL);
+
+  pop_dummy_frame_from_ptr (dp);
+}
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.
+
+   NOTE: This can only pop the one frame, even if it is in the middle of the
+   stack, because the other frames may be for different threads, and there's
+   currently no way to tell which stack frame is for which thread.  */
+
+void
+dummy_frame_pop (struct frame_id dummy_id)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy_id (dummy_id);
+  gdb_assert (dp != NULL);
+
+  pop_dummy_frame_from_ptr (dp);
+}
+
+/* Discard DUMMY and remove it from the dummy frame stack.
+   If the frame isn't found, flag an internal error.  */
+
+void
+dummy_frame_discard (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy (dummy);
+  gdb_assert (dp != NULL);
+
+  remove_dummy_frame (dp);
+}
+
+/* Utility for make_cleanup_pop_dummy_frame.  */
+
+static void
+do_pop_dummy_frame_cleanup (void *dummy)
+{
+  pop_dummy_frame (dummy);
+}
+
+/* Schedule a cleanup to pop DUMMY_FRAME and restore inferior state.  */
+
+struct cleanup *
+make_cleanup_pop_dummy_frame (struct dummy_frame *dummy)
+{
+  return make_cleanup (do_pop_dummy_frame_cleanup, dummy);
+}
+
+/* There may be stale dummy frames, perhaps left over from when a longjump took
+   us out of a function that was called by the debugger.  Clean them up at
+   least once whenever we start a new inferior.  */
+
+static void
+cleanup_dummy_frames (struct target_ops *target, int from_tty)
+{
+  while (dummy_frame_stack != NULL)
+    {
+      remove_dummy_frame (&dummy_frame_stack);
+    }
 }
 
 /* Return the dummy frame cache, it contains both the ID, and a
@@ -178,7 +298,7 @@ dummy_frame_sniffer (const struct frame_
 	    {
 	      struct dummy_frame_cache *cache;
 	      cache = FRAME_OBSTACK_ZALLOC (struct dummy_frame_cache);
-	      cache->prev_regcache = dummyframe->regcache;
+	      cache->prev_regcache = get_inferior_program_state_regcache (dummyframe->caller_state);
 	      cache->this_id = this_id;
 	      (*this_prologue_cache) = cache;
 	      return 1;
@@ -215,7 +335,7 @@ dummy_frame_prev_register (struct frame_
   return reg_val;
 }
 
-/* Assuming that THIS frame is a dummy, return the ID of THIS frame.  That ID is
+/* Assuming that THIS_FRAME is a dummy, return its ID.  That ID is
    determined by examining the NEXT frame's unwound registers using
    the method dummy_id().  As a side effect, THIS dummy frame's
    dummy cache is located and and saved in THIS_PROLOGUE_CACHE.  */
Index: dummy-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.h,v
retrieving revision 1.23
diff -u -p -r1.23 dummy-frame.h
--- dummy-frame.h	8 Sep 2008 15:23:12 -0000	1.23
+++ dummy-frame.h	20 Nov 2008 07:09:58 -0000
@@ -23,8 +23,10 @@
 #include "frame.h"
 
 struct frame_info;
-struct regcache;
+struct inferior_program_state;
 struct frame_unwind;
+struct dummy_frame;
+struct cleanup;
 
 /* Push the information needed to identify, and unwind from, a dummy
    frame onto the dummy frame stack.  */
@@ -39,11 +41,29 @@ struct frame_unwind;
    be expanded so that it knowns the lower/upper extent of the dummy
    frame's code.  */
 
-extern void dummy_frame_push (struct regcache *regcache,
-			      const struct frame_id *dummy_id);
+extern struct dummy_frame *dummy_frame_push (struct inferior_program_state *caller_state,
+					     const struct frame_id *dummy_id);
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.
+
+   NOTE: This can only pop the one frame, even if it is in the middle of the
+   stack, because the other frames may be for different threads, and there's
+   currently no way to tell which stack frame is for which thread.  */
 
 extern void dummy_frame_pop (struct frame_id dummy_id);
 
+/* Discard DUMMY and remove it from the dummy frame stack.
+   If the frame isn't found, flag an internal error.  */
+
+extern void dummy_frame_discard (struct dummy_frame *dummy);
+
+/* Schedule a cleanup to pop DUMMY_FRAME.  */
+
+extern struct cleanup *make_cleanup_pop_dummy_frame (struct dummy_frame *);
+
 /* If the PC falls in a dummy frame, return a dummy frame
    unwinder.  */
 
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.255
diff -u -p -r1.255 frame.c
--- frame.c	24 Sep 2008 12:59:49 -0000	1.255
+++ frame.c	20 Nov 2008 07:09:58 -0000
@@ -536,6 +536,14 @@ frame_pop (struct frame_info *this_frame
   struct regcache *scratch;
   struct cleanup *cleanups;
 
+  if (get_frame_type (this_frame) == DUMMY_FRAME)
+    {
+      /* Popping a dummy frame involves restoring more than just registers.
+	 dummy_frame_pop does all the work.  */
+      dummy_frame_pop (get_frame_id (this_frame));
+      return;
+    }
+
   /* Ensure that we have a frame to pop to.  */
   prev_frame = get_prev_frame_1 (this_frame);
 
@@ -549,11 +557,6 @@ frame_pop (struct frame_info *this_frame
   scratch = frame_save_as_regcache (prev_frame);
   cleanups = make_cleanup_regcache_xfree (scratch);
 
-  /* If we are popping a dummy frame, clean up the associated
-     data as well.  */
-  if (get_frame_type (this_frame) == DUMMY_FRAME)
-    dummy_frame_pop (get_frame_id (this_frame));
-
   /* FIXME: cagney/2003-03-16: It should be possible to tell the
      target's register cache that it is about to be hit with a burst
      register transfer and that the sequence of register writes should
Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.106
diff -u -p -r1.106 infcall.c
--- infcall.c	18 Nov 2008 00:13:02 -0000	1.106
+++ infcall.c	20 Nov 2008 07:09:58 -0000
@@ -318,16 +318,19 @@ call_function_by_hand (struct value *fun
   struct cleanup *retbuf_cleanup;
   struct inferior_status *inf_status;
   struct cleanup *inf_status_cleanup;
+  struct inferior_program_state *caller_state;
+  struct cleanup *caller_state_cleanup;
+  struct dummy_frame *dummy_frame;
+  struct cleanup *dummy_frame_cleanup;
   CORE_ADDR funaddr;
   CORE_ADDR real_pc;
   struct type *ftype = check_typedef (value_type (function));
   CORE_ADDR bp_addr;
-  struct regcache *caller_regcache;
-  struct cleanup *caller_regcache_cleanup;
   struct frame_id dummy_id;
   struct cleanup *args_cleanup;
   struct frame_info *frame;
   struct gdbarch *gdbarch;
+  struct thread_info *this_thread;
 
   if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -335,6 +338,7 @@ call_function_by_hand (struct value *fun
   if (!target_has_execution)
     noprocess ();
 
+  this_thread = inferior_thread ();
   frame = get_current_frame ();
   gdbarch = get_frame_arch (frame);
 
@@ -351,15 +355,16 @@ call_function_by_hand (struct value *fun
   /* A cleanup for the inferior status.  Create this AFTER the retbuf
      so that this can be discarded or applied without interfering with
      the regbuf.  */
-  inf_status = save_inferior_status (1);
+  inf_status = save_inferior_status ();
   inf_status_cleanup = make_cleanup_restore_inferior_status (inf_status);
 
-  /* Save the caller's registers so that they can be restored once the
+  /* Save the caller's registers and other state associated with the
+     inferior itself so that they can be restored once the
      callee returns.  To allow nested calls the registers are (further
      down) pushed onto a dummy frame stack.  Include a cleanup (which
      is tossed once the regcache has been pushed).  */
-  caller_regcache = frame_save_as_regcache (frame);
-  caller_regcache_cleanup = make_cleanup_regcache_xfree (caller_regcache);
+  caller_state = save_inferior_program_state ();
+  caller_state_cleanup = make_cleanup_restore_inferior_program_state (caller_state);
 
   /* Ensure that the initial SP is correctly aligned.  */
   {
@@ -642,8 +647,10 @@ call_function_by_hand (struct value *fun
   /* 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);
 
   /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP -
      If you're looking to implement asynchronous dummy-frames, then
@@ -671,7 +678,6 @@ call_function_by_hand (struct value *fun
     struct cleanup *old_cleanups = make_cleanup (null_cleanup, 0);
     struct cleanup *old_cleanups2;
     int saved_async = 0;
-    struct thread_info *tp = inferior_thread ();
 
     /* If all error()s out of proceed ended up calling normal_stop
        (and perhaps they should; it already does in the special case
@@ -679,7 +685,7 @@ call_function_by_hand (struct value *fun
     make_cleanup (breakpoint_auto_delete_contents, NULL);
 
     disable_watchpoints_before_interactive_call_start ();
-    tp->proceed_to_finish = 1;	/* We want stop_registers, please... */
+    this_thread->proceed_to_finish = 1; /* We want stop_registers, please... */
 
     if (target_can_async_p ())
       saved_async = target_async_mask (0);
@@ -690,12 +696,12 @@ call_function_by_hand (struct value *fun
     suppress_stop_observer = 1;
     proceed (real_pc, TARGET_SIGNAL_0, 0);
     do_cleanups (old_cleanups2);
-    
+
     if (saved_async)
       target_async_mask (saved_async);
-    
+
     enable_watchpoints_after_interactive_call_stop ();
-      
+
     discard_cleanups (old_cleanups);
   }
 
@@ -705,10 +711,27 @@ call_function_by_hand (struct value *fun
 	 we'll crash as the inferior is no longer running.  */
       discard_cleanups (inf_status_cleanup);
       discard_inferior_status (inf_status);
+      dummy_frame_discard (dummy_frame);
       error (_("\
 The program being debugged exited while in a function called from GDB."));
     }
 
+  if (! ptid_equal (this_thread->ptid, inferior_thread ()->ptid))
+    {
+      /* We've switched threads.  Perhaps this shouldn't happen, but we
+	 protect ourselves anyway.
+	 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);
+      dummy_frame_discard (dummy_frame);
+      error (_("\
+The current thread has switched 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."));
+    }
+
   if (stopped_by_random_signal || !stop_stack_dummy)
     {
       /* Find the name of the function we're about to complain about.  */
@@ -745,9 +768,10 @@ The program being debugged exited while 
 	    {
 	      /* The user wants the context restored. */
 
-	      /* We must get back to the frame we were before the
-		 dummy call. */
-	      frame_pop (get_current_frame ());
+	      /* We must get back to the frame we were before the dummy call.
+		 Plus we need to restore inferior status to that before the
+		 dummy call.  This is all handled by cleanups
+		 dummy_frame_cleanup and inf_status_cleanup.  */
 
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
@@ -761,14 +785,16 @@ Evaluation of the expression containing 
 	  else
 	    {
 	      /* The user wants to stay in the frame where we stopped
-                 (default).*/
-	      /* If we restored the inferior status (via the cleanup),
+		 (default).
+		 If we restored the inferior status (via the cleanup),
 		 we would print a spurious error message (Unable to
-		 restore previously selected frame), would write the
-		 registers from the inf_status (which is wrong), and
-		 would do other wrong things.  */
+		 restore previously selected frame), and
+		 would do other wrong things.
+		 Discarding inf_status_cleanup also discards
+		 dummy_frame_cleanup.  */
 	      discard_cleanups (inf_status_cleanup);
 	      discard_inferior_status (inf_status);
+
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
 	      error (_("\
@@ -782,14 +808,17 @@ Evaluation of the expression containing 
 
       if (!stop_stack_dummy)
 	{
-	  /* We hit a breakpoint inside the FUNCTION. */
-	  /* If we restored the inferior status (via the cleanup), we
+	  /* We hit a breakpoint inside the FUNCTION.
+	     If we restored the inferior status (via the cleanup), we
 	     would print a spurious error message (Unable to restore
 	     previously selected frame), would write the registers
 	     from the inf_status (which is wrong), and would do other
-	     wrong things.  */
+	     wrong things.
+	     Discarding inf_status_cleanup also discards
+	     dummy_frame_cleanup.  */
 	  discard_cleanups (inf_status_cleanup);
 	  discard_inferior_status (inf_status);
+
 	  /* The following error message used to say "The expression
 	     which contained the function call has been discarded."
 	     It is a hard concept to explain in a few words.  Ideally,
@@ -811,6 +840,9 @@ the function call)."), name);
 
   /* If we get here the called FUNCTION run to completion. */
 
+  /* The dummy frame has been popped so discard its cleanup.  */
+  discard_cleanups (dummy_frame_cleanup);
+
   /* On normal return, the stack dummy has been popped already.  */
   regcache_cpy_no_passthrough (retbuf, stop_registers);
 
Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.116
diff -u -p -r1.116 inferior.h
--- inferior.h	20 Nov 2008 00:35:23 -0000	1.116
+++ inferior.h	20 Nov 2008 07:09:58 -0000
@@ -40,24 +40,40 @@ struct ui_out;
 /* For struct frame_id.  */
 #include "frame.h"
 
-/* Structure in which to save the status of the inferior.  Create/Save
-   through "save_inferior_status", restore through
-   "restore_inferior_status".
-
-   This pair of routines should be called around any transfer of
-   control to the inferior which you don't want showing up in your
-   control variables.  */
+/* 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.
+
+   Call these routines around hand called functions, including function calls
+   in conditional breakpoints for example.  */
+
+struct inferior_program_state;
 struct inferior_status;
 
-extern struct inferior_status *save_inferior_status (int);
+extern struct inferior_program_state *save_inferior_program_state (void);
+extern struct inferior_status *save_inferior_status (void);
 
+extern void restore_inferior_program_state (struct inferior_program_state *);
 extern void restore_inferior_status (struct inferior_status *);
 
+extern struct cleanup *make_cleanup_restore_inferior_program_state (struct inferior_program_state *);
 extern struct cleanup *make_cleanup_restore_inferior_status (struct inferior_status *);
 
+extern void discard_inferior_program_state (struct inferior_program_state *);
 extern void discard_inferior_status (struct inferior_status *);
 
+extern struct regcache *get_inferior_program_state_regcache (struct inferior_program_state *);
+
 /* The -1 ptid, often used to indicate either an error condition
    or a "don't care" condition, i.e, "run all threads."  */
 extern ptid_t minus_one_ptid;
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.339
diff -u -p -r1.339 infrun.c
--- infrun.c	20 Nov 2008 00:35:23 -0000	1.339
+++ infrun.c	20 Nov 2008 07:09:58 -0000
@@ -45,7 +45,7 @@
 #include "language.h"
 #include "solib.h"
 #include "main.h"
-
+#include "dummy-frame.h"
 #include "gdb_assert.h"
 #include "mi/mi-common.h"
 #include "event-top.h"
@@ -4342,15 +4342,23 @@ Further execution is probably impossible
 
   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"));
+	}
     }
 
 done:
@@ -4746,10 +4754,86 @@ signals_info (char *signum_exp, int from
   printf_filtered (_("\nUse the \"handle\" command to change these tables.\n"));
 }
 \f
-struct inferior_status
+/* Inferior program state.
+   These are details related to the inferior itself, and don't include
+   things like what frame the user had selected or what gdb was doing
+   with the target at the time.
+   For inferior function calls these are things we want to restore
+   regardless of whether the function call successfully completes
+   or the dummy frame has to be manually popped.  */
+
+struct inferior_program_state
 {
   enum target_signal stop_signal;
   CORE_ADDR stop_pc;
+  struct regcache *registers;
+};
+
+struct inferior_program_state *
+save_inferior_program_state ()
+{
+  struct inferior_program_state *inf_state = XMALLOC (struct inferior_program_state);
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  inf_state->stop_signal = tp->stop_signal;
+  inf_state->stop_pc = stop_pc;
+
+  inf_state->registers = regcache_dup (get_current_regcache ());
+
+  return inf_state;
+}
+
+/* Restore inferior session state to INF_STATE.  */
+
+void
+restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  tp->stop_signal = inf_state->stop_signal;
+  stop_pc = inf_state->stop_pc;
+
+  /* The inferior can be gone if the user types "print exit(0)"
+     (and perhaps other times).  */
+  if (target_has_execution)
+    /* NB: The register write goes through to the target.  */
+    regcache_cpy (get_current_regcache (), inf_state->registers);
+  regcache_xfree (inf_state->registers);
+}
+
+static void
+do_restore_inferior_program_state_cleanup (void *state)
+{
+  restore_inferior_program_state (state);
+}
+
+struct cleanup *
+make_cleanup_restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  return make_cleanup (do_restore_inferior_program_state_cleanup, inf_state);
+}
+
+void
+discard_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  regcache_xfree (inf_state->registers);
+  xfree (inf_state);
+}
+
+struct regcache *
+get_inferior_program_state_regcache (struct inferior_program_state *inf_state)
+{
+  return inf_state->registers;
+}
+
+/* Session related state for inferior function calls.
+   These are the additional bits of state that need to be restored
+   when an inferior function call successfully completes.  */
+
+struct inferior_status
+{
   bpstat stop_bpstat;
   int stop_step;
   int stop_stack_dummy;
@@ -4763,32 +4847,23 @@ struct inferior_status
   int stop_after_trap;
   int stop_soon;
 
-  /* These are here because if call_function_by_hand has written some
-     registers and then decides to call error(), we better not have changed
-     any registers.  */
-  struct regcache *registers;
-
-  /* A frame unique identifier.  */
+  /* ID if the selected frame when the inferior function call was made.  */
   struct frame_id selected_frame_id;
 
   int breakpoint_proceeded;
-  int restore_stack_info;
   int proceed_to_finish;
 };
 
 /* Save all of the information associated with the inferior<==>gdb
-   connection.  INF_STATUS is a pointer to a "struct inferior_status"
-   (defined in inferior.h).  */
+   connection.  */
 
 struct inferior_status *
-save_inferior_status (int restore_stack_info)
+save_inferior_status ()
 {
   struct inferior_status *inf_status = XMALLOC (struct inferior_status);
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  inf_status->stop_signal = tp->stop_signal;
-  inf_status->stop_pc = stop_pc;
   inf_status->stop_step = tp->stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
@@ -4806,12 +4881,10 @@ save_inferior_status (int restore_stack_
   inf_status->stop_bpstat = tp->stop_bpstat;
   tp->stop_bpstat = bpstat_copy (tp->stop_bpstat);
   inf_status->breakpoint_proceeded = breakpoint_proceeded;
-  inf_status->restore_stack_info = restore_stack_info;
   inf_status->proceed_to_finish = tp->proceed_to_finish;
 
-  inf_status->registers = regcache_dup (get_current_regcache ());
-
   inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL));
+
   return inf_status;
 }
 
@@ -4836,14 +4909,14 @@ restore_selected_frame (void *args)
   return (1);
 }
 
+/* Restore inferior session state to INF_STATUS.  */
+
 void
 restore_inferior_status (struct inferior_status *inf_status)
 {
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  tp->stop_signal = inf_status->stop_signal;
-  stop_pc = inf_status->stop_pc;
   tp->stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
@@ -4856,24 +4929,11 @@ restore_inferior_status (struct inferior
   inf->stop_soon = inf_status->stop_soon;
   bpstat_clear (&tp->stop_bpstat);
   tp->stop_bpstat = inf_status->stop_bpstat;
+  inf_status->stop_bpstat = NULL;
   breakpoint_proceeded = inf_status->breakpoint_proceeded;
   tp->proceed_to_finish = inf_status->proceed_to_finish;
 
-  /* The inferior can be gone if the user types "print exit(0)"
-     (and perhaps other times).  */
-  if (target_has_execution)
-    /* NB: The register write goes through to the target.  */
-    regcache_cpy (get_current_regcache (), inf_status->registers);
-  regcache_xfree (inf_status->registers);
-
-  /* FIXME: If we are being called after stopping in a function which
-     is called from gdb, we should not be trying to restore the
-     selected frame; it just prints a spurious error message (The
-     message is useful, however, in detecting bugs in gdb (like if gdb
-     clobbers the stack)).  In fact, should we be restoring the
-     inferior status at all in that case?  .  */
-
-  if (target_has_stack && inf_status->restore_stack_info)
+  if (target_has_stack)
     {
       /* The point of catch_errors is that if the stack is clobbered,
          walking the stack might encounter a garbage pointer and
@@ -4885,7 +4945,6 @@ restore_inferior_status (struct inferior
 	/* Error in restoring the selected frame.  Select the innermost
 	   frame.  */
 	select_frame (get_current_frame ());
-
     }
 
   xfree (inf_status);
@@ -4908,10 +4967,9 @@ discard_inferior_status (struct inferior
 {
   /* See save_inferior_status for info on stop_bpstat. */
   bpstat_clear (&inf_status->stop_bpstat);
-  regcache_xfree (inf_status->registers);
   xfree (inf_status);
 }
-
+\f
 int
 inferior_has_forked (ptid_t pid, ptid_t *child_pid)
 {
Index: testsuite/gdb.base/call-signal-resume.exp
===================================================================
RCS file: testsuite/gdb.base/call-signal-resume.exp
diff -N testsuite/gdb.base/call-signal-resume.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signal-resume.exp	20 Nov 2008 07:09:59 -0000
@@ -0,0 +1,107 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping call-signal-resume.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested call-signal-resume.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+proc get_dummy_frame_number { } {
+  global gdb_prompt
+
+  send_gdb "bt\n"
+  gdb_expect {
+    -re "#(\[0-9\]*) *<function called from gdb>.*$gdb_prompt $"
+      {
+	return $expect_out(1,string)
+      }
+    -re "$gdb_prompt $"
+      {
+	return ""
+      }
+    timeout
+      {
+	return ""
+      }
+  }
+  return ""
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"call-signal-resume, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "call-signal-resume, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "call-signal-resume, inferior function call signaled"
+    }
+}
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "call-signal-resume, dummy stack frame number"
+    setup_xfail "*-*-*"
+} else {
+    pass "call-signal-resume, dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Resume execution, the program should successfully complete.
+gdb_test "continue" "Program exited normally."
+
+return 0
Index: testsuite/gdb.base/call-signals.c
===================================================================
RCS file: testsuite/gdb.base/call-signals.c
diff -N testsuite/gdb.base/call-signals.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signals.c	20 Nov 2008 07:09:59 -0000
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing handling of inferior function calls
+   in the presence of signals.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported,
+     so try that first.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+#ifdef SIGSEGV
+  kill (getpid (), SIGSEGV);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+  return 0;
+}
Index: testsuite/gdb.base/unwindonsignal.exp
===================================================================
RCS file: testsuite/gdb.base/unwindonsignal.exp
diff -N testsuite/gdb.base/unwindonsignal.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/unwindonsignal.exp	20 Nov 2008 07:09:59 -0000
@@ -0,0 +1,94 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping unwindonsignal.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested unwindonsignal.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+# Turn on unwindonsignal.
+gdb_test "set unwindonsignal on" \
+	"" \
+	"setting unwindonsignal"
+gdb_test "show unwindonsignal" \
+	"Unwinding of stack .* is on." \
+	"showing unwindonsignal"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"unwindonsignal, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "unwindonsignal, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, inferior function call signaled"
+    }
+}
+
+# Verify the stack got unwound.
+gdb_test "bt" \
+	"#0 *main \\(.*\\) at .*" \
+	"unwindonsignal, stack unwound"
+
+# Verify the dummy frame got removed from dummy_frame_stack.
+gdb_test_multiple "maint print dummy-frames" \
+	"unwindonsignal, dummy frame removed" {
+    -re "\[\r\n\]*.*stack=.*code=.*\[\r\n\]+$gdb_prompt $" {
+	fail "unwindonsignal, dummy frame removed"
+    }
+    -re "\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, dummy frame removed"
+    }
+}
+
+return 0

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

[Doug Evans]

Ping.

On Thu, Nov 20, 2008 at 12:07 AM, Doug Evans <dje@google.com> wrote:
> On Thu, Nov 20, 2008 at 12:03 AM, Doug Evans <dje@google.com> wrote:
>> On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
>>> Hi.  This patch was in progress when I wrote
>>> http://sourceware.org/ml/gdb-patches/2008-11/msg00391.html
>>> This patch subsumes that one.
>>>
>>> There are two things this patch does:
>>> 1) properly pop dummy frames more often
>>> 2) make the inferior resumable after an inferior function call gets a
>>>   signal without having to remember to do "signal 0"
>>>
>>> 1) properly pop dummy frames more often
>>>
>>> Ulrich asked:
>>>> I agree that it would be better if call_function_by_hand were to call
>>>> dummy_frame_pop in this case.  However, why exactly is this a bug?
>>>
>>> It's a bug because it's confusing to not pop the frame in the places
>>> where one is able to.  Plus it's, well, unclean.
>>> I recognize that the dummy frame stack can't be precisely managed because
>>> the user can do things like:
>>>
>>> set $orig_pc = $pc
>>> set $orig_sp = $sp
>>> break my_fun
>>> call my_fun()
>>> set $pc = $orig_pc
>>> set $sp = $orig_sp
>>> continue
>>>
>>> [This doesn't always work, but you get the idea.]
>>> At the "continue", the inferior function call no longer exists and
>>> gdb's mechanisms for removing the dummy frame from dummy_frame_stack
>>> will never trigger (until the program is resumed and cleanup_dummy_frames
>>> is called).  But we can still keep things clean
>>> and unconfusing for the common case.
>>>
>>> 2) make the inferior resumable after an inferior function call gets a
>>>   signal without having to remember to do "signal 0"
>>>
>>> If an inferior function call gets a signal (say SIGSEGV or SIGABRT),
>>> one should be able to easily resume program execution after popping the stack.
>>> It works today, but after manually popping the stack (e.g. with
>>> "frame <dummy> ; return" where <dummy> is the dummy stack frame's number)
>>> one has to remember to resume the program with "signal 0".
>>> This is because stop_signal doesn't get restored.
>>> Maybe there's a reason it shouldn't be, or maybe should be made an option,
>>> but the current behaviour seems user-unfriendly for the normal case.
>>>
>>> Restoring stop_signal also has the benefit that if an inferior function
>>> call is made after getting a signal, and the inferior function call
>>> doesn't complete (e.g. has a breakpoint and doesn't complete right away),
>>> the user can resume the program (after popping the inf fun call off the
>>> stack) with "continue" without having to remember what the signal was
>>> and remember to use "signal N".
>>>
>>> [BTW, IWBN if there was a command that did the equivalent of
>>> "frame <dummy> ; return", named say "abandon", so that one didn't have
>>> to go to the trouble of finding the dummy frame's stack number.
>>> "abandon" would have the additional benefit that if the stack
>>> was corrupted and one couldn't find the dummy frame, it would still
>>> work since everything it needs is in dummy_frame_stack - it doesn't need
>>> to examine the inferior's stack, except maybe for some sanity checking.
>>> Continuing the inferior may still not be possible, but it does give the
>>> user a more straightforward way to abandon an inferior function call
>>> than exists today.]
>>>
>>> The bulk of the patch goes into making (2) work in a clean way.
>>> Right now the inferior state that can be restored is a collection of
>>> inferior_status, regcache, and misc. things like stop_pc (see the
>>> assignment of stop_pc in normal_stop() when stop_stack_dummy).
>>> The patch organizes the state into two pieces: inferior program state
>>> (registers, stop_pc, stop_signal) and gdb session state
>>> (the rest of inferior_status).
>>> Program state is recorded on the dummy frame stack and is always
>>> restored, either when the inferior function call completes or
>>> when the stack frame is manually popped.
>>> inferior_status contains the things that only get restored
>>> if either the inferior function call successfully completes or
>>> it gets a signal and unwindonsignal is set.
>>>
>>> P.S. I've removed one copy of the regcache.  Hopefully I've structured
>>> things in a way that doesn't lose.
>>>
>>> NOTES:
>>> - this adds the unwindonsignal.exp testcase from before, plus a new
>>>  testcase that verifies one can resume the inferior after abandoning
>>>  an inferior function call that gets a signal
>>>
>>> It's a big patch so presumably there are issues with it.
>>> Comments?
>>>
>>> [tested on i386-linux and x86_64-linux]
>>>
>>> 2008-11-18  Doug Evans  <dje@google.com>
>>>
>>>        * dummy-frame.c (dummy_frame): Replace regcache member with
>>>        caller_state.
>>>        (dummy_frame_push): Replace caller_regcache arg with caller_state.
>>>        Return pointer to created dummy frame.  All callers updated.
>>>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame,
>>>        assert_dummy_present,pop_dummy_frame_below,lookup_dummy_id,
>>>        dummy_frame_discard,do_pop_dummy_frame_cleanup,
>>>        make_cleanup_pop_dummy_frame): New functions.
>>>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>>>        dummy frame stack.  Restore program state.  Discard dummy frames below
>>>        the one being deleted.
>>>        (dummy_frame_sniffer): Update.
>>>        * dummy-frame.h (dummy_frame_push): Update prototype.
>>>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>>>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>>>        DUMMY_FRAMEs.
>>>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>>>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>>>        New locals dummy_frame, dummy_frame_cleanup.
>>>        Ensure dummy frame is popped/discarded for all possible exit paths.
>>>        * inferior.h (inferior_program_state): Declare (opaque type).
>>>        (save_inferior_program_state,restore_inferior_program_state,
>>>        make_cleanup_restore_inferior_program_state,
>>>        discard_inferior_program_state,
>>>        get_inferior_program_state_regcache): Declare.
>>>        (save_inferior_status): Update prototype.
>>>        * infrun.c: #include "dummy-frame.h"
>>>        (normal_stop): When stopped for the completion of an inferior function
>>>        call, verify the expected stack frame kind and use dummy_frame_pop.
>>>        (inferior_program_state): New struct.
>>>        (save_inferior_program_state,restore_inferior_program_state,
>>>        make_cleanup_restore_inferior_program_state,
>>>        discard_inferior_program_state,
>>>        get_inferior_program_state_regcache): New functions.
>>>        (save_inferior_status): Remove arg restore_stack_info.
>>>        All callers updated.  Remove saving of state now saved by
>>>        save_inferior_program_state.
>>>        (restore_inferior_status): Remove restoration of state now done by
>>>        restore_inferior_program_state.
>>>        (discard_inferior_status): Remove freeing of registers, now done by
>>>        discard_inferior_program_state.
>>>
>>>        * gdb.base/call-signal-resume.exp: New file.
>>>        * gdb.base/call-signals.c: New file.
>>>        * gdb.base/unwindonsignal.exp: New file.
>>
>> ref: http://sourceware.org/ml/gdb-patches/2008-11/msg00454.html
>>
>> Blech.  I went too far in trying to keep dummy_frame_stack accurate.
>> One can (theoretically) have multiple hand-function-calls outstanding
>> in multiple threads (and soon in multiple processes presumably).
>> Attached is a new version of the patch that goes back to having
>> dummy_frame_pop only popping the requested frame (and similarily for
>> dummy_frame_discard).
>>
>> I wrote a testcase that calls functions in multiple threads (with
>> scheduler-locking on) by setting a breakpoint on the function being
>> called.  I think there's a bug in scheduler-locking support as when I
>> make the second call in the second thread, gdb makes the first thread
>> step over the breakpoint and then resume, and control returns to
>> call_function_by_hand with inferior_ptid changed to the first thread.
>> To protect call_function_by_hand from this it now flags an error if
>> the thread changes.
>> [I'll submit the testcase separately once I can get it to pass, unless
>> folks want it to see it.]
>>
>> How's this?
>>
>> 2008-11-18  Doug Evans  <dje@google.com>
>>
>>        * dummy-frame.c (dummy_frame): Replace regcache member with
>>        caller_state.
>>        (dummy_frame_push): Replace caller_regcache arg with caller_state.
>>        Return pointer to created dummy frame.  All callers updated.
>>        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
>>        lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
>>        do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
>>        functions.
>>        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
>>        dummy frame stack.  Restore program state.
>>        (cleanup_dummy_frames): Rewrite.
>>        (dummy_frame_sniffer): Update.
>>        * dummy-frame.h (dummy_frame_push): Update prototype.
>>        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
>>        * frame.c (frame_pop): dummy_frame_pop now does all the work for
>>        DUMMY_FRAMEs.
>>        * infcall.c (call_function_by_hand): Replace caller_regcache,
>>        caller_regcache_cleanup with caller_state, caller_state_cleanup.
>>        New locals dummy_frame, dummy_frame_cleanup, this_thread.
>>        Ensure dummy frame is popped/discarded for all possible exit paths.
>>        Detect program stopping in a different thread.
>>        * inferior.h (inferior_program_state): Declare (opaque type).
>>        (save_inferior_program_state,restore_inferior_program_state,
>>        make_cleanup_restore_inferior_program_state,
>>        discard_inferior_program_state,
>>        get_inferior_program_state_regcache): Declare.
>>        (save_inferior_status): Update prototype.
>>        * infrun.c: #include "dummy-frame.h"
>>        (normal_stop): When stopped for the completion of an inferior function
>>        call, verify the expected stack frame kind and use dummy_frame_pop.
>>        (inferior_program_state): New struct.
>>        (save_inferior_program_state,restore_inferior_program_state,
>>        do_restore_inferior_program_state_cleanup,
>>        make_cleanup_restore_inferior_program_state,
>>        discard_inferior_program_state,
>>        get_inferior_program_state_regcache): New functions.
>>        (inferior_status): Remove members stop_signal, stop_pc, registers,
>>        restore_stack_info.
>>        (save_inferior_status): Remove arg restore_stack_info.
>>        All callers updated.  Remove saving of state now saved by
>>        save_inferior_program_state.
>>        (restore_inferior_status): Remove restoration of state now done by
>>        restore_inferior_program_state.
>>        (discard_inferior_status): Remove freeing of registers, now done by
>>        discard_inferior_program_state.
>>
>>        * gdb.base/call-signal-resume.exp: New file.
>>        * gdb.base/call-signals.c: New file.
>>        * gdb.base/unwindonsignal.exp: New file.
>>
>
> Ummm, this time with the correct patch attached.  Sorry!
>

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

[Pedro Alves]

Hi Doug,

I'd like to bring a current GDB deficiency to your attention, in
case it affects anything related to this patch.

If GDB stops due to a signal instead of hitting the dummy frame
breakpoint, and you have set GDB to restore the state
automatically with "set unwindonsignal on", and the thread
that reported the signal (say a SIGSEGV) was *not* the same that was
doing the infcall, GDB will currently restore the old context to the
wrong thread (seen by inspection, having really tried it).

Not having studied the patch yet, I'm just wondering if your changes
would make it easier or harder to fix this, or if you could be
extending the problem by possibly restoring things in the wrong
thread as well.

-- 
Pedro Alves

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

[Doug Evans]

On Mon, Dec 1, 2008 at 1:21 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> Hi Doug,
>
> I'd like to bring a current GDB deficiency to your attention, in
> case it affects anything related to this patch.
>
> If GDB stops due to a signal instead of hitting the dummy frame
> breakpoint, and you have set GDB to restore the state
> automatically with "set unwindonsignal on", and the thread
> that reported the signal (say a SIGSEGV) was *not* the same that was
> doing the infcall, GDB will currently restore the old context to the
> wrong thread (seen by inspection, having really tried it).
>
> Not having studied the patch yet, I'm just wondering if your changes
> would make it easier or harder to fix this, or if you could be
> extending the problem by possibly restoring things in the wrong
> thread as well.

Heh.  My patch serendipitously has the following to catch another case
I was seeing where the current thread unexpectedly changed.

  if (! ptid_equal (this_thread->ptid, inferior_thread ()->ptid))
    {
      /* We've switched threads.  Perhaps this shouldn't happen, but we
         protect ourselves anyway.
         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);
      dummy_frame_discard (dummy_frame);
      error (_("\
The current thread has switched 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."));
    }

I need fix my patch to save the calling thread's ptid
(this_thread->ptid) before resuming execution in case the thread dies.
I think I shouldn't call dummy_frame_discard here too.
And I also need to change the comment to document why the current
thread can switch, because it _can_ happen. :-)
I'll prepare a new version of my patch, and include a testcase to
handle the situation you mention.  Thanks!

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

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 5281 bytes --]

On Mon, Dec 1, 2008 at 5:20 PM, Doug Evans <dje@google.com> wrote:
> On Mon, Dec 1, 2008 at 1:21 PM, Pedro Alves <pedro@codesourcery.com> wrote:
>> Hi Doug,
>>
>> I'd like to bring a current GDB deficiency to your attention, in
>> case it affects anything related to this patch.
>>
>> If GDB stops due to a signal instead of hitting the dummy frame
>> breakpoint, and you have set GDB to restore the state
>> automatically with "set unwindonsignal on", and the thread
>> that reported the signal (say a SIGSEGV) was *not* the same that was
>> doing the infcall, GDB will currently restore the old context to the
>> wrong thread (seen by inspection, having really tried it).
>>
>> Not having studied the patch yet, I'm just wondering if your changes
>> would make it easier or harder to fix this, or if you could be
>> extending the problem by possibly restoring things in the wrong
>> thread as well.
>
> Heh.  My patch serendipitously has the following to catch another case
> I was seeing where the current thread unexpectedly changed.
>
>  if (! ptid_equal (this_thread->ptid, inferior_thread ()->ptid))
>    {
>      /* We've switched threads.  Perhaps this shouldn't happen, but we
>         protect ourselves anyway.
>         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);
>      dummy_frame_discard (dummy_frame);
>      error (_("\
> The current thread has switched 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."));
>    }
>
> I need fix my patch to save the calling thread's ptid
> (this_thread->ptid) before resuming execution in case the thread dies.
> I think I shouldn't call dummy_frame_discard here too.
> And I also need to change the comment to document why the current
> thread can switch, because it _can_ happen. :-)
> I'll prepare a new version of my patch, and include a testcase to
> handle the situation you mention.  Thanks!


Here's an updated version of my patch.
It includes a testcase for the current thread changing in the middle
of a hand-called function.

Ok to check in?

2008-12-02  Doug Evans  <dje@google.com>

        * dummy-frame.c (dummy_frame): Replace regcache member with
        caller_state.
        (dummy_frame_push): Replace caller_regcache arg with caller_state.
        Return pointer to created dummy frame.  All callers updated.
        (remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
        lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
        do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
        functions.
        (dummy_frame_pop): Rewrite.  Verify requested frame is in the
        dummy frame stack.  Restore program state.
        (cleanup_dummy_frames): Rewrite.
        (dummy_frame_sniffer): Update.
        * dummy-frame.h (regcache): Delete forward decl.
        (inferior_program_state,dummy_frame,cleanup): Add forward decls.
        (dummy_frame_push): Update prototype.
        (dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
        * frame.c (frame_pop): dummy_frame_pop now does all the work for
        DUMMY_FRAMEs.
        * infcall.c (call_function_by_hand): Replace caller_regcache,
        caller_regcache_cleanup with caller_state, caller_state_cleanup.
        New locals dummy_frame, dummy_frame_cleanup, this_thread_ptid.
        Detect program stopping in a different thread.
        * inferior.h (inferior_program_state): Declare (opaque type).
        (save_inferior_program_state,restore_inferior_program_state,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_state_regcache): Declare.
        (save_inferior_status): Update prototype.
        * infrun.c: #include "dummy-frame.h"
        (normal_stop): When stopped for the completion of an inferior function
        call, verify the expected stack frame kind and use dummy_frame_pop.
        (inferior_program_state): New struct.
        (save_inferior_program_state,restore_inferior_program_state,
        do_restore_inferior_program_state_cleanup,
        make_cleanup_restore_inferior_program_state,
        discard_inferior_program_state,
        get_inferior_program_state_regcache): New functions.
        (inferior_status): Remove members stop_signal, stop_pc, registers,
        restore_stack_info.
        (save_inferior_status): Remove arg restore_stack_info.
        All callers updated.  Remove saving of state now saved by
        save_inferior_program_state.
        (restore_inferior_status): Remove restoration of state now done by
        restore_inferior_program_state.
        (discard_inferior_status): Remove freeing of registers, now done by
        discard_inferior_program_state.

        * gdb.base/call-signal-resume.exp: New file.
        * gdb.base/call-signals.c: New file.
        * gdb.base/unwindonsignal.exp: New file.
        * gdb.base/unwindonsignal.c: New file.
        * gdb.threads/interrupted-hand-call.exp: New file.
        * gdb.threads/interrupted-hand-call.cv: New file.

[-- Attachment #2: gdb-081202-infcall-3.patch.txt --]
[-- Type: text/plain, Size: 51485 bytes --]

2008-11-18  Doug Evans  <dje@google.com>

	* dummy-frame.c (dummy_frame): Replace regcache member with
	caller_state.
	(dummy_frame_push): Replace caller_regcache arg with caller_state.
	Return pointer to created dummy frame.  All callers updated.
	(remove_dummy_frame,do_dummy_frame_cleanup,pop_dummy_frame_from_ptr,
	lookup_dummy,lookup_dummy_id, pop_dummy_frame,dummy_frame_discard,
	do_pop_dummy_frame_cleanup,make_cleanup_pop_dummy_frame): New
	functions.
	(dummy_frame_pop): Rewrite.  Verify requested frame is in the
	dummy frame stack.  Restore program state.
	(cleanup_dummy_frames): Rewrite.
	(dummy_frame_sniffer): Update.
	* dummy-frame.h (regcache): Delete forward decl.
	(inferior_program_state,dummy_frame,cleanup): Add forward decls.
	(dummy_frame_push): Update prototype.
	(dummy_frame_discard,make_cleanup_pop_dummy_frame): Declare.
	* frame.c (frame_pop): dummy_frame_pop now does all the work for
	DUMMY_FRAMEs.
	* infcall.c (call_function_by_hand): Replace caller_regcache,
	caller_regcache_cleanup with caller_state, caller_state_cleanup.
	New locals dummy_frame, dummy_frame_cleanup, this_thread_ptid.
	Detect program stopping in a different thread.
	* inferior.h (inferior_program_state): Declare (opaque type).
	(save_inferior_program_state,restore_inferior_program_state,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): Declare.
	(save_inferior_status): Update prototype.
	* infrun.c: #include "dummy-frame.h"
	(normal_stop): When stopped for the completion of an inferior function
	call, verify the expected stack frame kind and use dummy_frame_pop.
	(inferior_program_state): New struct.
	(save_inferior_program_state,restore_inferior_program_state,
	do_restore_inferior_program_state_cleanup,
	make_cleanup_restore_inferior_program_state,
	discard_inferior_program_state,
	get_inferior_program_state_regcache): New functions.
	(inferior_status): Remove members stop_signal, stop_pc, registers,
	restore_stack_info.
	(save_inferior_status): Remove arg restore_stack_info.
	All callers updated.  Remove saving of state now saved by
	save_inferior_program_state.
	(restore_inferior_status): Remove restoration of state now done by
	restore_inferior_program_state.
	(discard_inferior_status): Remove freeing of registers, now done by
	discard_inferior_program_state.

	* gdb.base/call-signal-resume.exp: New file.
	* gdb.base/call-signals.c: New file.
	* gdb.base/unwindonsignal.exp: New file.
	* gdb.base/unwindonsignal.c: New file.
	* gdb.threads/interrupted-hand-call.exp: New file.
	* gdb.threads/interrupted-hand-call.cv: New file.

Index: dummy-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.c,v
retrieving revision 1.53
diff -u -p -r1.53 dummy-frame.c
--- dummy-frame.c	30 Oct 2008 20:35:30 -0000	1.53
+++ dummy-frame.c	3 Dec 2008 05:00:42 -0000
@@ -42,8 +42,8 @@ struct dummy_frame
   /* This frame's ID.  Must match the value returned by
      gdbarch_dummy_id.  */
   struct frame_id id;
-  /* The caller's regcache.  */
-  struct regcache *regcache;
+  /* The caller's state prior to the call.  */
+  struct inferior_program_state *caller_state;
 };
 
 static struct dummy_frame *dummy_frame_stack = NULL;
@@ -81,61 +81,181 @@ deprecated_pc_in_call_dummy (CORE_ADDR p
   return 0;
 }
 
-/* Push the caller's state, along with the dummy frame info, onto a
+/* Push the caller's state, along with the dummy frame info, onto the
    dummy-frame stack.  */
 
-void
-dummy_frame_push (struct regcache *caller_regcache,
+struct dummy_frame *
+dummy_frame_push (struct inferior_program_state *caller_state,
 		  const struct frame_id *dummy_id)
 {
   struct dummy_frame *dummy_frame;
 
   dummy_frame = XZALLOC (struct dummy_frame);
-  dummy_frame->regcache = caller_regcache;
+  dummy_frame->caller_state = caller_state;
   dummy_frame->id = (*dummy_id);
   dummy_frame->next = dummy_frame_stack;
   dummy_frame_stack = dummy_frame;
+
+  return dummy_frame;
 }
 
-/* Pop the dummy frame with ID dummy_id from the dummy-frame stack.  */
+/* Remove *DUMMY_PTR from the dummy frame stack.  */
 
-void
-dummy_frame_pop (struct frame_id dummy_id)
+static void
+remove_dummy_frame (struct dummy_frame **dummy_ptr)
+{
+  struct dummy_frame *dummy = *dummy_ptr;
+
+  *dummy_ptr = dummy->next;
+  discard_inferior_program_state (dummy->caller_state);
+  xfree (dummy);
+}
+
+/* Cleanup handler for dummy_frame_pop.  */
+
+static void
+do_dummy_frame_cleanup (void *arg)
 {
-  struct dummy_frame **dummy_ptr;
+  struct dummy_frame **dummy_ptr = arg;
+
+  remove_dummy_frame (dummy_ptr);
+}
+
+/* Pop *DUMMY_PTR, restoring program state to that before the
+   frame was created.  */
+
+static void
+pop_dummy_frame_from_ptr (struct dummy_frame **dummy_ptr)
+{
+  struct cleanup *cleanups;
+  struct dummy_frame *dummy;
+
+  cleanups = make_cleanup (do_dummy_frame_cleanup, dummy_ptr);
 
-  for (dummy_ptr = &dummy_frame_stack;
-       (*dummy_ptr) != NULL;
-       dummy_ptr = &(*dummy_ptr)->next)
+  restore_inferior_program_state ((*dummy_ptr)->caller_state);
+
+  /* restore_inferior_status frees inf_status,
+     all that remains is to pop *dummy_ptr */
+  discard_cleanups (cleanups);
+  dummy = *dummy_ptr;
+  *dummy_ptr = dummy->next;
+  xfree (dummy);
+
+  /* We've made right mess of GDB's local state, just discard
+     everything.  */
+  reinit_frame_cache ();
+}
+
+/* Return a pointer to DUMMY's entry in the dummy frame stack.
+   Returns NULL if DUMMY is not present.  */
+
+static struct dummy_frame **
+lookup_dummy (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  for (dp = &dummy_frame_stack; *dp != NULL; dp = &(*dp)->next)
     {
-      struct dummy_frame *dummy = *dummy_ptr;
-      if (frame_id_eq (dummy->id, dummy_id))
-	{
-	  *dummy_ptr = dummy->next;
-	  regcache_xfree (dummy->regcache);
-	  xfree (dummy);
-	  break;
-	}
+      if (*dp == dummy)
+	return dp;
     }
+
+  return NULL;
 }
 
-/* There may be stale dummy frames, perhaps left over from when a longjump took us
-   out of a function that was called by the debugger.  Clean them up at least once
-   whenever we start a new inferior.  */
+/* Look up DUMMY_ID.
+   Return NULL if not found.  */
 
-static void
-cleanup_dummy_frames (struct target_ops *target, int from_tty)
+static struct dummy_frame **
+lookup_dummy_id (struct frame_id dummy_id)
 {
-  struct dummy_frame *dummy, *next;
+  struct dummy_frame **dp;
 
-  for (dummy = dummy_frame_stack; dummy; dummy = next)
+  for (dp = &dummy_frame_stack; *dp != NULL; dp = &(*dp)->next)
     {
-      next = dummy->next;
-      regcache_xfree (dummy->regcache);
-      xfree (dummy);
+      if (frame_id_eq ((*dp)->id, dummy_id))
+	return dp;
     }
 
-  dummy_frame_stack = NULL;
+  return NULL;
+}
+
+/* Pop DUMMY, restore inferior state.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.  */
+
+static void
+pop_dummy_frame (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy (dummy);
+  gdb_assert (dp != NULL);
+
+  pop_dummy_frame_from_ptr (dp);
+}
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.
+
+   NOTE: This can only pop the one frame, even if it is in the middle of the
+   stack, because the other frames may be for different threads, and there's
+   currently no way to tell which stack frame is for which thread.  */
+
+void
+dummy_frame_pop (struct frame_id dummy_id)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy_id (dummy_id);
+  gdb_assert (dp != NULL);
+
+  pop_dummy_frame_from_ptr (dp);
+}
+
+/* Discard DUMMY and remove it from the dummy frame stack.
+   If the frame isn't found, flag an internal error.  */
+
+void
+dummy_frame_discard (struct dummy_frame *dummy)
+{
+  struct dummy_frame **dp;
+
+  dp = lookup_dummy (dummy);
+  gdb_assert (dp != NULL);
+
+  remove_dummy_frame (dp);
+}
+
+/* Utility for make_cleanup_pop_dummy_frame.  */
+
+static void
+do_pop_dummy_frame_cleanup (void *dummy)
+{
+  pop_dummy_frame (dummy);
+}
+
+/* Schedule a cleanup to pop DUMMY_FRAME and restore inferior state.  */
+
+struct cleanup *
+make_cleanup_pop_dummy_frame (struct dummy_frame *dummy)
+{
+  return make_cleanup (do_pop_dummy_frame_cleanup, dummy);
+}
+
+/* There may be stale dummy frames, perhaps left over from when a longjump took
+   us out of a function that was called by the debugger.  Clean them up at
+   least once whenever we start a new inferior.  */
+
+static void
+cleanup_dummy_frames (struct target_ops *target, int from_tty)
+{
+  while (dummy_frame_stack != NULL)
+    {
+      remove_dummy_frame (&dummy_frame_stack);
+    }
 }
 
 /* Return the dummy frame cache, it contains both the ID, and a
@@ -178,7 +298,7 @@ dummy_frame_sniffer (const struct frame_
 	    {
 	      struct dummy_frame_cache *cache;
 	      cache = FRAME_OBSTACK_ZALLOC (struct dummy_frame_cache);
-	      cache->prev_regcache = dummyframe->regcache;
+	      cache->prev_regcache = get_inferior_program_state_regcache (dummyframe->caller_state);
 	      cache->this_id = this_id;
 	      (*this_prologue_cache) = cache;
 	      return 1;
@@ -215,7 +335,7 @@ dummy_frame_prev_register (struct frame_
   return reg_val;
 }
 
-/* Assuming that THIS frame is a dummy, return the ID of THIS frame.  That ID is
+/* Assuming that THIS_FRAME is a dummy, return its ID.  That ID is
    determined by examining the NEXT frame's unwound registers using
    the method dummy_id().  As a side effect, THIS dummy frame's
    dummy cache is located and and saved in THIS_PROLOGUE_CACHE.  */
Index: dummy-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/dummy-frame.h,v
retrieving revision 1.23
diff -u -p -r1.23 dummy-frame.h
--- dummy-frame.h	8 Sep 2008 15:23:12 -0000	1.23
+++ dummy-frame.h	3 Dec 2008 05:00:42 -0000
@@ -23,8 +23,10 @@
 #include "frame.h"
 
 struct frame_info;
-struct regcache;
+struct inferior_program_state;
 struct frame_unwind;
+struct dummy_frame;
+struct cleanup;
 
 /* Push the information needed to identify, and unwind from, a dummy
    frame onto the dummy frame stack.  */
@@ -39,11 +41,29 @@ struct frame_unwind;
    be expanded so that it knowns the lower/upper extent of the dummy
    frame's code.  */
 
-extern void dummy_frame_push (struct regcache *regcache,
-			      const struct frame_id *dummy_id);
+extern struct dummy_frame *dummy_frame_push (struct inferior_program_state *caller_state,
+					     const struct frame_id *dummy_id);
+
+/* Pop the dummy frame DUMMY_ID, restoring program state to that before the
+   frame was created.
+   On return reinit_frame_cache has been called.
+   If the frame isn't found, flag an internal error.
+
+   NOTE: This can only pop the one frame, even if it is in the middle of the
+   stack, because the other frames may be for different threads, and there's
+   currently no way to tell which stack frame is for which thread.  */
 
 extern void dummy_frame_pop (struct frame_id dummy_id);
 
+/* Discard DUMMY and remove it from the dummy frame stack.
+   If the frame isn't found, flag an internal error.  */
+
+extern void dummy_frame_discard (struct dummy_frame *dummy);
+
+/* Schedule a cleanup to pop DUMMY_FRAME.  */
+
+extern struct cleanup *make_cleanup_pop_dummy_frame (struct dummy_frame *);
+
 /* If the PC falls in a dummy frame, return a dummy frame
    unwinder.  */
 
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.256
diff -u -p -r1.256 frame.c
--- frame.c	20 Nov 2008 22:16:16 -0000	1.256
+++ frame.c	3 Dec 2008 05:00:43 -0000
@@ -536,6 +536,14 @@ frame_pop (struct frame_info *this_frame
   struct regcache *scratch;
   struct cleanup *cleanups;
 
+  if (get_frame_type (this_frame) == DUMMY_FRAME)
+    {
+      /* Popping a dummy frame involves restoring more than just registers.
+	 dummy_frame_pop does all the work.  */
+      dummy_frame_pop (get_frame_id (this_frame));
+      return;
+    }
+
   /* Ensure that we have a frame to pop to.  */
   prev_frame = get_prev_frame_1 (this_frame);
 
@@ -549,11 +557,6 @@ frame_pop (struct frame_info *this_frame
   scratch = frame_save_as_regcache (prev_frame);
   cleanups = make_cleanup_regcache_xfree (scratch);
 
-  /* If we are popping a dummy frame, clean up the associated
-     data as well.  */
-  if (get_frame_type (this_frame) == DUMMY_FRAME)
-    dummy_frame_pop (get_frame_id (this_frame));
-
   /* FIXME: cagney/2003-03-16: It should be possible to tell the
      target's register cache that it is about to be hit with a burst
      register transfer and that the sequence of register writes should
Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.106
diff -u -p -r1.106 infcall.c
--- infcall.c	18 Nov 2008 00:13:02 -0000	1.106
+++ infcall.c	3 Dec 2008 05:00:43 -0000
@@ -318,16 +318,19 @@ call_function_by_hand (struct value *fun
   struct cleanup *retbuf_cleanup;
   struct inferior_status *inf_status;
   struct cleanup *inf_status_cleanup;
+  struct inferior_program_state *caller_state;
+  struct cleanup *caller_state_cleanup;
+  struct dummy_frame *dummy_frame;
+  struct cleanup *dummy_frame_cleanup;
   CORE_ADDR funaddr;
   CORE_ADDR real_pc;
   struct type *ftype = check_typedef (value_type (function));
   CORE_ADDR bp_addr;
-  struct regcache *caller_regcache;
-  struct cleanup *caller_regcache_cleanup;
   struct frame_id dummy_id;
   struct cleanup *args_cleanup;
   struct frame_info *frame;
   struct gdbarch *gdbarch;
+  ptid_t this_thread_ptid;
 
   if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -351,15 +354,16 @@ call_function_by_hand (struct value *fun
   /* A cleanup for the inferior status.  Create this AFTER the retbuf
      so that this can be discarded or applied without interfering with
      the regbuf.  */
-  inf_status = save_inferior_status (1);
+  inf_status = save_inferior_status ();
   inf_status_cleanup = make_cleanup_restore_inferior_status (inf_status);
 
-  /* Save the caller's registers so that they can be restored once the
+  /* Save the caller's registers and other state associated with the
+     inferior itself so that they can be restored once the
      callee returns.  To allow nested calls the registers are (further
      down) pushed onto a dummy frame stack.  Include a cleanup (which
      is tossed once the regcache has been pushed).  */
-  caller_regcache = frame_save_as_regcache (frame);
-  caller_regcache_cleanup = make_cleanup_regcache_xfree (caller_regcache);
+  caller_state = save_inferior_program_state ();
+  caller_state_cleanup = make_cleanup_restore_inferior_program_state (caller_state);
 
   /* Ensure that the initial SP is correctly aligned.  */
   {
@@ -642,8 +646,10 @@ call_function_by_hand (struct value *fun
   /* 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);
 
   /* - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP - SNIP -
      If you're looking to implement asynchronous dummy-frames, then
@@ -671,7 +677,12 @@ call_function_by_hand (struct value *fun
     struct cleanup *old_cleanups = make_cleanup (null_cleanup, 0);
     struct cleanup *old_cleanups2;
     int saved_async = 0;
-    struct thread_info *tp = inferior_thread ();
+    struct thread_info *this_thread;
+
+    this_thread = inferior_thread ();
+    /* Save this thread's ptid, we need it later but the thread
+       may have exited.  */
+    this_thread_ptid = this_thread->ptid;
 
     /* If all error()s out of proceed ended up calling normal_stop
        (and perhaps they should; it already does in the special case
@@ -679,7 +690,7 @@ call_function_by_hand (struct value *fun
     make_cleanup (breakpoint_auto_delete_contents, NULL);
 
     disable_watchpoints_before_interactive_call_start ();
-    tp->proceed_to_finish = 1;	/* We want stop_registers, please... */
+    this_thread->proceed_to_finish = 1; /* We want stop_registers, please... */
 
     if (target_can_async_p ())
       saved_async = target_async_mask (0);
@@ -690,12 +701,12 @@ call_function_by_hand (struct value *fun
     suppress_stop_observer = 1;
     proceed (real_pc, TARGET_SIGNAL_0, 0);
     do_cleanups (old_cleanups2);
-    
+
     if (saved_async)
       target_async_mask (saved_async);
-    
+
     enable_watchpoints_after_interactive_call_stop ();
-      
+
     discard_cleanups (old_cleanups);
   }
 
@@ -705,10 +716,28 @@ call_function_by_hand (struct value *fun
 	 we'll crash as the inferior is no longer running.  */
       discard_cleanups (inf_status_cleanup);
       discard_inferior_status (inf_status);
+      dummy_frame_discard (dummy_frame);
       error (_("\
 The program being debugged exited while in a function called from GDB."));
     }
 
+  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."));
+    }
+
   if (stopped_by_random_signal || !stop_stack_dummy)
     {
       /* Find the name of the function we're about to complain about.  */
@@ -745,9 +774,10 @@ The program being debugged exited while 
 	    {
 	      /* The user wants the context restored. */
 
-	      /* We must get back to the frame we were before the
-		 dummy call. */
-	      frame_pop (get_current_frame ());
+	      /* We must get back to the frame we were before the dummy call.
+		 Plus we need to restore inferior status to that before the
+		 dummy call.  This is all handled by cleanups
+		 dummy_frame_cleanup and inf_status_cleanup.  */
 
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
@@ -761,14 +791,16 @@ Evaluation of the expression containing 
 	  else
 	    {
 	      /* The user wants to stay in the frame where we stopped
-                 (default).*/
-	      /* If we restored the inferior status (via the cleanup),
+		 (default).
+		 If we restored the inferior status (via the cleanup),
 		 we would print a spurious error message (Unable to
-		 restore previously selected frame), would write the
-		 registers from the inf_status (which is wrong), and
-		 would do other wrong things.  */
+		 restore previously selected frame), and
+		 would do other wrong things.
+		 Discarding inf_status_cleanup also discards
+		 dummy_frame_cleanup.  */
 	      discard_cleanups (inf_status_cleanup);
 	      discard_inferior_status (inf_status);
+
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
 	      error (_("\
@@ -782,14 +814,17 @@ Evaluation of the expression containing 
 
       if (!stop_stack_dummy)
 	{
-	  /* We hit a breakpoint inside the FUNCTION. */
-	  /* If we restored the inferior status (via the cleanup), we
+	  /* We hit a breakpoint inside the FUNCTION.
+	     If we restored the inferior status (via the cleanup), we
 	     would print a spurious error message (Unable to restore
 	     previously selected frame), would write the registers
 	     from the inf_status (which is wrong), and would do other
-	     wrong things.  */
+	     wrong things.
+	     Discarding inf_status_cleanup also discards
+	     dummy_frame_cleanup.  */
 	  discard_cleanups (inf_status_cleanup);
 	  discard_inferior_status (inf_status);
+
 	  /* The following error message used to say "The expression
 	     which contained the function call has been discarded."
 	     It is a hard concept to explain in a few words.  Ideally,
@@ -811,6 +846,9 @@ the function call)."), name);
 
   /* If we get here the called FUNCTION run to completion. */
 
+  /* The dummy frame has been popped so discard its cleanup.  */
+  discard_cleanups (dummy_frame_cleanup);
+
   /* On normal return, the stack dummy has been popped already.  */
   regcache_cpy_no_passthrough (retbuf, stop_registers);
 
Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.116
diff -u -p -r1.116 inferior.h
--- inferior.h	20 Nov 2008 00:35:23 -0000	1.116
+++ inferior.h	3 Dec 2008 05:00:43 -0000
@@ -40,24 +40,40 @@ struct ui_out;
 /* For struct frame_id.  */
 #include "frame.h"
 
-/* Structure in which to save the status of the inferior.  Create/Save
-   through "save_inferior_status", restore through
-   "restore_inferior_status".
-
-   This pair of routines should be called around any transfer of
-   control to the inferior which you don't want showing up in your
-   control variables.  */
+/* 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.
+
+   Call these routines around hand called functions, including function calls
+   in conditional breakpoints for example.  */
+
+struct inferior_program_state;
 struct inferior_status;
 
-extern struct inferior_status *save_inferior_status (int);
+extern struct inferior_program_state *save_inferior_program_state (void);
+extern struct inferior_status *save_inferior_status (void);
 
+extern void restore_inferior_program_state (struct inferior_program_state *);
 extern void restore_inferior_status (struct inferior_status *);
 
+extern struct cleanup *make_cleanup_restore_inferior_program_state (struct inferior_program_state *);
 extern struct cleanup *make_cleanup_restore_inferior_status (struct inferior_status *);
 
+extern void discard_inferior_program_state (struct inferior_program_state *);
 extern void discard_inferior_status (struct inferior_status *);
 
+extern struct regcache *get_inferior_program_state_regcache (struct inferior_program_state *);
+
 /* The -1 ptid, often used to indicate either an error condition
    or a "don't care" condition, i.e, "run all threads."  */
 extern ptid_t minus_one_ptid;
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.345
diff -u -p -r1.345 infrun.c
--- infrun.c	2 Dec 2008 19:20:23 -0000	1.345
+++ infrun.c	3 Dec 2008 05:00:43 -0000
@@ -45,7 +45,7 @@
 #include "language.h"
 #include "solib.h"
 #include "main.h"
-
+#include "dummy-frame.h"
 #include "gdb_assert.h"
 #include "mi/mi-common.h"
 #include "event-top.h"
@@ -4346,15 +4346,23 @@ Further execution is probably impossible
 
   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"));
+	}
     }
 
 done:
@@ -4750,10 +4758,86 @@ signals_info (char *signum_exp, int from
   printf_filtered (_("\nUse the \"handle\" command to change these tables.\n"));
 }
 \f
-struct inferior_status
+/* Inferior program state.
+   These are details related to the inferior itself, and don't include
+   things like what frame the user had selected or what gdb was doing
+   with the target at the time.
+   For inferior function calls these are things we want to restore
+   regardless of whether the function call successfully completes
+   or the dummy frame has to be manually popped.  */
+
+struct inferior_program_state
 {
   enum target_signal stop_signal;
   CORE_ADDR stop_pc;
+  struct regcache *registers;
+};
+
+struct inferior_program_state *
+save_inferior_program_state ()
+{
+  struct inferior_program_state *inf_state = XMALLOC (struct inferior_program_state);
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  inf_state->stop_signal = tp->stop_signal;
+  inf_state->stop_pc = stop_pc;
+
+  inf_state->registers = regcache_dup (get_current_regcache ());
+
+  return inf_state;
+}
+
+/* Restore inferior session state to INF_STATE.  */
+
+void
+restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  struct thread_info *tp = inferior_thread ();
+  struct inferior *inf = current_inferior ();
+
+  tp->stop_signal = inf_state->stop_signal;
+  stop_pc = inf_state->stop_pc;
+
+  /* The inferior can be gone if the user types "print exit(0)"
+     (and perhaps other times).  */
+  if (target_has_execution)
+    /* NB: The register write goes through to the target.  */
+    regcache_cpy (get_current_regcache (), inf_state->registers);
+  regcache_xfree (inf_state->registers);
+}
+
+static void
+do_restore_inferior_program_state_cleanup (void *state)
+{
+  restore_inferior_program_state (state);
+}
+
+struct cleanup *
+make_cleanup_restore_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  return make_cleanup (do_restore_inferior_program_state_cleanup, inf_state);
+}
+
+void
+discard_inferior_program_state (struct inferior_program_state *inf_state)
+{
+  regcache_xfree (inf_state->registers);
+  xfree (inf_state);
+}
+
+struct regcache *
+get_inferior_program_state_regcache (struct inferior_program_state *inf_state)
+{
+  return inf_state->registers;
+}
+
+/* Session related state for inferior function calls.
+   These are the additional bits of state that need to be restored
+   when an inferior function call successfully completes.  */
+
+struct inferior_status
+{
   bpstat stop_bpstat;
   int stop_step;
   int stop_stack_dummy;
@@ -4767,32 +4851,23 @@ struct inferior_status
   int stop_after_trap;
   int stop_soon;
 
-  /* These are here because if call_function_by_hand has written some
-     registers and then decides to call error(), we better not have changed
-     any registers.  */
-  struct regcache *registers;
-
-  /* A frame unique identifier.  */
+  /* ID if the selected frame when the inferior function call was made.  */
   struct frame_id selected_frame_id;
 
   int breakpoint_proceeded;
-  int restore_stack_info;
   int proceed_to_finish;
 };
 
 /* Save all of the information associated with the inferior<==>gdb
-   connection.  INF_STATUS is a pointer to a "struct inferior_status"
-   (defined in inferior.h).  */
+   connection.  */
 
 struct inferior_status *
-save_inferior_status (int restore_stack_info)
+save_inferior_status ()
 {
   struct inferior_status *inf_status = XMALLOC (struct inferior_status);
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  inf_status->stop_signal = tp->stop_signal;
-  inf_status->stop_pc = stop_pc;
   inf_status->stop_step = tp->stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
@@ -4810,12 +4885,10 @@ save_inferior_status (int restore_stack_
   inf_status->stop_bpstat = tp->stop_bpstat;
   tp->stop_bpstat = bpstat_copy (tp->stop_bpstat);
   inf_status->breakpoint_proceeded = breakpoint_proceeded;
-  inf_status->restore_stack_info = restore_stack_info;
   inf_status->proceed_to_finish = tp->proceed_to_finish;
 
-  inf_status->registers = regcache_dup (get_current_regcache ());
-
   inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL));
+
   return inf_status;
 }
 
@@ -4840,14 +4913,14 @@ restore_selected_frame (void *args)
   return (1);
 }
 
+/* Restore inferior session state to INF_STATUS.  */
+
 void
 restore_inferior_status (struct inferior_status *inf_status)
 {
   struct thread_info *tp = inferior_thread ();
   struct inferior *inf = current_inferior ();
 
-  tp->stop_signal = inf_status->stop_signal;
-  stop_pc = inf_status->stop_pc;
   tp->stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
@@ -4860,24 +4933,11 @@ restore_inferior_status (struct inferior
   inf->stop_soon = inf_status->stop_soon;
   bpstat_clear (&tp->stop_bpstat);
   tp->stop_bpstat = inf_status->stop_bpstat;
+  inf_status->stop_bpstat = NULL;
   breakpoint_proceeded = inf_status->breakpoint_proceeded;
   tp->proceed_to_finish = inf_status->proceed_to_finish;
 
-  /* The inferior can be gone if the user types "print exit(0)"
-     (and perhaps other times).  */
-  if (target_has_execution)
-    /* NB: The register write goes through to the target.  */
-    regcache_cpy (get_current_regcache (), inf_status->registers);
-  regcache_xfree (inf_status->registers);
-
-  /* FIXME: If we are being called after stopping in a function which
-     is called from gdb, we should not be trying to restore the
-     selected frame; it just prints a spurious error message (The
-     message is useful, however, in detecting bugs in gdb (like if gdb
-     clobbers the stack)).  In fact, should we be restoring the
-     inferior status at all in that case?  .  */
-
-  if (target_has_stack && inf_status->restore_stack_info)
+  if (target_has_stack)
     {
       /* The point of catch_errors is that if the stack is clobbered,
          walking the stack might encounter a garbage pointer and
@@ -4889,7 +4949,6 @@ restore_inferior_status (struct inferior
 	/* Error in restoring the selected frame.  Select the innermost
 	   frame.  */
 	select_frame (get_current_frame ());
-
     }
 
   xfree (inf_status);
@@ -4912,10 +4971,9 @@ discard_inferior_status (struct inferior
 {
   /* See save_inferior_status for info on stop_bpstat. */
   bpstat_clear (&inf_status->stop_bpstat);
-  regcache_xfree (inf_status->registers);
   xfree (inf_status);
 }
-
+\f
 int
 inferior_has_forked (ptid_t pid, ptid_t *child_pid)
 {
Index: testsuite/gdb.base/call-signal-resume.exp
===================================================================
RCS file: testsuite/gdb.base/call-signal-resume.exp
diff -N testsuite/gdb.base/call-signal-resume.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signal-resume.exp	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,159 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test inferior resumption after discarding a hand-called function.
+# There are two things to test.
+# 1) Inferior stops normally.  Upon resumption it should continue normally,
+#    regardless of whatever signal the hand-called function got.
+# 2) Inferior is stopped at a signal.  Upon resumption it should continue
+#    with that signal, regardless of whatever the hand-called function did.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping call-signal-resume.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested call-signal-resume.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+proc get_dummy_frame_number { } {
+  global gdb_prompt
+
+  send_gdb "bt\n"
+  gdb_expect {
+    -re "#(\[0-9\]*) *<function called from gdb>.*$gdb_prompt $"
+      {
+	return $expect_out(1,string)
+      }
+    -re "$gdb_prompt $"
+      {
+	return ""
+      }
+    timeout
+      {
+	return ""
+      }
+  }
+  return ""
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break stop_one" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Continuing.*Breakpoint \[0-9\]*, stop_one.*" \
+    "continue to breakpoint at stop_one"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "inferior function call signaled"
+    }
+}
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "dummy stack frame number"
+    setup_xfail "*-*-*"
+} else {
+    pass "dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Resume execution, the program should continue without any signal.
+
+gdb_test "break stop_two" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Breakpoint \[0-9\]*, stop_two.*" \
+    "continue to breakpoint at stop_two"
+
+# Continue again, we should get a signal.
+
+gdb_test "continue" "Program received signal .*" \
+    "continue to receipt of signal"
+
+# Hand call another function that prematurely stops,
+# then manually pop the dummy stack frame.
+
+gdb_test "break null_hand_call" "Breakpoint \[0-9\]* at .*"
+gdb_test "call null_hand_call ()" "Breakpoint \[0-9\]*, null_hand_call.*" \
+    "null_hand_call"
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "dummy stack frame number"
+    setup_xfail "*-*-*"
+    # Need something.
+    set frame_number 0
+} else {
+    pass "dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Continue again, this time we should get to the signal handler.
+
+gdb_test "break handle_signal" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Breakpoint \[0-9\]*, handle_signal.*" \
+    "continue to breakpoint at handle_signal"
+
+# Continue one last time, the program should exit normally.
+
+gdb_test "continue" "Program exited normally." \
+    "continue to program exit"
+
+return 0
Index: testsuite/gdb.base/call-signals.c
===================================================================
RCS file: testsuite/gdb.base/call-signals.c
diff -N testsuite/gdb.base/call-signals.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/call-signals.c	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,89 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing handling of inferior function calls
+   in the presence of signals.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+handle_signal (int sig)
+{
+}
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+/* Easy place to set a breakpoint.  */
+
+void
+stop_one ()
+{
+}
+
+void
+stop_two ()
+{
+}
+
+void
+null_hand_call ()
+{
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+
+#ifdef SIG_SETMASK
+  /* Ensure all the signals aren't blocked.
+     The environment in which the testsuite is run may have blocked some
+     for whatever reason.  */
+  {
+    sigset_t newset;
+    sigemptyset (&newset);
+    sigprocmask (SIG_SETMASK, &newset, NULL);
+  }
+#endif
+
+  signal (SIGABRT, handle_signal);
+
+  /* Stop here so we can hand-call gen_signal.  */
+  stop_one ();
+
+  /* When we're resumed stop here.  */
+  stop_two ();
+
+  /* When we're resumed we generate a signal ourselves.  */
+  gen_signal ();
+
+  return 0;
+}
Index: testsuite/gdb.base/unwindonsignal.c
===================================================================
RCS file: testsuite/gdb.base/unwindonsignal.c
diff -N testsuite/gdb.base/unwindonsignal.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/unwindonsignal.c	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing unwindonsignal.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+/* Easy place to set a breakpoint.  */
+
+void
+stop_here ()
+{
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+
+#ifdef SIG_SETMASK
+  /* Ensure all the signals aren't blocked.
+     The environment in which the testsuite is run may have blocked some
+     for whatever reason.  */
+  {
+    sigset_t newset;
+    sigemptyset (&newset);
+    sigprocmask (SIG_SETMASK, &newset, NULL);
+  }
+#endif
+
+  /* Stop here so we can hand-call gen_signal.  */
+  stop_here ();
+
+  return 0;
+}
Index: testsuite/gdb.base/unwindonsignal.exp
===================================================================
RCS file: testsuite/gdb.base/unwindonsignal.exp
diff -N testsuite/gdb.base/unwindonsignal.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/unwindonsignal.exp	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,98 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping unwindonsignal.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "unwindonsignal"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested unwindonsignal.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break stop_here" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Continuing.*Breakpoint \[0-9\]*, stop_here.*" \
+    "continue to breakpoint at stop_here"
+
+# Turn on unwindonsignal.
+gdb_test "set unwindonsignal on" \
+	"" \
+	"setting unwindonsignal"
+gdb_test "show unwindonsignal" \
+	"Unwinding of stack .* is on." \
+	"showing unwindonsignal"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"unwindonsignal, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "unwindonsignal, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, inferior function call signaled"
+    }
+}
+
+# Verify the stack got unwound.
+gdb_test "bt" \
+    "#0 *\[x0-9a-f in\]*stop_here \\(.*\\) at .*#1 *\[x0-9a-f in\]*main \\(.*\\) at .*" \
+	"unwindonsignal, stack unwound"
+
+# Verify the dummy frame got removed from dummy_frame_stack.
+gdb_test_multiple "maint print dummy-frames" \
+	"unwindonsignal, dummy frame removed" {
+    -re "\[\r\n\]*.*stack=.*code=.*\[\r\n\]+$gdb_prompt $" {
+	fail "unwindonsignal, dummy frame removed"
+    }
+    -re "\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, dummy frame removed"
+    }
+}
+
+return 0
Index: testsuite/gdb.threads/interrupted-hand-call.c
===================================================================
RCS file: testsuite/gdb.threads/interrupted-hand-call.c
diff -N testsuite/gdb.threads/interrupted-hand-call.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.threads/interrupted-hand-call.c	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,144 @@
+/* Test case for hand function calls interrupted by a signal in another thread.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+#include <unistd.h>
+
+#ifndef NR_THREADS
+#define NR_THREADS 4
+#endif
+
+pthread_t threads[NR_THREADS];
+
+/* Number of threads currently running.  */
+int thread_count;
+
+pthread_mutex_t thread_count_mutex;
+
+pthread_cond_t thread_count_condvar;
+
+sig_atomic_t sigabrt_received;
+
+void
+incr_thread_count (void)
+{
+  pthread_mutex_lock (&thread_count_mutex);
+  ++thread_count;
+  if (thread_count == NR_THREADS)
+    pthread_cond_signal (&thread_count_condvar);
+  pthread_mutex_unlock (&thread_count_mutex);
+}
+
+void
+cond_wait (pthread_cond_t *cond, pthread_mutex_t *mut)
+{
+  pthread_mutex_lock (mut);
+  pthread_cond_wait (cond, mut);
+  pthread_mutex_unlock (mut);
+}
+
+void
+noreturn (void)
+{
+  pthread_mutex_t mut;
+  pthread_cond_t cond;
+
+  pthread_mutex_init (&mut, NULL);
+  pthread_cond_init (&cond, NULL);
+
+  /* Wait for a condition that will never be signaled, so we effectively
+     block the thread here.  */
+  cond_wait (&cond, &mut);
+}
+
+void *
+thread_entry (void *unused)
+{
+  incr_thread_count ();
+  noreturn ();
+}
+
+void
+sigabrt_handler (int signo)
+{
+  sigabrt_received = 1;
+}
+
+/* Helper to test a hand-call being "interrupted" by a signal on another
+   thread.  */
+
+void
+hand_call_with_signal (void)
+{
+  const struct timespec ts = { 0, 10000000 }; /* 0.01 sec */
+
+  sigabrt_received = 0;
+  pthread_kill (threads[0], SIGABRT);
+  while (! sigabrt_received)
+    nanosleep (&ts, NULL);
+}
+
+/* Wait until all threads are running.  */
+
+void
+wait_all_threads_running (void)
+{
+  pthread_mutex_lock (&thread_count_mutex);
+  pthread_cond_wait (&thread_count_condvar, &thread_count_mutex);
+  if (thread_count == NR_THREADS)
+    {
+      pthread_mutex_unlock (&thread_count_mutex);
+      return;
+    }
+  pthread_mutex_unlock (&thread_count_mutex);
+  printf ("failed waiting for all threads to start\n");
+  abort ();
+}
+
+/* Called when all threads are running.
+   Easy place for a breakpoint.  */
+
+void
+all_threads_running (void)
+{
+}
+
+int
+main (void)
+{
+  int i;
+
+  signal (SIGABRT, sigabrt_handler);
+
+  pthread_mutex_init (&thread_count_mutex, NULL);
+  pthread_cond_init (&thread_count_condvar, NULL);
+
+  for (i = 0; i < NR_THREADS; ++i)
+    pthread_create (&threads[i], NULL, thread_entry, NULL);
+
+  wait_all_threads_running ();
+  all_threads_running ();
+
+  return 0;
+}
+
Index: testsuite/gdb.threads/interrupted-hand-call.exp
===================================================================
RCS file: testsuite/gdb.threads/interrupted-hand-call.exp
diff -N testsuite/gdb.threads/interrupted-hand-call.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.threads/interrupted-hand-call.exp	3 Dec 2008 05:00:43 -0000
@@ -0,0 +1,92 @@
+# Copyright (C) 2004, 2007, 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test recovering from a hand function call that gets interrupted
+# by a signal in another thread.
+
+set NR_THREADS 4
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set testfile "interrupted-hand-call"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}" "additional_flags=-DNR_THREADS=$NR_THREADS"]] != "" } {
+    return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break all_threads_running" \
+         "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+         "breakpoint on all_threads_running"
+
+# Run the program and make sure GDB reports that we stopped after
+# hitting breakpoint 2 in all_threads_running().
+
+gdb_test "continue" \
+         ".*Breakpoint 2, all_threads_running ().*" \
+         "run to all_threads_running"
+
+# NOTE: Don't turn on scheduler-locking here.
+# We want the main thread (hand_call_with_signal) and
+# thread 1 (sigabrt_handler) to both run.
+
+gdb_test "call hand_call_with_signal()" \
+    ".*current thread has changed.*" \
+    "hand-call interrupted by signal in another thread"
+
+# Verify dummy stack frame is still present.
+
+gdb_test "maint print dummy-frames" ".*stack=.*" "dummy stack frame present"
+
+# Continuing now should exit the hand-call and pop the dummy frame.
+
+gdb_test "continue" "" "finish hand-call"
+
+gdb_test_multiple "maint print dummy-frames" "dummy frame popped" {
+    -re ".*stack=.*$gdb_prompt $" {
+	fail "all dummies popped"
+    }
+    -re ".*$gdb_prompt $" {
+	pass "all dummies popped"
+    }
+}
+
+# Continue one last time, the program should exit normally.
+
+gdb_test "continue" "Program exited normally." \
+    "continue to program exit"
+
+return 0

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

[Ulrich Weigand]

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

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

[Pedro Alves]

On Thursday 04 December 2008 15:31:14, Ulrich Weigand wrote:
> 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 ...

And it *can happen* (although it should be rare) that when the exception is
thrown, inferior_ptid is set to a different ptid the infcall started
with (say, while you're handling a thread specific breakpoint hitting the wrong thread),
so, the cleanup can still write the context back, the wrong thread.  Threading
and infcalls failing is a clear mess.

Thinking out loud, I wonder if we shouldn't be storing the
thread state prior to the infcall in struct thread_info itself, and
leave out in a separate object only the session state (what doesn't make
sense to have in thread_info).  So, a thread_info would hold two objects
of the same type, something like, thread_info->infrun_state,
thread_info->stored_infrun_state.

If a thread exits, (or the whole inferior) while doing the infcall, the
state is automatically deleted/discarded when you delete the thread.

If an exception is thrown, you'd go through all threads and set the
infrun_state from the stored_infrun_state, if there's any.

-- 
Pedro Alves

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

[Doug Evans]

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.

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

[Pedro Alves]

On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> In the original code, is there a case when stop_pc != registers.pc?

Here,

<stopped at 0x1234, thread 1>
 (gdb) set $pc = 0xf00
 (gdb) call func()

But here it isn't (!=):

<stopped at 0x1234, thread 1>
 (gdb) set $pc = 0xf00
 (gdb) thread 2
 (gdb) thread 1
<stop_pc is set to 0x1234>
 (gdb) call func()

Don't you love globals?  :-)  I bet that if we got rid of
stop_pc most people wouldn't notice a difference.

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > In the original code, is there a case when stop_pc != registers.pc?
> 
> Here,
> 
> <stopped at 0x1234, thread 1>
>  (gdb) set $pc = 0xf00
>  (gdb) call func()

Huh.  But that case is in fact *broken*, because GDB will use stop_pc
incorrectly: for example, the check whether we are about to continue
at a breakpoint will look at stop_pc, but then continue at $pc.  Also,
if we issue a "step", the step range will be set according to the line
at stop_pc, but we will actually continue at $pc ...

It seems to me just about every current user of stop_pc *really* wants
to look at regcache_read_pc (get_current_regcache ()) ...

Bye,
Ulrich

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

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

[Pedro Alves]

On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > > In the original code, is there a case when stop_pc != registers.pc?
> > 
> > Here,
> > 
> > <stopped at 0x1234, thread 1>
> >  (gdb) set $pc = 0xf00
> >  (gdb) call func()
> 
> Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> incorrectly: for example, the check whether we are about to continue
> at a breakpoint will look at stop_pc, but then continue at $pc.  

This one I believe was the original intention.  The rationale being
that you'd not want to hit a breakpoint again at stop_pc (0x1234),
because there's where you stopped; but, you'd want to hit a a breakpoint
at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.

Note, I'm not saying I agree with this.  I did say that probably nobody
would notice if we got rid of stop_pc.

> It seems to me just about every current user of stop_pc *really* wants
> to look at regcache_read_pc (get_current_regcache ()) ...

I've been sneaking the idea of getting rid of stop_pc for a while now:
 http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html

In fact, I have a months old patch here that completelly removes stop_pc.
IIRC, there were no visible changes in the testsuite.  Say the word,
and I'll brush it up, regtest, submit it.

-- 
Pedro Alves

Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Friday 05 December 2008 00:36:56, Pedro Alves wrote:
> On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > > > In the original code, is there a case when stop_pc != registers.pc?
> > > 
> > > Here,
> > > 
> > > <stopped at 0x1234, thread 1>
> > >  (gdb) set $pc = 0xf00
> > >  (gdb) call func()
> > 
> > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > incorrectly: for example, the check whether we are about to continue
> > at a breakpoint will look at stop_pc, but then continue at $pc.  
> 
> This one I believe was the original intention.  The rationale being
> that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> because there's where you stopped; but, you'd want to hit a a breakpoint
> at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> 
> Note, I'm not saying I agree with this.  I did say that probably nobody
> would notice if we got rid of stop_pc.
> 
> > It seems to me just about every current user of stop_pc *really* wants
> > to look at regcache_read_pc (get_current_regcache ()) ...

Is using read_pc instead OK with you?  It's what I had written already.

> I've been sneaking the idea of getting rid of stop_pc for a while now:
>  http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html
> 
> In fact, I have a months old patch here that completelly removes stop_pc.
> IIRC, there were no visible changes in the testsuite.  Say the word,
> and I'll brush it up, regtest, submit it.

Here it is, it still applied cleanly.  It's smallish.  Regtested on
x86-64-unknown-linux-gnu.

My original motivation was to get rid of the ugly checks
in switch_to_thread, and to try to minimize the extra thread
switching and register reads in non-stop mode.

I had held posting this when I wrote it, since I was not sure we'd not
miss stop_pc in some case.

-- 
Pedro Alves

[-- Attachment #2: stop_pc.diff --]
[-- Type: text/x-diff, Size: 11578 bytes --]

2008-12-05  Pedro Alves  <pedro@codesourcery.com>

	Remove stop_pc.

	* infrun.c (proceed): Don't check stop_pc against the current pc.
	(handle_inferior_event): Add a stop_pc local.
	(handle_step_into_function): Likewise.
	(handle_step_into_function_backward): Likewise.
	(normal_stop): Likewise.
	(struct inferior_status) <stop_pc>: Remove it.
	(save_inferior_status): Don't store the stop_pc.
	(restore_inferior_status): Don't restore it.
	* linux-fork.c (fork_load_infrun_state): Don't set it.
	* m32c-tdep.c (m32c_skip_trampoline_code): Adjust comment.
	* remote-mips.c (common_open): Remove reading the stop_pc.
	* solib-sunos.c (disable_break): Replace stop_pc reference by a
	read_pc call.
	(sunos_solib_create_inferior_hook): Add a stop_pc local.
	* target.h (target_wait): Adjust comment.
	* thread.c (switch_to_thread): Don't read the stop_pc.
	* infcmd.c (stop_pc): Delete.
	(step_1, step_once, signal_command, program_info): Adjust.
	* inferior.h (stop_pc): Delete declaration.

---
 gdb/fork-child.c  |    2 --
 gdb/infcmd.c      |   19 +++++++++----------
 gdb/inferior.h    |    4 ----
 gdb/infrun.c      |   14 +++++---------
 gdb/linux-fork.c  |    1 -
 gdb/m32c-tdep.c   |    2 +-
 gdb/remote-mips.c |    1 -
 gdb/solib-sunos.c |    3 ++-
 gdb/target.h      |    2 +-
 gdb/thread.c      |    8 --------
 10 files changed, 18 insertions(+), 38 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-12-05 00:49:28.000000000 +0000
+++ src/gdb/infrun.c	2008-12-05 01:10:15.000000000 +0000
@@ -1311,7 +1311,7 @@ proceed (CORE_ADDR addr, enum target_sig
 
   if (addr == (CORE_ADDR) -1)
     {
-      if (pc == stop_pc && breakpoint_here_p (pc) 
+      if (breakpoint_here_p (pc)
 	  && execution_direction != EXEC_REVERSE)
 	/* There is a breakpoint at the address we will resume at,
 	   step one instruction before inserting breakpoints so that
@@ -2100,6 +2100,7 @@ handle_inferior_event (struct execution_
   int stepped_after_stopped_by_watchpoint = 0;
   struct symtab_and_line stop_pc_sal;
   enum stop_kind stop_soon;
+  CORE_ADDR stop_pc;
 
   if (ecs->ws.kind != TARGET_WAITKIND_EXITED
       && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED
@@ -3705,6 +3706,7 @@ handle_step_into_function (struct execut
 {
   struct symtab *s;
   struct symtab_and_line stop_func_sal, sr_sal;
+  CORE_ADDR stop_pc = read_pc ();
 
   s = find_pc_symtab (stop_pc);
   if (s && s->language != language_asm)
@@ -3781,6 +3783,7 @@ handle_step_into_function_backward (stru
 {
   struct symtab *s;
   struct symtab_and_line stop_func_sal, sr_sal;
+  CORE_ADDR stop_pc = read_pc ();
 
   s = find_pc_symtab (stop_pc);
   if (s && s->language != language_asm)
@@ -4283,7 +4286,7 @@ Further execution is probably impossible
 	      if (tp->stop_step
 		  && frame_id_eq (tp->step_frame_id,
 				  get_frame_id (get_current_frame ()))
-		  && step_start_function == find_pc_function (stop_pc))
+		  && step_start_function == find_pc_function (read_pc ()))
 		source_flag = SRC_LINE;	/* finished step, just print source line */
 	      else
 		source_flag = SRC_AND_LOC;	/* print location and source line */
@@ -4350,10 +4353,6 @@ Further execution is probably impossible
          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 ());
     }
 
@@ -4753,7 +4752,6 @@ signals_info (char *signum_exp, int from
 struct inferior_status
 {
   enum target_signal stop_signal;
-  CORE_ADDR stop_pc;
   bpstat stop_bpstat;
   int stop_step;
   int stop_stack_dummy;
@@ -4792,7 +4790,6 @@ save_inferior_status (int restore_stack_
   struct inferior *inf = current_inferior ();
 
   inf_status->stop_signal = tp->stop_signal;
-  inf_status->stop_pc = stop_pc;
   inf_status->stop_step = tp->stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
@@ -4847,7 +4844,6 @@ restore_inferior_status (struct inferior
   struct inferior *inf = current_inferior ();
 
   tp->stop_signal = inf_status->stop_signal;
-  stop_pc = inf_status->stop_pc;
   tp->stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
Index: src/gdb/linux-fork.c
===================================================================
--- src.orig/gdb/linux-fork.c	2008-12-05 00:47:34.000000000 +0000
+++ src/gdb/linux-fork.c	2008-12-05 00:49:53.000000000 +0000
@@ -250,7 +250,6 @@ fork_load_infrun_state (struct fork_info
   registers_changed ();
   reinit_frame_cache ();
 
-  stop_pc = read_pc ();
   nullify_last_target_wait_ptid ();
 
   /* Now restore the file positions of open file descriptors.  */
Index: src/gdb/m32c-tdep.c
===================================================================
--- src.orig/gdb/m32c-tdep.c	2008-10-21 15:45:49.000000000 +0100
+++ src/gdb/m32c-tdep.c	2008-12-05 00:49:53.000000000 +0000
@@ -2306,7 +2306,7 @@ m32c_skip_trampoline_code (struct frame_
   struct gdbarch_tdep *tdep = gdbarch_tdep (get_frame_arch (frame));
 
   /* It would be nicer to simply look up the addresses of known
-     trampolines once, and then compare stop_pc with them.  However,
+     trampolines once, and then compare the stop pc with them.  However,
      we'd need to ensure that that cached address got invalidated when
      someone loaded a new executable, and I'm not quite sure of the
      best way to do that.  find_pc_partial_function does do some
Index: src/gdb/remote-mips.c
===================================================================
--- src.orig/gdb/remote-mips.c	2008-12-02 12:19:23.000000000 +0000
+++ src/gdb/remote-mips.c	2008-12-05 00:49:53.000000000 +0000
@@ -1583,7 +1583,6 @@ device is attached to the target board (
 
   reinit_frame_cache ();
   registers_changed ();
-  stop_pc = read_pc ();
   print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC);
   xfree (serial_port_name);
 }
Index: src/gdb/solib-sunos.c
===================================================================
--- src.orig/gdb/solib-sunos.c	2008-10-21 15:45:49.000000000 +0100
+++ src/gdb/solib-sunos.c	2008-12-05 00:49:53.000000000 +0000
@@ -533,7 +533,7 @@ disable_break (void)
      SunOS version we don't know it until the above code is executed.
      Grumble if we are stopped anywhere besides the breakpoint address. */
 
-  if (stop_pc != breakpoint_addr)
+  if (read_pc () != breakpoint_addr)
     {
       warning (_("stopped at unknown breakpoint while handling shared libraries"));
     }
@@ -790,6 +790,7 @@ sunos_solib_create_inferior_hook (void)
 
   if (gdbarch_decr_pc_after_break (target_gdbarch))
     {
+      CORE_ADDR stop_pc = read_pc ();
       stop_pc -= gdbarch_decr_pc_after_break (target_gdbarch);
       write_pc (stop_pc);
     }
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h	2008-12-05 00:47:34.000000000 +0000
+++ src/gdb/target.h	2008-12-05 00:49:53.000000000 +0000
@@ -628,7 +628,7 @@ extern void target_resume (ptid_t ptid, 
    _NOT_ OK to throw_exception() out of target_wait() without popping
    the debugging target from the stack; GDB isn't prepared to get back
    to the prompt with a debugging target but without the frame cache,
-   stop_pc, etc., set up.  */
+   etc., set up.  */
 
 #define	target_wait(ptid, status)		\
      (*current_target.to_wait) (ptid, status)
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-12-05 00:47:41.000000000 +0000
+++ src/gdb/thread.c	2008-12-05 00:49:53.000000000 +0000
@@ -766,14 +766,6 @@ switch_to_thread (ptid_t ptid)
   inferior_ptid = ptid;
   reinit_frame_cache ();
   registers_changed ();
-
-  /* We don't check for is_stopped, because we're called at times
-     while in the TARGET_RUNNING state, e.g., while handling an
-     internal event.  */
-  if (!is_exited (ptid) && !is_executing (ptid))
-    stop_pc = read_pc ();
-  else
-    stop_pc = ~(CORE_ADDR) 0;
 }
 
 static void
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 00:47:41.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 00:49:53.000000000 +0000
@@ -144,10 +144,6 @@ static char *inferior_io_terminal;
 
 ptid_t inferior_ptid;
 
-/* Address at which inferior stopped.  */
-
-CORE_ADDR stop_pc;
-
 /* Flag indicating that a command has proceeded the inferior past the
    current breakpoint.  */
 
@@ -805,6 +801,7 @@ step_1 (int skip_subroutines, int single
       for (; count > 0; count--)
 	{
 	  struct thread_info *tp = inferior_thread ();
+	  CORE_ADDR stop_pc = read_pc ();
 	  clear_proceed_status ();
 
 	  frame = get_current_frame ();
@@ -924,14 +921,17 @@ step_once (int skip_subroutines, int sin
 	 the longjmp breakpoint was not required.  Use the
 	 INFERIOR_PTID thread instead, which is the same thread when
 	 THREAD is set.  */
-      struct thread_info *tp = inferior_thread ();
+      struct thread_info *tp;
+      CORE_ADDR stop_pc;
+
+      tp = inferior_thread ();
       clear_proceed_status ();
 
       frame = get_current_frame ();
-      if (!frame)		/* Avoid coredump here.  Why tho? */
-	error (_("No current frame"));
       tp->step_frame_id = get_frame_id (frame);
 
+      stop_pc = read_pc ();
+
       if (!single_inst)
 	{
 	  find_pc_line_pc_range (stop_pc,
@@ -1149,7 +1149,7 @@ signal_command (char *signum_exp, int fr
      FIXME: Neither should "signal foo" but when I tried passing
      (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
      tried to track down yet.  */
-  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
+  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : read_pc (), oursig, 0);
 }
 
 /* Proceed until we reach a different source line with pc greater than
@@ -1585,8 +1585,7 @@ program_info (char *args, int from_tty)
   stat = bpstat_num (&bs, &num);
 
   target_files_info ();
-  printf_filtered (_("Program stopped at %s.\n"),
-		   hex_string ((unsigned long) stop_pc));
+  printf_filtered (_("Program stopped at %s.\n"), paddr_nz (read_pc ()));
   if (tp->stop_step)
     printf_filtered (_("It stopped after being stepped.\n"));
   else if (stat != 0)
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h	2008-12-05 00:47:40.000000000 +0000
+++ src/gdb/inferior.h	2008-12-05 00:49:53.000000000 +0000
@@ -271,10 +271,6 @@ extern void interrupt_target_1 (int all_
 
 extern void detach_command (char *, int);
 
-/* Address at which inferior stopped.  */
-
-extern CORE_ADDR stop_pc;
-
 /* Flag indicating that a command has proceeded the inferior past the
    current breakpoint.  */
 
Index: src/gdb/fork-child.c
===================================================================
--- src.orig/gdb/fork-child.c	2008-12-05 00:51:40.000000000 +0000
+++ src/gdb/fork-child.c	2008-12-05 00:52:00.000000000 +0000
@@ -530,8 +530,6 @@ startup_inferior (int ntraps)
 
   /* Mark all threads non-executing.  */
   set_executing (pid_to_ptid (-1), 0);
-
-  stop_pc = read_pc ();
 }
 
 /* Implement the "unset exec-wrapper" command.  */

Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Doug Evans]

On Thu, Dec 4, 2008 at 5:15 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 05 December 2008 00:36:56, Pedro Alves wrote:
>> On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
>> > Pedro Alves wrote:
>> > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
>> > > > In the original code, is there a case when stop_pc != registers.pc?
>> > >
>> > > Here,
>> > >
>> > > <stopped at 0x1234, thread 1>
>> > >  (gdb) set $pc = 0xf00
>> > >  (gdb) call func()
>> >
>> > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
>> > incorrectly: for example, the check whether we are about to continue
>> > at a breakpoint will look at stop_pc, but then continue at $pc.
>>
>> This one I believe was the original intention.  The rationale being
>> that you'd not want to hit a breakpoint again at stop_pc (0x1234),
>> because there's where you stopped; but, you'd want to hit a a breakpoint
>> at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
>>
>> Note, I'm not saying I agree with this.  I did say that probably nobody
>> would notice if we got rid of stop_pc.
>>
>> > It seems to me just about every current user of stop_pc *really* wants
>> > to look at regcache_read_pc (get_current_regcache ()) ...
>
> Is using read_pc instead OK with you?  It's what I had written already.
>
>> I've been sneaking the idea of getting rid of stop_pc for a while now:
>>  http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html
>>
>> In fact, I have a months old patch here that completelly removes stop_pc.
>> IIRC, there were no visible changes in the testsuite.  Say the word,
>> and I'll brush it up, regtest, submit it.
>
> Here it is, it still applied cleanly.  It's smallish.  Regtested on
> x86-64-unknown-linux-gnu.
>
> My original motivation was to get rid of the ugly checks
> in switch_to_thread, and to try to minimize the extra thread
> switching and register reads in non-stop mode.
>
> I had held posting this when I wrote it, since I was not sure we'd not
> miss stop_pc in some case.
>
> --
> Pedro Alves
>

Nit.

The check for !frame isn't related to removing stop_pc.  Separate
patch or add a changelog entry?

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 00:47:41.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 00:49:53.000000000 +0000
@@ -924,14 +921,17 @@ step_once (int skip_subroutines, int sin
 	 the longjmp breakpoint was not required.  Use the
 	 INFERIOR_PTID thread instead, which is the same thread when
 	 THREAD is set.  */
-      struct thread_info *tp = inferior_thread ();
+      struct thread_info *tp;
+      CORE_ADDR stop_pc;
+
+      tp = inferior_thread ();
       clear_proceed_status ();

       frame = get_current_frame ();
-      if (!frame)		/* Avoid coredump here.  Why tho? */
-	error (_("No current frame"));
       tp->step_frame_id = get_frame_id (frame);

+      stop_pc = read_pc ();
+
       if (!single_inst)
 	{
 	  find_pc_line_pc_range (stop_pc,

Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On Friday 05 December 2008 01:49:02, Doug Evans wrote:

> Nit.
> 
> The check for !frame isn't related to removing stop_pc.  Separate
> patch or add a changelog entry?

Oops, sorry.  That is dead code.  There is always a frame nowadays.
get_current_frame either returns a frame, or throws --- never
returns NULL.

I've checked in the below.

-- 
Pedro Alves

[-- Attachment #2: no_frame_core_dump.diff --]
[-- Type: text/x-diff, Size: 665 bytes --]

2008-12-05  Pedro Alves  <pedro@codesourcery.com>

	* infcmd.c (step_once): Remove dead code.

---
 gdb/infcmd.c |    2 --
 1 file changed, 2 deletions(-)

Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2008-12-05 01:57:41.000000000 +0000
+++ src/gdb/infcmd.c	2008-12-05 02:00:17.000000000 +0000
@@ -928,8 +928,6 @@ step_once (int skip_subroutines, int sin
       clear_proceed_status ();
 
       frame = get_current_frame ();
-      if (!frame)		/* Avoid coredump here.  Why tho? */
-	error (_("No current frame"));
       tp->step_frame_id = get_frame_id (frame);
 
       if (!single_inst)

Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves]

On Friday 05 December 2008 01:15:30, Pedro Alves wrote:
> On Friday 05 December 2008 00:36:56, Pedro Alves wrote:
> > On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
> > > Pedro Alves wrote:
> > > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > > > > In the original code, is there a case when stop_pc != registers.pc?
> > > > 
> > > > Here,
> > > > 
> > > > <stopped at 0x1234, thread 1>
> > > >  (gdb) set $pc = 0xf00
> > > >  (gdb) call func()
> > > 
> > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > incorrectly: for example, the check whether we are about to continue
> > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > 
> > This one I believe was the original intention.  The rationale being
> > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > because there's where you stopped; but, you'd want to hit a a breakpoint
> > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > 
> > Note, I'm not saying I agree with this.  I did say that probably nobody
> > would notice if we got rid of stop_pc.
> > 
> > > It seems to me just about every current user of stop_pc *really* wants
> > > to look at regcache_read_pc (get_current_regcache ()) ...
> 
> Is using read_pc instead OK with you?  It's what I had written already.
> 
> > I've been sneaking the idea of getting rid of stop_pc for a while now:
> >  http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html
> > 
> > In fact, I have a months old patch here that completelly removes stop_pc.
> > IIRC, there were no visible changes in the testsuite.  Say the word,
> > and I'll brush it up, regtest, submit it.
> 
> Here it is, it still applied cleanly.  It's smallish.  Regtested on
> x86-64-unknown-linux-gnu.
> 
> My original motivation was to get rid of the ugly checks
> in switch_to_thread, and to try to minimize the extra thread
> switching and register reads in non-stop mode.
> 
> I had held posting this when I wrote it, since I was not sure we'd not
> miss stop_pc in some case.
> 

I should say that I also considered going the other direction and
adding a stop_pc per thread for use in `proceed', while still
replacing most other references to stop_pc by read_pc.

Say something like, in all-stop mode, the thread that hit the breakpoint
would have stop_pc set to read_pc, and all the other threads would have it
set to say (CORE_ADDR) -1.  This was to consider the following case:

Say where you're debugging an inferior with 2 threads, and two distinct breakpoints
installed.  thread 1 has just reported a hit on breakpoint 1; and thread 2
happens to have hit breakpoint 2 simultaneously, but, since events are serialized,
linux-nat.c:cancel_breakpoint took action, so thread 2 got the PC rolled back.

The user does:

(gdb) b foo
<bpkt 1>
(gdb) b bar
<bpkt 2>
(gdb) continue
<thread 1, bpk1 1 hit>
(gdb) thread 2
<GDB reads stop_pc of thread 2, which is now pointing at a breakpoint 2>
(gdb) delete 1

 proceed'ing now doesn't need to step over
 breakpoint 1 (prepare_to_proceed will do nothing), since breakpoint 1
 is gone.

(gdb) continue

At this point, current GDB will step over breakpoint 2, although it
was never reported as hit.

proceed:
  if (addr == (CORE_ADDR) -1)
    {
      if (pc == stop_pc && breakpoint_here_p (pc) 
          ^^^^^^^^^^^^^

If this was instead tp->stop_pc, and it was -1 at this point, because
thread 2 was not the thread that reported the last event, breakpoint 2
would not be missed.

-- 
Pedro Alves

Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Ulrich Weigand]

Pedro Alves wrote:

> > > > <stopped at 0x1234, thread 1>
> > > >  (gdb) set $pc = 0xf00
> > > >  (gdb) call func()
> > > 
> > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > incorrectly: for example, the check whether we are about to continue
> > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > 
> > This one I believe was the original intention.  The rationale being
> > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > because there's where you stopped; but, you'd want to hit a a breakpoint
> > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > 
> > Note, I'm not saying I agree with this.  I did say that probably nobody
> > would notice if we got rid of stop_pc.

OK, I see.  This is a valid use case, and it may make sense to keep it.
However, as you point out, to make this really work as intended, we'd
have make stop_pc a per-thread variable.

And even in that case, the uses of stop_pc in step_1 and step_once seem
invalid to me.

> > > It seems to me just about every current user of stop_pc *really* wants
> > > to look at regcache_read_pc (get_current_regcache ()) ...
> 
> Is using read_pc instead OK with you?  It's what I had written already.

I guess so; longer term read_pc should probably die, but for now it's
not really worse than the alternative.

> @@ -3705,6 +3706,7 @@ handle_step_into_function (struct execut
>  {
>    struct symtab *s;
>    struct symtab_and_line stop_func_sal, sr_sal;
> +  CORE_ADDR stop_pc = read_pc ();
>  
>    s = find_pc_symtab (stop_pc);
>    if (s && s->language != language_asm)
> @@ -3781,6 +3783,7 @@ handle_step_into_function_backward (stru
>  {
>    struct symtab *s;
>    struct symtab_and_line stop_func_sal, sr_sal;
> +  CORE_ADDR stop_pc = read_pc ();
>  
>    s = find_pc_symtab (stop_pc);
>    if (s && s->language != language_asm)

These could probably receive the stop_pc from handle_inferior_event
instead of recomputing it.

> @@ -4283,7 +4286,7 @@ Further execution is probably impossible
>  	      if (tp->stop_step
>  		  && frame_id_eq (tp->step_frame_id,
>  				  get_frame_id (get_current_frame ()))
> -		  && step_start_function == find_pc_function (stop_pc))
> +		  && step_start_function == find_pc_function (read_pc ()))
>  		source_flag = SRC_LINE;	/* finished step, just print source line */
>  	      else
>  		source_flag = SRC_AND_LOC;	/* print location and source line */

As Andrew's comment notes, the function comparison should be redundant
these days as it is already implied in the frame-ID comparison.

> @@ -1149,7 +1149,7 @@ signal_command (char *signum_exp, int fr
>       FIXME: Neither should "signal foo" but when I tried passing
>       (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
>       tried to track down yet.  */
> -  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
> +  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : read_pc (), oursig, 0);
>  }
>  
>  /* Proceed until we reach a different source line with pc greater than

Dan wanted to get rid of this use of stop_pc anyway, see:
http://sourceware.org/ml/gdb-patches/2008-08/msg00651.html

> @@ -1585,8 +1585,7 @@ program_info (char *args, int from_tty)
>    stat = bpstat_num (&bs, &num);
>  
>    target_files_info ();
> -  printf_filtered (_("Program stopped at %s.\n"),
> -		   hex_string ((unsigned long) stop_pc));
> +  printf_filtered (_("Program stopped at %s.\n"), paddr_nz (read_pc ()));
>    if (tp->stop_step)
>      printf_filtered (_("It stopped after being stepped.\n"));
>    else if (stat != 0)

If we keep a tp->stop_pc, this place should also make use of it;
otherwise the message isn't really valid (and not very useful:
if it always just prints $pc, it would be redundant with the
other commands to do so ...).

Bye,
Ulrich

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

Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves]

On Friday 05 December 2008 18:42:45, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > > > > <stopped at 0x1234, thread 1>
> > > > >  (gdb) set $pc = 0xf00
> > > > >  (gdb) call func()
> > > > 
> > > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > > incorrectly: for example, the check whether we are about to continue
> > > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > > 
> > > This one I believe was the original intention.  The rationale being
> > > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > > because there's where you stopped; but, you'd want to hit a a breakpoint
> > > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > > 
> > > Note, I'm not saying I agree with this.  I did say that probably nobody
> > > would notice if we got rid of stop_pc.
> 
> OK, I see.  This is a valid use case, and it may make sense to keep it.
> However, as you point out, to make this really work as intended, we'd
> have make stop_pc a per-thread variable.
> 
> And even in that case, the uses of stop_pc in step_1 and step_once seem
> invalid to me.
> 

100% Agreed.  I'll take care of it.

> > @@ -3705,6 +3706,7 @@ handle_step_into_function (struct execut
> >  {
> >    struct symtab *s;
> >    struct symtab_and_line stop_func_sal, sr_sal;
> > +  CORE_ADDR stop_pc = read_pc ();
> >  
> >    s = find_pc_symtab (stop_pc);
> >    if (s && s->language != language_asm)
> > @@ -3781,6 +3783,7 @@ handle_step_into_function_backward (stru
> >  {
> >    struct symtab *s;
> >    struct symtab_and_line stop_func_sal, sr_sal;
> > +  CORE_ADDR stop_pc = read_pc ();
> >  
> >    s = find_pc_symtab (stop_pc);
> >    if (s && s->language != language_asm)
> 
> These could probably receive the stop_pc from handle_inferior_event
> instead of recomputing it.

Right.  It would hit the cache, but, then again, if/when we have
a stop_pc per-thread, we'd use that.

> 
> > @@ -4283,7 +4286,7 @@ Further execution is probably impossible
> >  	      if (tp->stop_step
> >  		  && frame_id_eq (tp->step_frame_id,
> >  				  get_frame_id (get_current_frame ()))
> > -		  && step_start_function == find_pc_function (stop_pc))
> > +		  && step_start_function == find_pc_function (read_pc ()))
> >  		source_flag = SRC_LINE;	/* finished step, just print source line */
> >  	      else
> >  		source_flag = SRC_AND_LOC;	/* print location and source line */
> 
> As Andrew's comment notes, the function comparison should be redundant
> these days as it is already implied in the frame-ID comparison.
> 

Oh, that's what that comment means?  I always had trouble parsing the
English in it.  Makes sense.

> > @@ -1149,7 +1149,7 @@ signal_command (char *signum_exp, int fr
> >       FIXME: Neither should "signal foo" but when I tried passing
> >       (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
> >       tried to track down yet.  */
> > -  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
> > +  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : read_pc (), oursig, 0);
> >  }
> >  
> >  /* Proceed until we reach a different source line with pc greater than
> 
> Dan wanted to get rid of this use of stop_pc anyway, see:
> http://sourceware.org/ml/gdb-patches/2008-08/msg00651.html

Yep.  I think his patch makes sense:
 http://sourceware.org/ml/gdb-patches/2008-11/msg00439.html

> 
> > @@ -1585,8 +1585,7 @@ program_info (char *args, int from_tty)
> >    stat = bpstat_num (&bs, &num);
> >  
> >    target_files_info ();
> > -  printf_filtered (_("Program stopped at %s.\n"),
> > -		   hex_string ((unsigned long) stop_pc));
> > +  printf_filtered (_("Program stopped at %s.\n"), paddr_nz (read_pc ()));
> >    if (tp->stop_step)
> >      printf_filtered (_("It stopped after being stepped.\n"));
> >    else if (stat != 0)
> 
> If we keep a tp->stop_pc, this place should also make use of it;
> otherwise the message isn't really valid (and not very useful:
> if it always just prints $pc, it would be redundant with the
> other commands to do so ...).

Right you are.

I'll fix the step_1/step_once bit for now.

-- 
Pedro Alves

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

[Ulrich Weigand]

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

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

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

ref: http://sourceware.org/ml/gdb-patches/2008-11/msg00454.html

On Tue, Nov 18, 2008 at 4:58 AM, Doug Evans <dje@google.com> wrote:
> [...]
> There are two things this patch does:
> 1) properly pop dummy frames more often
> 2) make the inferior resumable after an inferior function call gets a
>   signal without having to remember to do "signal 0"
> [...]
> Restoring stop_signal also has the benefit that if an inferior function
> call is made after getting a signal, and the inferior function call
> doesn't complete (e.g. has a breakpoint and doesn't complete right away),
> the user can resume the program (after popping the inf fun call off the
> stack) with "continue" without having to remember what the signal was
> and remember to use "signal N".
> [...]

Hi.  This patch is an update to the testcases for this patch,
call-signal-resume.exp now tests for the above case.

Comments?

[-- Attachment #2: gdb-081125-infcall-tests-1.patch.txt --]
[-- Type: text/plain, Size: 12547 bytes --]

2008-11-25  Doug Evans  <dje@google.com>

	* gdb.base/call-signal-resume.exp: New file.
	* gdb.base/call-signals.c: New file.
	* gdb.base/unwindonsignal.exp: New file.
	* gdb.base/unwindonsignal.c: New file.

Index: call-signal-resume.exp
===================================================================
RCS file: call-signal-resume.exp
diff -N call-signal-resume.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ call-signal-resume.exp	26 Nov 2008 01:51:04 -0000
@@ -0,0 +1,157 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test inferior resumption after discarding a hand-called function.
+# There are two things to test.
+# 1) Inferior stops normally.  Upon resumption it should continue normally,
+#    regardless of whatever signal the hand-called function got.
+# 2) Inferior is stopped at a signal.  Upon resumption it should continue
+#    with that signal, regardless of whatever the hand-called function did.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping call-signal-resume.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "call-signals"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested call-signal-resume.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+proc get_dummy_frame_number { } {
+  global gdb_prompt
+
+  send_gdb "bt\n"
+  gdb_expect {
+    -re "#(\[0-9\]*) *<function called from gdb>.*$gdb_prompt $"
+      {
+	return $expect_out(1,string)
+      }
+    -re "$gdb_prompt $"
+      {
+	return ""
+      }
+    timeout
+      {
+	return ""
+      }
+  }
+  return ""
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break stop_one" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Continuing.*Breakpoint \[0-9\]*, stop_one.*" \
+    "continue to breakpoint at stop_one"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"call-signal-resume, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "call-signal-resume, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "call-signal-resume, inferior function call signaled"
+    }
+}
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "call-signal-resume, dummy stack frame number"
+    setup_xfail "*-*-*"
+} else {
+    pass "call-signal-resume, dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Resume execution, the program should continue without any signal.
+
+gdb_test "break stop_two" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Breakpoint \[0-9\]*, stop_two.*" \
+    "continue to breakpoint at stop_two"
+
+# Continue again, we should get a signal.
+
+gdb_test "continue" "Program received signal .*" \
+    "continue to receipt of signal"
+
+# Hand call another function that prematurely stops,
+# then manually pop the dummy stack frame.
+
+gdb_test "break null_hand_call" "Breakpoint \[0-9\]* at .*"
+gdb_test "call null_hand_call ()" "Breakpoint \[0-9\]*, null_hand_call.*" \
+    "null_hand_call"
+
+set frame_number [get_dummy_frame_number]
+if { "$frame_number" == "" } {
+    fail "call-signal-resume, dummy stack frame number"
+    setup_xfail "*-*-*"
+} else {
+    pass "call-signal-resume, dummy stack frame number"
+}
+
+# Pop the dummy frame.
+gdb_test "frame $frame_number" ""
+gdb_test "set confirm off" ""
+gdb_test "return" ""
+
+# Continue again, this time we should get to the signal handler.
+
+gdb_test "break handle_signal" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Breakpoint \[0-9\]*, handle_signal.*" \
+    "continue to breakpoint at handle_signal"
+
+# Continue one last time, the program should exit normally.
+
+gdb_test "continue" "Program exited normally." \
+    "continue to program exit"
+
+return 0
Index: call-signals.c
===================================================================
RCS file: call-signals.c
diff -N call-signals.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ call-signals.c	26 Nov 2008 01:51:04 -0000
@@ -0,0 +1,89 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing handling of inferior function calls
+   in the presence of signals.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+handle_signal (int sig)
+{
+}
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+/* Easy place to set a breakpoint.  */
+
+void
+stop_one ()
+{
+}
+
+void
+stop_two ()
+{
+}
+
+void
+null_hand_call ()
+{
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+
+#ifdef SIG_SETMASK
+  /* Ensure all the signals aren't blocked.
+     The environment in which the testsuite is run may have blocked some
+     for whatever reason.  */
+  {
+    sigset_t newset;
+    sigemptyset (&newset);
+    sigprocmask (SIG_SETMASK, &newset, NULL);
+  }
+#endif
+
+  signal (SIGABRT, handle_signal);
+
+  /* Stop here so we can hand-call gen_signal.  */
+  stop_one ();
+
+  /* When we're resumed stop here.  */
+  stop_two ();
+
+  /* When we're resumed we generate a signal ourselves.  */
+  gen_signal ();
+
+  return 0;
+}
Index: unwindonsignal.c
===================================================================
RCS file: unwindonsignal.c
diff -N unwindonsignal.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ unwindonsignal.c	26 Nov 2008 01:51:04 -0000
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Support program for testing unwindonsignal.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+void
+gen_signal ()
+{
+  /* According to sigall.exp, SIGABRT is always supported.  */
+#ifdef SIGABRT
+  kill (getpid (), SIGABRT);
+#endif
+  /* If we get here we couldn't generate a signal, tell dejagnu.  */
+  printf ("no signal\n");
+}
+
+/* Easy place to set a breakpoint.  */
+
+void
+stop_here ()
+{
+}
+
+int
+main ()
+{
+#ifdef usestubs
+  set_debug_traps ();
+  breakpoint ();
+#endif
+
+#ifdef SIG_SETMASK
+  /* Ensure all the signals aren't blocked.
+     The environment in which the testsuite is run may have blocked some
+     for whatever reason.  */
+  {
+    sigset_t newset;
+    sigemptyset (&newset);
+    sigprocmask (SIG_SETMASK, &newset, NULL);
+  }
+#endif
+
+  /* Stop here so we can hand-call gen_signal.  */
+  stop_here ();
+
+  return 0;
+}
Index: unwindonsignal.exp
===================================================================
RCS file: unwindonsignal.exp
diff -N unwindonsignal.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ unwindonsignal.exp	26 Nov 2008 01:51:04 -0000
@@ -0,0 +1,98 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+if [target_info exists gdb,noinferiorio] {
+    verbose "Skipping unwindonsignal.exp because of no fileio capabilities."
+    continue
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "unwindonsignal"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested unwindonsignal.exp
+     return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break stop_here" "Breakpoint \[0-9\]* at .*"
+gdb_test "continue" "Continuing.*Breakpoint \[0-9\]*, stop_here.*" \
+    "continue to breakpoint at stop_here"
+
+# Turn on unwindonsignal.
+gdb_test "set unwindonsignal on" \
+	"" \
+	"setting unwindonsignal"
+gdb_test "show unwindonsignal" \
+	"Unwinding of stack .* is on." \
+	"showing unwindonsignal"
+
+# Call function (causing the program to get a signal), and see if gdb handles
+# it properly.
+gdb_test_multiple "call gen_signal ()" \
+	"unwindonsignal, inferior function call signaled" {
+    -re "\[\r\n\]*no signal\[\r\n\]+$gdb_prompt $" {
+	unsupported "unwindonsignal, inferior function call signaled"
+	return 0
+    }
+    -re "\[\r\n\]*The program being debugged was signaled.*\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, inferior function call signaled"
+    }
+}
+
+# Verify the stack got unwound.
+gdb_test "bt" \
+    "#0 *\[x0-9a-f in\]*stop_here \\(.*\\) at .*#1 *\[x0-9a-f in\]*main \\(.*\\) at .*" \
+	"unwindonsignal, stack unwound"
+
+# Verify the dummy frame got removed from dummy_frame_stack.
+gdb_test_multiple "maint print dummy-frames" \
+	"unwindonsignal, dummy frame removed" {
+    -re "\[\r\n\]*.*stack=.*code=.*\[\r\n\]+$gdb_prompt $" {
+	fail "unwindonsignal, dummy frame removed"
+    }
+    -re "\[\r\n\]+$gdb_prompt $" {
+	pass "unwindonsignal, dummy frame removed"
+    }
+}
+
+return 0