[patch] infcall: Remove gdb_assert ($sp overflow)

[patch] infcall: Remove gdb_assert ($sp overflow)

[Jan Kratochvil]

Hi,

set $sp=0
call something()
->
../../gdb/infcall.c:521: internal-error: call_function_by_hand: Assertion
`(gdbarch_inner_than (gdbarch, 1, 2) && sp <= old_sp) || (gdbarch_inner_than
(gdbarch, 2, 1) && sp >= old_sp)' failed.

as $sp - frame == 0xffffsmth which is not lower than $sp.

It must not be gdb_assert().  It can be an error() but I left it just to do:
	(gdb) set $sp=0
	(gdb) call doubleit (1)
	Cannot access memory at address 0xffffffffffffff78
	(gdb) set $sp=-1
	(gdb) call doubleit (1)
	Cannot access memory at address 0xffffffffffffff68

Unaware how inconvenient is an intentional crash of the testcase on some
embedded/non-MMU systems.
No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-02-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infcall.c (call_function_by_hand): Remove gdb_assert on sp and old_sp.
	New comment.

gdb/testsuite/
2010-02-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/callfuncs.exp: New tests for $spval 0 and -1.  Remove return.

--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -518,10 +518,9 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	      /* Stack grows up.  */
 	      sp = gdbarch_frame_align (gdbarch, old_sp + 1);
 	  }
-	gdb_assert ((gdbarch_inner_than (gdbarch, 1, 2)
-		    && sp <= old_sp)
-		    || (gdbarch_inner_than (gdbarch, 2, 1)
-		       && sp >= old_sp));
+	/* SP may have overflown address zero here from OLD_SP.  Memory access
+	   functions will probably fail in such case but that is a target's
+	   problem.  */
       }
     else
       /* FIXME: cagney/2002-09-18: Hey, you loose!
--- a/gdb/testsuite/gdb.base/callfuncs.exp
+++ b/gdb/testsuite/gdb.base/callfuncs.exp
@@ -469,5 +469,18 @@ if {$old_reg_content == $new_reg_content} then {
     fail "nested call dummies preserve register contents"
 }
 
-return 0
+# GDB should not crash by internal error on $sp overflow during the inferior
+# call.  It is OK it will stop on some: Cannot access memory at address 0x$hex.
 
+foreach spval {0 -1} {
+    set old_ldprefix $pf_prefix
+    lappend pf_prefix "sp=$spval:"
+
+    gdb_test {set $old_sp = $sp}
+    gdb_test "set \$sp = $spval"
+
+    gdb_test "call doubleit (1)"
+
+    gdb_test {set $sp = $old_sp}
+    set pf_prefix $old_ldprefix
+}

Re: [patch] infcall: Remove gdb_assert ($sp overflow)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> set $sp=0
Jan> call something()

I don't really have much problem with this patch, I guess, but do people
really do this sort of thing?  Or is this a reduced case of some other
scenario that actually does happen?

Tom

Re: [patch] infcall: Remove gdb_assert ($sp underflow)

[Jan Kratochvil]

On Fri, 26 Feb 2010 23:45:10 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> set $sp=0
> Jan> call something()
[...]
> do people really do this sort of thing?  Or is this a reduced case of some
> other scenario that actually does happen?

$sp=0 is a perfectly valid 16bit embedded device initialization with ROM 0..16KB
and RAM 16KB..64KB address range with normal PUSH as *--$sp (and POP as *sp++).


But it is true I have met this case as a consequence of a different problem.
Assuming it is a bug in the ia64 part of the subsystem in Linux kernel for
ptrace emulated on top of utrace (that is RHEL-5, such as RHEL-5.4).

Reproducer instructions:
	http://cvs.fedoraproject.org/viewvc/rpms/gdb/F-12/gdb-ia64-infcall-workaround.patch?content-type=text%2Fplain&view=co
FYI the ia64 kernel may lock up while dealing with this reproducer.

After an inferior call and some commands inferior $sp gets read by ptrace as 0.

The problem happens since arch-independent change:
	Re: [rfc, v3] Fix frame_id_inner comparison false positives
	http://sourceware.org/ml/gdb-patches/2008-08/msg00578.html
	http://sourceware.org/ml/gdb-cvs/2008-08/msg00182.html
	916dde5d38b45a659514e47942ece70aec04cd78
specifically its last part:
	* stack.c (return_command): Directly pop the selected frame.
which is at the bottom of this mail.

I have not found there a bug in this GDB change.  The problem is also not
reproducible on ia64 RHEL-4 (RHEL-4.8) which uses non-utrace legacy ptrace
implementation in its Linux kernel.


Thanks,
Jan


--- src/gdb/stack.c	2008/08/21 18:14:39	1.176
+++ src/gdb/stack.c	2008/08/26 17:40:25	1.177
@@ -1844,29 +1844,8 @@
 	error (_("Not confirmed"));
     }
 
-  /* NOTE: cagney/2003-01-18: Is this silly?  Rather than pop each
-     frame in turn, should this code just go straight to the relevant
-     frame and pop that?  */
-
-  /* First discard all frames inner-to the selected frame (making the
-     selected frame current).  */
-  {
-    struct frame_id selected_id = get_frame_id (get_selected_frame (NULL));
-    while (!frame_id_eq (selected_id, get_frame_id (get_current_frame ())))
-      {
-	struct frame_info *frame = get_current_frame ();
-	if (frame_id_inner (get_frame_arch (frame), selected_id,
-			    get_frame_id (frame)))
-	  /* Caught in the safety net, oops!  We've gone way past the
-             selected frame.  */
-	  error (_("Problem while popping stack frames (corrupt stack?)"));
-	frame_pop (get_current_frame ());
-      }
-  }
-
-  /* Second discard the selected frame (which is now also the current
-     frame).  */
-  frame_pop (get_current_frame ());
+  /* Discard the selected frame and all frames inner-to it.  */
+  frame_pop (get_selected_frame (NULL));
 
   /* Store RETURN_VALUE in the just-returned register set.  */
   if (return_value != NULL)

Re: [patch] infcall: Remove gdb_assert ($sp overflow)

[Joel Brobecker]

> Jan> set $sp=0
> Jan> call something()
> 
> I don't really have much problem with this patch, I guess, but do people
> really do this sort of thing?  Or is this a reduced case of some other
> scenario that actually does happen?

I used to do that (setting $sp) in an ancient testcase whose purpose
was to break the callstack. It turned out that breaking the callstack
was too platform specific, so I gave up on using this.

I am guessing that some people might try setting $sp for various
purposes (maybe debugging a problem with code that misaligns SP
for instance), and a screwup might lead to this situation too...

-- 
Joel

Re: [patch] infcall: Remove gdb_assert ($sp overflow)

[Daniel Jacobowitz]

On Fri, Feb 19, 2010 at 11:48:05PM +0100, Jan Kratochvil wrote:
> Unaware how inconvenient is an intentional crash of the testcase on some
> embedded/non-MMU systems.

Maybe limit it at least by nosignals?  It might still blow up uClinux
testing, I don't know offhand if there are other crashy tests.

> +foreach spval {0 -1} {
> +    set old_ldprefix $pf_prefix
> +    lappend pf_prefix "sp=$spval:"
> +
> +    gdb_test {set $old_sp = $sp}
> +    gdb_test "set \$sp = $spval"
> +
> +    gdb_test "call doubleit (1)"
> +
> +    gdb_test {set $sp = $old_sp}
> +    set pf_prefix $old_ldprefix
> +}

Because these tests run more than once, please give them unique names.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] infcall: Remove gdb_assert ($sp underflow)

[Jan Kratochvil]

On Fri, 26 Feb 2010 23:52:52 +0100, Daniel Jacobowitz wrote:
> On Fri, Feb 19, 2010 at 11:48:05PM +0100, Jan Kratochvil wrote:
> > Unaware how inconvenient is an intentional crash of the testcase on some
> > embedded/non-MMU systems.
> 
> Maybe limit it at least by nosignals?  It might still blow up uClinux
> testing, I don't know offhand if there are other crashy tests.

OK, used:
	if {![target_info exists gdb,nosignals] && ![istarget "*-*-uclinux*"]} {


> > +foreach spval {0 -1} {
> > +    set old_ldprefix $pf_prefix
> > +    lappend pf_prefix "sp=$spval:"
> > +
> > +    gdb_test {set $old_sp = $sp}
> > +    gdb_test "set \$sp = $spval"
> > +
> > +    gdb_test "call doubleit (1)"
> > +
> > +    gdb_test {set $sp = $old_sp}
> > +    set pf_prefix $old_ldprefix
> > +}
> 
> Because these tests run more than once, please give them unique names.

The tests have been already producing:

PASS: gdb.base/callfuncs.exp: sp=0: set $old_sp = $sp
PASS: gdb.base/callfuncs.exp: sp=0: set $sp = 0
PASS: gdb.base/callfuncs.exp: sp=0: call doubleit (1)
PASS: gdb.base/callfuncs.exp: sp=0: set $sp = $old_sp
PASS: gdb.base/callfuncs.exp: sp=-1: set $old_sp = $sp
PASS: gdb.base/callfuncs.exp: sp=-1: set $sp = -1
PASS: gdb.base/callfuncs.exp: sp=-1: call doubleit (1)
PASS: gdb.base/callfuncs.exp: sp=-1: set $sp = $old_sp

But OK, made now the tests code more obvious:

PASS: gdb.base/callfuncs.exp: set $old_sp = $sp
PASS: gdb.base/callfuncs.exp: set $sp = 0
PASS: gdb.base/callfuncs.exp: sp == 0: call doubleit (1)
PASS: gdb.base/callfuncs.exp: set $sp = -1
PASS: gdb.base/callfuncs.exp: sp == -1: call doubleit (1)
PASS: gdb.base/callfuncs.exp: set $sp = $old_sp


Thanks,
Jan


gdb/
2010-02-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infcall.c (call_function_by_hand): Remove gdb_assert on sp and old_sp.
	New comment.

gdb/testsuite/
2010-02-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/callfuncs.exp: New tests for $spval 0 and -1.  Remove return.

--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -518,10 +518,9 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	      /* Stack grows up.  */
 	      sp = gdbarch_frame_align (gdbarch, old_sp + 1);
 	  }
-	gdb_assert ((gdbarch_inner_than (gdbarch, 1, 2)
-		    && sp <= old_sp)
-		    || (gdbarch_inner_than (gdbarch, 2, 1)
-		       && sp >= old_sp));
+	/* SP may have underflown address zero here from OLD_SP.  Memory access
+	   functions will probably fail in such case but that is a target's
+	   problem.  */
       }
     else
       /* FIXME: cagney/2002-09-18: Hey, you loose!
--- a/gdb/testsuite/gdb.base/callfuncs.exp
+++ b/gdb/testsuite/gdb.base/callfuncs.exp
@@ -469,5 +469,17 @@ if {$old_reg_content == $new_reg_content} then {
     fail "nested call dummies preserve register contents"
 }
 
-return 0
+# GDB should not crash by internal error on $sp underflow during the inferior
+# call.  It is OK it will stop on some: Cannot access memory at address 0x$hex.
 
+if {![target_info exists gdb,nosignals] && ![istarget "*-*-uclinux*"]} {
+    gdb_test {set $old_sp = $sp}
+
+    gdb_test {set $sp = 0}
+    gdb_test {sp == 0: call doubleit (1)}
+
+    gdb_test {set $sp = -1}
+    gdb_test {sp == -1: call doubleit (1)}
+
+    gdb_test {set $sp = $old_sp}
+}

Re: [patch] infcall: Remove gdb_assert ($sp underflow)

[Daniel Jacobowitz]

On Sat, Feb 27, 2010 at 01:46:34AM +0100, Jan Kratochvil wrote:
> The tests have been already producing:
> 
> PASS: gdb.base/callfuncs.exp: sp=0: set $old_sp = $sp
> PASS: gdb.base/callfuncs.exp: sp=0: set $sp = 0
> PASS: gdb.base/callfuncs.exp: sp=0: call doubleit (1)
> PASS: gdb.base/callfuncs.exp: sp=0: set $sp = $old_sp
> PASS: gdb.base/callfuncs.exp: sp=-1: set $old_sp = $sp
> PASS: gdb.base/callfuncs.exp: sp=-1: set $sp = -1
> PASS: gdb.base/callfuncs.exp: sp=-1: call doubleit (1)
> PASS: gdb.base/callfuncs.exp: sp=-1: set $sp = $old_sp
> 

Oh, you're right.  I missed pf_prefix.  Either way is fine with me.

> +    gdb_test {sp == 0: call doubleit (1)}

Except what does this do?  I've never seen this syntax before, I'm
surprised that it works.

I think the patch is otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] infcall: Remove gdb_assert ($sp underflow)

[Jan Kratochvil]

On Sun, 28 Feb 2010 15:35:09 +0100, Daniel Jacobowitz wrote:
> On Sat, Feb 27, 2010 at 01:46:34AM +0100, Jan Kratochvil wrote:
> > +    gdb_test {sp == 0: call doubleit (1)}
> 
> Except what does this do?  I've never seen this syntax before, I'm
> surprised that it works.

It generated false PASS - not testing the internal error at all.
	Undefined command: "sp".  Try "help".
Thanks for catching it.


> I think the patch is otherwise OK.

Checked-in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-02/msg00222.html

--- src/gdb/ChangeLog	2010/02/28 15:04:10	1.11406
+++ src/gdb/ChangeLog	2010/02/28 17:56:36	1.11407
@@ -1,3 +1,8 @@
+2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* infcall.c (call_function_by_hand): Remove gdb_assert on sp and old_sp.
+	New comment.
+
 2010-02-28  Corinna Vinschen  <vinschen@redhat.com>
 
 	* Makefile.in (SUBDIR_MI_OBS): Move mi-common.o from here...
--- src/gdb/infcall.c	2010/01/28 22:07:58	1.126
+++ src/gdb/infcall.c	2010/02/28 17:56:36	1.127
@@ -518,10 +518,9 @@
 	      /* Stack grows up.  */
 	      sp = gdbarch_frame_align (gdbarch, old_sp + 1);
 	  }
-	gdb_assert ((gdbarch_inner_than (gdbarch, 1, 2)
-		    && sp <= old_sp)
-		    || (gdbarch_inner_than (gdbarch, 2, 1)
-		       && sp >= old_sp));
+	/* SP may have underflown address zero here from OLD_SP.  Memory access
+	   functions will probably fail in such case but that is a target's
+	   problem.  */
       }
     else
       /* FIXME: cagney/2002-09-18: Hey, you loose!
--- src/gdb/testsuite/ChangeLog	2010/02/26 05:50:35	1.2154
+++ src/gdb/testsuite/ChangeLog	2010/02/28 17:56:37	1.2155
@@ -1,3 +1,8 @@
+2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Daniel Jacobowitz  <dan@codesourcery.com>
+
+	* gdb.base/callfuncs.exp: New tests for $spval 0 and -1.  Remove return.
+
 2010-02-25  David S. Miller  <davem@davemloft.net>
 
 	* gdb.base/catch-syscall.exp: Allow to run on sparc*-*-linux and
--- src/gdb/testsuite/gdb.base/callfuncs.exp	2010/01/01 07:32:00	1.31
+++ src/gdb/testsuite/gdb.base/callfuncs.exp	2010/02/28 17:56:37	1.32
@@ -469,5 +469,17 @@
     fail "nested call dummies preserve register contents"
 }
 
-return 0
+# GDB should not crash by internal error on $sp underflow during the inferior
+# call.  It is OK it will stop on some: Cannot access memory at address 0x$hex.
 
+if {![target_info exists gdb,nosignals] && ![istarget "*-*-uclinux*"]} {
+    gdb_test {set $old_sp = $sp}
+
+    gdb_test {set $sp = 0}
+    gdb_test "call doubleit (1)" "" "sp == 0: call doubleit (1)"
+
+    gdb_test {set $sp = -1}
+    gdb_test "call doubleit (1)" "" "sp == -1: call doubleit (1)"
+
+    gdb_test {set $sp = $old_sp}
+}