[RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Doug Evans]

Hi.  This fixes a bug where a signal in an inferior function call with
unwindonsignal set doesn't remove the dummy frame from dummy_frame_stack.
A testcase is included to exercise the bug.

It also adds a comment to clear up something that was a bit confusing
at first.  The lone frame_pop isn't enough to properly restore program
state, but things still work because registers (and thus stack) get
restored from inf_status.

Tested on i386-linux.

Ok to check in?

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

	* infcall.c (call_function_by_hand): Properly pop the dummy frame
	if the inferior gets a signal and unwindonsignal is set.

	* gdb.base/unwindonsignal.c: New file.
	* gdb.base/unwindonsignal.exp: New file.

Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.105
diff -u -p -u -p -r1.105 infcall.c
--- infcall.c	12 Nov 2008 00:39:28 -0000	1.105
+++ infcall.c	15 Nov 2008 22:08:54 -0000
@@ -357,7 +358,10 @@ call_function_by_hand (struct value *fun
   /* Save the caller's registers 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).  */
+     is tossed once the regcache has been pushed).
+     NOTE: If the inferior gets a signal and unwindonsignal is set,
+     then this regcache is not used to restore target regs.  Instead
+     the regcache in inf_status is used.  */
   caller_regcache = frame_save_as_regcache (frame);
   caller_regcache_cleanup = make_cleanup_regcache_xfree (caller_regcache);
 
@@ -746,8 +749,12 @@ 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 ());
+		 dummy call.
+		 NOTE: This discards the regcache recorded in the dummy frame
+		 but that's ok, the registers will get restored from the
+		 regcache in inf_status (which gets called by a cleanup).  */
+	      dummy_frame_pop (dummy_id);
+	      reinit_frame_cache ();
 
 	      /* FIXME: Insert a bunch of wrap_here; name can be very
 		 long if it's a C++ name with arguments and stuff.  */
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	16 Nov 2008 01:11:22 -0000
@@ -0,0 +1,49 @@
+/* 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 unwindonsignal.  */
+
+#include <stdio.h>
+#include <signal.h>
+#include <unistd.h>
+
+/* This function is handcalled from unwindonsignal.exp.  */
+
+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	16 Nov 2008 01:11:22 -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 "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
+}
+
+# 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] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Michael Snyder]

I don't understand why calling dummy_frame_pop directly works,
but calling frame_pop doesn't work.  Doesn't frame_pop call
dummy_frame_pop?  If not, what prevents it?

Doug Evans wrote:
> Hi.  This fixes a bug where a signal in an inferior function call with
> unwindonsignal set doesn't remove the dummy frame from dummy_frame_stack.
> A testcase is included to exercise the bug.
> 
> It also adds a comment to clear up something that was a bit confusing
> at first.  The lone frame_pop isn't enough to properly restore program
> state, but things still work because registers (and thus stack) get
> restored from inf_status.
> 
> Tested on i386-linux.
> 
> Ok to check in?
> 
> 2008-11-15  Doug Evans  <dje@google.com>
> 
> 	* infcall.c (call_function_by_hand): Properly pop the dummy frame
> 	if the inferior gets a signal and unwindonsignal is set.
> 
> 	* gdb.base/unwindonsignal.c: New file.
> 	* gdb.base/unwindonsignal.exp: New file.
> 
> Index: infcall.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcall.c,v
> retrieving revision 1.105
> diff -u -p -u -p -r1.105 infcall.c
> --- infcall.c	12 Nov 2008 00:39:28 -0000	1.105
> +++ infcall.c	15 Nov 2008 22:08:54 -0000
> @@ -357,7 +358,10 @@ call_function_by_hand (struct value *fun
>    /* Save the caller's registers 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).  */
> +     is tossed once the regcache has been pushed).
> +     NOTE: If the inferior gets a signal and unwindonsignal is set,
> +     then this regcache is not used to restore target regs.  Instead
> +     the regcache in inf_status is used.  */
>    caller_regcache = frame_save_as_regcache (frame);
>    caller_regcache_cleanup = make_cleanup_regcache_xfree (caller_regcache);
>  
> @@ -746,8 +749,12 @@ 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 ());
> +		 dummy call.
> +		 NOTE: This discards the regcache recorded in the dummy frame
> +		 but that's ok, the registers will get restored from the
> +		 regcache in inf_status (which gets called by a cleanup).  */
> +	      dummy_frame_pop (dummy_id);
> +	      reinit_frame_cache ();
>  
>  	      /* FIXME: Insert a bunch of wrap_here; name can be very
>  		 long if it's a C++ name with arguments and stuff.  */
> 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	16 Nov 2008 01:11:22 -0000
> @@ -0,0 +1,49 @@
> +/* 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 unwindonsignal.  */
> +
> +#include <stdio.h>
> +#include <signal.h>
> +#include <unistd.h>
> +
> +/* This function is handcalled from unwindonsignal.exp.  */
> +
> +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	16 Nov 2008 01:11:22 -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 "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
> +}
> +
> +# 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] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Ulrich Weigand]

Doug Evans wrote:

> Hi.  This fixes a bug where a signal in an inferior function call with
> unwindonsignal set doesn't remove the dummy frame from dummy_frame_stack.
> A testcase is included to exercise the bug.

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?

In general, the list of dummy frames is expected to occasionally contains
stale entries (e.g. if the inferior jumped out of an inferior call via
longjmp).  Your test case only uses "maint pring dummy-frames" to verify
the dummy frame list -- apart from this, was there any actual problem
you experienced due to this issue?

>  	      /* We must get back to the frame we were before the
> -		 dummy call. */
> -	      frame_pop (get_current_frame ());
> +		 dummy call.
> +		 NOTE: This discards the regcache recorded in the dummy frame
> +		 but that's ok, the registers will get restored from the
> +		 regcache in inf_status (which gets called by a cleanup).  */
> +	      dummy_frame_pop (dummy_id);
> +	      reinit_frame_cache ();

Why the reinit_frame_cache here?

Michael Synder wrote:

> I don't understand why calling dummy_frame_pop directly works,
> but calling frame_pop doesn't work.  Doesn't frame_pop call
> dummy_frame_pop?  If not, what prevents it?

frame_pop will call dummy_frame_pop if you pop the dummy frame
directly.  However, in this case, frame_pop (get_current_frame ())
only pops the current frame during the inferior call sequence, 
and the dummy frame is somewhere higher on the stack.

I guess you could do a
  frame_pop (find_frame_by_id (dummy_id))
instead, but as Doug points out this is not really needed because
of the outstanding inf_status cleanup.

On the other hand, I'm wondering if we really need the inf_status
cleanup in the first place.  It supposed to protect against errors
during the initial setup of the registers, see the comment in infrun.c:

  /* 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;

I'm not sure what the point is of having two copies of the regcache
preserved across the full extent of the inferior call ...

Bye,
Ulrich

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

Re: [RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Daniel Jacobowitz]

On Mon, Nov 17, 2008 at 08:50:13PM +0100, Ulrich Weigand wrote:
> I'm not sure what the point is of having two copies of the regcache
> preserved across the full extent of the inferior call ...

Maybe in case we get to somewhere we can not backtrace from?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Mon, Nov 17, 2008 at 08:50:13PM +0100, Ulrich Weigand wrote:
> > I'm not sure what the point is of having two copies of the regcache
> > preserved across the full extent of the inferior call ...
> 
> Maybe in case we get to somewhere we can not backtrace from?

OK, good point.  The inf_status will always work ...

Bye,
Ulrich

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

Re: [RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Doug Evans]

On Mon, Nov 17, 2008 at 12:42 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Daniel Jacobowitz wrote:
>> On Mon, Nov 17, 2008 at 08:50:13PM +0100, Ulrich Weigand wrote:
>> > I'm not sure what the point is of having two copies of the regcache
>> > preserved across the full extent of the inferior call ...
>>
>> Maybe in case we get to somewhere we can not backtrace from?
>
> OK, good point.  The inf_status will always work ...

I don't understand why two copies are needed.
Can either of you elaborate?

Re: [RFA] fix popping of dummy frame if inferior gets signal with unwindonsignal

[Daniel Jacobowitz]

On Mon, Nov 17, 2008 at 02:37:10PM -0800, Doug Evans wrote:
> On Mon, Nov 17, 2008 at 12:42 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > Daniel Jacobowitz wrote:
> >> On Mon, Nov 17, 2008 at 08:50:13PM +0100, Ulrich Weigand wrote:
> >> > I'm not sure what the point is of having two copies of the regcache
> >> > preserved across the full extent of the inferior call ...
> >>
> >> Maybe in case we get to somewhere we can not backtrace from?
> >
> > OK, good point.  The inf_status will always work ...
> 
> I don't understand why two copies are needed.
> Can either of you elaborate?

Hypothetically, if you call a function which completely trashes the
registers but not the previous stack, we might not be able to find the
dummy frame to pop.  So having it saved somewhere else seems
reasonable.  I don't know how to structure that in a useful
least-surprising way though...

-- 
Daniel Jacobowitz
CodeSourcery