RFC: Do not call write_pc for "signal SIGINT"

RFC: Do not call write_pc for "signal SIGINT"

[Daniel Jacobowitz]

This FIXME has been there since the dawn of CVS.  Removing it causes
no testsuite regressions on x86_64-linux; I don't think it will on
other platforms either.

Any comments on this patch?  Otherwise I'll check it in after a little
while.

This isn't the only place where Linux's internal errno codes can leak
back into user programs because of how we fiddle orig_eax.  I'll file
another bug report about that.

-- 
Daniel Jacobowitz
CodeSourcery

2008-08-28  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/2241
	* infcmd.c (signal_command): Do not specify a resume PC.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.200
diff -u -p -r1.200 infcmd.c
--- infcmd.c	21 Aug 2008 20:13:08 -0000	1.200
+++ infcmd.c	28 Aug 2008 15:51:15 -0000
@@ -1123,11 +1123,7 @@ signal_command (char *signum_exp, int fr
     }
 
   clear_proceed_status ();
-  /* "signal 0" should not get stuck if we are stopped at a breakpoint.
-     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 ((CORE_ADDR) -1, oursig, 0);
 }
 
 /* Proceed until we reach a different source line with pc greater than

Re: RFC: Do not call write_pc for "signal SIGINT"

[Michael Snyder]

Daniel Jacobowitz wrote:
> This FIXME has been there since the dawn of CVS.  Removing it causes
> no testsuite regressions on x86_64-linux; I don't think it will on
> other platforms either.
> 
> Any comments on this patch?  Otherwise I'll check it in after a little
> while.

Hmmm, kind of opaque.  Your new code seems like the right
thing to do, but I don't understand the code that you're replacing.

Isn't -1 supposed to mean the same as stop_pc?
And isn't signal 0 equivalent to no signal?


> This isn't the only place where Linux's internal errno codes can leak
> back into user programs because of how we fiddle orig_eax.  I'll file
> another bug report about that.

Now you've really lost me.  What have errno codes
got to do with this?

> 2008-08-28  Daniel Jacobowitz  <dan@codesourcery.com>
> 
>         PR gdb/2241
>         * infcmd.c (signal_command): Do not specify a resume PC.
> 
> Index: infcmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infcmd.c,v
> retrieving revision 1.200
> diff -u -p -r1.200 infcmd.c
> --- infcmd.c    21 Aug 2008 20:13:08 -0000      1.200
> +++ infcmd.c    28 Aug 2008 15:51:15 -0000
> @@ -1123,11 +1123,7 @@ signal_command (char *signum_exp, int fr
>      }
> 
>    clear_proceed_status ();
> -  /* "signal 0" should not get stuck if we are stopped at a breakpoint.
> -     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 ((CORE_ADDR) -1, oursig, 0);
>  }
> 
>  /* Proceed until we reach a different source line with pc greater than

Re: RFC: Do not call write_pc for "signal SIGINT"

[Daniel Jacobowitz]

On Thu, Aug 28, 2008 at 11:09:56AM -0700, Michael Snyder wrote:
> Hmmm, kind of opaque.  Your new code seems like the right
> thing to do, but I don't understand the code that you're replacing.
>
> Isn't -1 supposed to mean the same as stop_pc?

Except that we call gdbarch_write_pc to replace the PC in this case,
and that has side effects - take a look at a Linux example.

> And isn't signal 0 equivalent to no signal?

Yes, that's right.

>> This isn't the only place where Linux's internal errno codes can leak
>> back into user programs because of how we fiddle orig_eax.  I'll file
>> another bug report about that.
>
> Now you've really lost me.  What have errno codes
> got to do with this?

Sorry, take a look at the PR (gdb/2241) for more information.  When
you hit C-c during select, and then type "signal SIGINT", the program
gets errno 514 instead of EINTR.

Yes, I know, I need a testcase...

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Do not call write_pc for "signal SIGINT"

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Thu, Aug 28, 2008 at 11:09:56AM -0700, Michael Snyder wrote:
>> Hmmm, kind of opaque.  Your new code seems like the right
>> thing to do, but I don't understand the code that you're replacing.
>>
>> Isn't -1 supposed to mean the same as stop_pc?
> 
> Except that we call gdbarch_write_pc to replace the PC in this case,
> and that has side effects - take a look at a Linux example.
> 
>> And isn't signal 0 equivalent to no signal?
> 
> Yes, that's right.
> 
>>> This isn't the only place where Linux's internal errno codes can leak
>>> back into user programs because of how we fiddle orig_eax.  I'll file
>>> another bug report about that.
>> Now you've really lost me.  What have errno codes
>> got to do with this?
> 
> Sorry, take a look at the PR (gdb/2241) for more information.  When
> you hit C-c during select, and then type "signal SIGINT", the program
> gets errno 514 instead of EINTR.
> 
> Yes, I know, I need a testcase...

More than a testcase.  This isn't very well explained.
You didn't reference the PR in your original post, and
I'm still not sure how we're getting from SIGINT to
signal 0...

Re: RFC: Do not call write_pc for "signal SIGINT"

[Daniel Jacobowitz]

On Thu, Aug 28, 2008 at 11:34:37AM -0700, Michael Snyder wrote:
> More than a testcase.  This isn't very well explained.
> You didn't reference the PR in your original post, and

It was in the ChangeLog entry, sorry.

> I'm still not sure how we're getting from SIGINT to
> signal 0...

We aren't.  The comment that I'm deleting is talking about the user
typing "signal 0", which worked correctly; I'm making "signal SIGINT"
treat the PC the same way.  All the complexity is in incorrect bits
I'm deleting :-)  Afterwards it's like every other call to proceed in
the file.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Do not call write_pc for "signal SIGINT"

[Daniel Jacobowitz]

On Thu, Aug 28, 2008 at 06:32:32PM -0400, Daniel Jacobowitz wrote:
> On Thu, Aug 28, 2008 at 11:34:37AM -0700, Michael Snyder wrote:
> > More than a testcase.  This isn't very well explained.
> > You didn't reference the PR in your original post, and
> 
> It was in the ChangeLog entry, sorry.

Here it is with a testcase.

To recap: there is a tricky bug in signal_command.  If any non-zero
signal is specified, it performs a jump to the current address instead
of just resuming there.  This causes any pending system call to be
interrupted, in a way that leaves a kernel-internal value in the
return value register.  If we just delete that code, and the FIXME
that goes with it, the right thing happens: instead of "Unknown
error 514", the system call returns EINTR and the loop continues.

Michael, is that clearer?

-- 
Daniel Jacobowitz
CodeSourcery

2008-11-17  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/2241
	* infcmd.c (signal_command): Do not specify a resume PC.

2008-11-17  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/2241
	* gdb.base/interrupt.c (sigint_handler): New.
	(main): Install a SIGINT handler if SIGNALS is defined.  Exit
	on error.
	* gdb.base/interrupt.exp: Define SIGNALS unless gdb,nosignals.
	Test "signal SIGINT".

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.222
diff -u -p -r1.222 infcmd.c
--- infcmd.c	5 Nov 2008 20:23:07 -0000	1.222
+++ infcmd.c	17 Nov 2008 21:27:58 -0000
@@ -1147,11 +1147,7 @@ signal_command (char *signum_exp, int fr
     }
 
   clear_proceed_status ();
-  /* "signal 0" should not get stuck if we are stopped at a breakpoint.
-     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 ((CORE_ADDR) -1, oursig, 0);
 }
 
 /* Proceed until we reach a different source line with pc greater than
Index: testsuite/gdb.base/interrupt.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/interrupt.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 interrupt.c
--- testsuite/gdb.base/interrupt.c	28 Jun 1999 16:03:16 -0000	1.1.1.2
+++ testsuite/gdb.base/interrupt.c	17 Nov 2008 21:29:38 -0000
@@ -2,6 +2,16 @@
 #include <stdio.h>
 #include <unistd.h>
 #include <stdlib.h>
+
+#ifdef SIGNALS
+#include <signal.h>
+
+static void
+sigint_handler (int signo)
+{
+}
+#endif
+
 int
 main ()
 {
@@ -11,6 +21,9 @@ main ()
   set_debug_traps();
   breakpoint();
 #endif
+#ifdef SIGNALS
+  signal (SIGINT, sigint_handler);
+#endif
   printf ("talk to me baby\n");
   while (1)
     {
@@ -20,7 +33,10 @@ main ()
 #ifdef EINTR
 	  if (errno != EINTR)
 #endif
-	    perror ("");
+	    {
+	      perror ("");
+	      return 1;
+	    }
 	}
       else if (nbytes == 0)
 	{
Index: testsuite/gdb.base/interrupt.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/interrupt.exp,v
retrieving revision 1.12
diff -u -p -r1.12 interrupt.exp
--- testsuite/gdb.base/interrupt.exp	6 Aug 2008 12:52:07 -0000	1.12
+++ testsuite/gdb.base/interrupt.exp	17 Nov 2008 21:29:38 -0000
@@ -34,7 +34,13 @@ set bug_id 0
 set testfile interrupt
 set srcfile ${testfile}.c
 set binfile ${objdir}/${subdir}/${testfile}
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+
+set options { debug }
+if { ! [target_info exists gdb,nosignals] } {
+    lappend options "additional_flags=-DSIGNALS"
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $options] != "" } {
      untested interrupt.exp
      return -1
 }
@@ -165,7 +171,35 @@ if ![file exists $binfile] then {
 	    eof { fail "echo data (eof)" }
 	}
 
-	setup_xfail "i*86-pc-linux*-gnu*"
+ 	if { ! [target_info exists gdb,nosignals] } {
+	    # Wait until the program is in the read system call again.
+	    sleep 2
+
+	    # Stop the program for another test.
+	    set msg "Send Control-C, second time"
+	    send_gdb "\003"
+	    gdb_test_multiple "" "$msg" {
+		-re "Program received signal SIGINT.*$gdb_prompt $" {
+		    pass "$msg"
+		}
+	    }
+
+	    # The "signal" command should deliver the correctg signal and
+	    # return to the loop.
+	    set msg "signal SIGINT"
+	    gdb_test_multiple "signal SIGINT" "$msg" {
+		-re "^signal SIGINT\r\nContinuing with signal SIGINT.\r\n(\r\n|)$" { pass "$msg" }
+	    }
+
+	    # We should be back in the loop.
+	    send_gdb "more data\n"
+	    gdb_expect {
+		-re "^(\r\n|)more data\r\n(|more data\r\n)$" { pass "echo more data" }
+		timeout { fail "echo more data (timeout)" }
+		eof { fail "echo more data (eof)" }
+	    }
+	}
+
 	send_gdb "\004"
 	gdb_expect {
 	    -re "end of file.*Program exited normally.*$gdb_prompt $" {

Re: RFC: Do not call write_pc for "signal SIGINT"

[Pedro Alves]

On Monday 17 November 2008 21:55:01, Daniel Jacobowitz wrote:

> To recap: there is a tricky bug in signal_command.  If any non-zero
> signal is specified, it performs a jump to the current address instead
> of just resuming there.  This causes any pending system call to be
> interrupted, in a way that leaves a kernel-internal value in the
> return value register.  If we just delete that code, and the FIXME
> that goes with it, the right thing happens: instead of "Unknown
> error 514", the system call returns EINTR and 
> the loop continues. 

This may help explain it better:

/* Set the program counter for process PTID to PC.  */

static void
i386_linux_write_pc (struct regcache *regcache, CORE_ADDR pc)
{
  regcache_cooked_write_unsigned (regcache, I386_EIP_REGNUM, pc);

  /* We must be careful with modifying the program counter.  If we
     just interrupted a system call, the kernel might try to restart
     it when we resume the inferior.  On restarting the system call,
     the kernel will try backing up the program counter even though it
     no longer points at the system call.  This typically results in a
     SIGSEGV or SIGILL.  We can prevent this by writing `-1' in the
     "orig_eax" pseudo-register.

     Note that "orig_eax" is saved when setting up a dummy call frame.
     This means that it is properly restored when that frame is
     popped, and that the interrupted system call will be restarted
     when we resume the inferior on return from a function call from
     within GDB.  In all other cases the system call will not be
     restarted.  */
  regcache_cooked_write_unsigned (regcache, I386_LINUX_ORIG_EAX_REGNUM, -1);
}

-- 
Pedro Alves

Re: RFC: Do not call write_pc for "signal SIGINT"

[Daniel Jacobowitz]

On Mon, Nov 17, 2008 at 04:55:01PM -0500, Daniel Jacobowitz wrote:
> Here it is with a testcase.
> 
> To recap: there is a tricky bug in signal_command.  If any non-zero
> signal is specified, it performs a jump to the current address instead
> of just resuming there.  This causes any pending system call to be
> interrupted, in a way that leaves a kernel-internal value in the
> return value register.  If we just delete that code, and the FIXME
> that goes with it, the right thing happens: instead of "Unknown
> error 514", the system call returns EINTR and the loop continues.

> 2008-11-17  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	PR gdb/2241
> 	* infcmd.c (signal_command): Do not specify a resume PC.
> 
> 2008-11-17  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	PR gdb/2241
> 	* gdb.base/interrupt.c (sigint_handler): New.
> 	(main): Install a SIGINT handler if SIGNALS is defined.  Exit
> 	on error.
> 	* gdb.base/interrupt.exp: Define SIGNALS unless gdb,nosignals.
> 	Test "signal SIGINT".

I have checked this in.

-- 
Daniel Jacobowitz
CodeSourcery