Re: [RFA] Fix GDB's handling of the inferior controlling terminal

Re: [RFA] Fix GDB's handling of the inferior controlling terminal

[Nick Roberts]

> The attached patch makes GDB set the inferior's controlling terminal,
> fixing both of the symptoms above. If the user specifies a different
> inferior terminal, GDB will now create a new session right after forking
> to run the inferior, and use the TIOCSCTTY ioctl to set the controlling
> terminal.

I like this change but under what conditions can GDB set the inferior's
controlling terminal?  I've created a pty without a controlling process in
Emacs and specified it for the inferior terminal but, using your patch, I still
get the message:

warning: GDB: Failed to set controlling terminal: Operation not permitted

Do I need to run GDB as root?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Fix GDB's handling of the inferior controlling terminal

[Andreas Schwab]

Nick Roberts <nickrob@snap.net.nz> writes:

> I like this change but under what conditions can GDB set the inferior's
> controlling terminal?

To set the controlling terminal you must be a session leader and don't
have a controlling terminal yet, and the terminal must not be the
controlling terminal of another session.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] Fix GDB's handling of the inferior controlling terminal

[Nick Roberts]

 > > I like this change but under what conditions can GDB set the inferior's
 > > controlling terminal?
 > 
 > To set the controlling terminal you must be a session leader and don't
 > have a controlling terminal yet, and the terminal must not be the
 > controlling terminal of another session.

OK, thanks.  I see now that I didn't apply the patch properly and it works
for me now.  I, for one, would like to this this change approved.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Fix GDB's handling of the inferior controlling terminal.

[Thiago Jung Bauermann]

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

On Wed, 2008-03-12 at 08:39 -0400, Daniel Jacobowitz wrote:
> On Mon, Mar 10, 2008 at 02:41:56PM -0300, Thiago Jung Bauermann wrote:
> This is OK, thanks.
> 
> > +  ret = setsid();
> 
> Space there before paren.

Thanks! Committed with the change. Attached is the final patch.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

[-- Attachment #2: controlling-tty-fix.diff --]
[-- Type: text/x-patch, Size: 4951 bytes --]

2008-03-10  Thiago Jung Bauermann  <bauerman.ibm.com>

	* configure.ac (AC_CHECK_FUNCS): Add check for setsid.
	* config.in, configure: Regenerate.
	* fork-child.c (fork_inferior): Call create_tty_session.
	* inflow.c (new_tty): Set controlling terminal with TIOCSCTTY.
	(create_tty_session): New function.
	* terminal.h: Declare create_tty_session.

=== modified file 'gdb/config.in'
Index: gdb/config.in
===================================================================
--- gdb/config.in.orig	2008-01-08 17:36:25.000000000 -0200
+++ gdb/config.in	2008-03-12 11:05:33.000000000 -0300
@@ -278,6 +278,9 @@
 /* Define to 1 if you have the `setpgrp' function. */
 #undef HAVE_SETPGRP
 
+/* Define to 1 if you have the `setsid' function. */
+#undef HAVE_SETSID
+
 /* Define to 1 if you have the <sgtty.h> header file. */
 #undef HAVE_SGTTY_H
 
Index: gdb/configure
===================================================================
--- gdb/configure.orig	2008-03-12 11:04:38.000000000 -0300
+++ gdb/configure	2008-03-12 11:05:34.000000000 -0300
@@ -18503,7 +18503,8 @@ done
 
 
 
-for ac_func in setpgid setpgrp
+
+for ac_func in setpgid setpgrp setsid
 do
 as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh`
 echo "$as_me:$LINENO: checking for $ac_func" >&5
Index: gdb/configure.ac
===================================================================
--- gdb/configure.ac.orig	2008-03-12 11:04:38.000000000 -0300
+++ gdb/configure.ac	2008-03-12 11:05:34.000000000 -0300
@@ -603,7 +603,7 @@ AC_CHECK_FUNCS(getuid getgid)
 AC_CHECK_FUNCS(poll)
 AC_CHECK_FUNCS(pread64)
 AC_CHECK_FUNCS(sbrk)
-AC_CHECK_FUNCS(setpgid setpgrp)
+AC_CHECK_FUNCS(setpgid setpgrp setsid)
 AC_CHECK_FUNCS(sigaction sigprocmask sigsetmask)
 AC_CHECK_FUNCS(socketpair)
 AC_CHECK_FUNCS(syscall)
Index: gdb/fork-child.c
===================================================================
--- gdb/fork-child.c.orig	2008-03-12 11:04:38.000000000 -0300
+++ gdb/fork-child.c	2008-03-12 11:05:34.000000000 -0300
@@ -303,10 +303,16 @@ fork_inferior (char *exec_file_arg, char
       if (debug_fork)
 	sleep (debug_fork);
 
-      /* Run inferior in a separate process group.  */
-      debug_setpgrp = gdb_setpgid ();
-      if (debug_setpgrp == -1)
-	perror ("setpgrp failed in child");
+      /* Create a new session for the inferior process, if necessary.
+         It will also place the inferior in a separate process group.  */
+      if (create_tty_session () <= 0)
+	{
+	  /* No session was created, but we still want to run the inferior
+	     in a separate process group.  */
+	  debug_setpgrp = gdb_setpgid ();
+	  if (debug_setpgrp == -1)
+	    perror ("setpgrp failed in child");
+	}
 
       /* Ask the tty subsystem to switch to the one we specified
          earlier (or to share the current terminal, if none was
Index: gdb/inflow.c
===================================================================
--- gdb/inflow.c.orig	2008-01-03 19:27:04.000000000 -0200
+++ gdb/inflow.c	2008-03-12 11:08:55.000000000 -0300
@@ -557,6 +557,16 @@ new_tty (void)
       close (2);
       dup (tty);
     }
+
+#ifdef TIOCSCTTY
+  /* Make tty our new controlling terminal.  */
+  if (ioctl (tty, TIOCSCTTY, 0) == -1)
+    /* Mention GDB in warning because it will appear in the inferior's
+       terminal instead of GDB's.  */
+    warning ("GDB: Failed to set controlling terminal: %s",
+	     safe_strerror (errno));
+#endif
+
   if (tty > 2)
     close (tty);
 #endif /* !go32 && !win32 */
@@ -683,6 +693,33 @@ clear_sigio_trap (void)
 #endif /* No SIGIO.  */
 \f
 
+/* Create a new session if the inferior will run in a different tty.
+   A session is UNIX's way of grouping processes that share a controlling
+   terminal, so a new one is needed if the inferior terminal will be
+   different from GDB's.
+
+   Returns the session id of the new session, 0 if no session was created
+   or -1 if an error occurred.  */
+pid_t
+create_tty_session (void)
+{
+#ifdef HAVE_SETSID
+  pid_t ret;
+
+  if (!job_control || inferior_thisrun_terminal == 0)
+    return 0;
+
+  ret = setsid ();
+  if (ret == -1)
+    warning ("Failed to create new terminal session: setsid: %s",
+	     safe_strerror (errno));
+
+  return ret;
+#else
+  return 0;
+#endif /* HAVE_SETSID */
+}
+
 /* This is here because this is where we figure out whether we (probably)
    have job control.  Just using job_control only does part of it because
    setpgid or setpgrp might not exist on a system without job control.
Index: gdb/terminal.h
===================================================================
--- gdb/terminal.h.orig	2008-01-03 19:27:07.000000000 -0200
+++ gdb/terminal.h	2008-03-12 11:05:34.000000000 -0300
@@ -82,6 +82,8 @@ extern void new_tty (void);
    a given run of GDB.  In inflow.c.  */
 extern int job_control;
 
+extern int create_tty_session (void);
+
 /* Set the process group of the caller to its own pid, or do nothing if
    we lack job control.  */
 extern int gdb_setpgid (void);

Re: [RFA] Fix GDB's handling of the inferior controlling terminal.

[Daniel Jacobowitz]

On Mon, Mar 10, 2008 at 02:41:56PM -0300, Thiago Jung Bauermann wrote:
> Hi,
> 
> With the current CVS version, if the user specifies a different terminal
> for the inferior to run on, GDB doesn't set it as the inferior's
> controlling terminal.
> 
> This leads to some problems. For instance, the inferior will not
> get SIGINT sent to the terminal. Also, if the inferior is GDB itself,
> the following message is shown in the inferior GDB's debugging session:
> 
> [tcsetpgrp failed in terminal_inferior: Inappropriate ioctl for device]
> 
> The attached patch makes GDB set the inferior's controlling terminal,
> fixing both of the symptoms above. If the user specifies a different
> inferior terminal, GDB will now create a new session right after forking
> to run the inferior, and use the TIOCSCTTY ioctl to set the controlling
> terminal.
> 
> Ran testsuite, no regressions found. Is this ok?

This is OK, thanks.

> +  ret = setsid();

Space there before paren.

-- 
Daniel Jacobowitz
CodeSourcery

[RFA] Fix GDB's handling of the inferior controlling terminal.

[Thiago Jung Bauermann]

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

Hi,

With the current CVS version, if the user specifies a different terminal
for the inferior to run on, GDB doesn't set it as the inferior's
controlling terminal.

This leads to some problems. For instance, the inferior will not
get SIGINT sent to the terminal. Also, if the inferior is GDB itself,
the following message is shown in the inferior GDB's debugging session:

[tcsetpgrp failed in terminal_inferior: Inappropriate ioctl for device]

The attached patch makes GDB set the inferior's controlling terminal,
fixing both of the symptoms above. If the user specifies a different
inferior terminal, GDB will now create a new session right after forking
to run the inferior, and use the TIOCSCTTY ioctl to set the controlling
terminal.

Ran testsuite, no regressions found. Is this ok?
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

[-- Attachment #2: controlling-tty-fix.diff --]
[-- Type: text/x-patch, Size: 4445 bytes --]

2008-03-10  Thiago Jung Bauermann  <bauerman.ibm.com>

	* configure.ac (AC_CHECK_FUNCS): Add check for setsid.
	* config.in, configure: Regenerate.
	* fork-child.c (fork_inferior): Call create_tty_session.
	* inflow.c (new_tty): Set controlling terminal with TIOCSCTTY.
	(create_tty_session): New function.
	* terminal.h: Declare create_tty_session.

=== modified file 'gdb/config.in'
--- gdb/config.in	2008-01-21 02:31:01 +0000
+++ gdb/config.in	2008-01-21 02:51:22 +0000
@@ -278,6 +278,9 @@
 /* Define to 1 if you have the `setpgrp' function. */
 #undef HAVE_SETPGRP
 
+/* Define to 1 if you have the `setsid' function. */
+#undef HAVE_SETSID
+
 /* Define to 1 if you have the <sgtty.h> header file. */
 #undef HAVE_SGTTY_H
 

=== modified file 'gdb/configure'
--- gdb/configure	2008-01-21 02:31:01 +0000
+++ gdb/configure	2008-01-21 02:51:22 +0000
@@ -18503,7 +18503,8 @@ done
 
 
 
-for ac_func in setpgid setpgrp
+
+for ac_func in setpgid setpgrp setsid
 do
 as_ac_var=`echo "ac_cv_func_$ac_func" | $as_tr_sh`
 echo "$as_me:$LINENO: checking for $ac_func" >&5

=== modified file 'gdb/configure.ac'
--- gdb/configure.ac	2008-01-21 02:31:02 +0000
+++ gdb/configure.ac	2008-01-21 02:51:22 +0000
@@ -603,7 +603,7 @@ AC_CHECK_FUNCS(getuid getgid)
 AC_CHECK_FUNCS(poll)
 AC_CHECK_FUNCS(pread64)
 AC_CHECK_FUNCS(sbrk)
-AC_CHECK_FUNCS(setpgid setpgrp)
+AC_CHECK_FUNCS(setpgid setpgrp setsid)
 AC_CHECK_FUNCS(sigaction sigprocmask sigsetmask)
 AC_CHECK_FUNCS(socketpair)
 AC_CHECK_FUNCS(syscall)

=== modified file 'gdb/fork-child.c'
--- gdb/fork-child.c	2008-03-01 21:54:26 +0000
+++ gdb/fork-child.c	2008-03-01 22:11:53 +0000
@@ -289,10 +289,16 @@ fork_inferior (char *exec_file_arg, char
       if (debug_fork)
 	sleep (debug_fork);
 
-      /* Run inferior in a separate process group.  */
-      debug_setpgrp = gdb_setpgid ();
-      if (debug_setpgrp == -1)
-	perror ("setpgrp failed in child");
+      /* Create a new session for the inferior process, if necessary.
+         It will also place the inferior in a separate process group.  */
+      if (create_tty_session () <= 0)
+	{
+	  /* No session was created, but we still want to run the inferior
+	     in a separate process group.  */
+	  debug_setpgrp = gdb_setpgid ();
+	  if (debug_setpgrp == -1)
+	    perror ("setpgrp failed in child");
+	}
 
       /* Ask the tty subsystem to switch to the one we specified
          earlier (or to share the current terminal, if none was

=== modified file 'gdb/inflow.c'
--- gdb/inflow.c	2008-01-01 22:53:11 +0000
+++ gdb/inflow.c	2008-01-21 02:51:38 +0000
@@ -557,6 +557,16 @@ new_tty (void)
       close (2);
       dup (tty);
     }
+
+#ifdef TIOCSCTTY
+  /* Make tty our new controlling terminal.  */
+  if (ioctl (tty, TIOCSCTTY, 0) == -1)
+    /* Mention GDB in warning because it will appear in the inferior's
+       terminal instead of GDB's.  */
+    warning ("GDB: Failed to set controlling terminal: %s",
+	     safe_strerror (errno));
+#endif
+
   if (tty > 2)
     close (tty);
 #endif /* !go32 && !win32 */
@@ -683,6 +693,33 @@ clear_sigio_trap (void)
 #endif /* No SIGIO.  */
 \f
 
+/* Create a new session if the inferior will run in a different tty.
+   A session is UNIX's way of grouping processes that share a controlling
+   terminal, so a new one is needed if the inferior terminal will be
+   different from GDB's.
+   
+   Returns the session id of the new session, 0 if no session was created
+   or -1 if an error occurred.  */
+pid_t
+create_tty_session (void)
+{
+#ifdef HAVE_SETSID
+  pid_t ret;
+
+  if (!job_control || inferior_thisrun_terminal == 0)
+    return 0;
+
+  ret = setsid();
+  if (ret == -1)
+    warning ("Failed to create new terminal session: setsid: %s",
+	     safe_strerror (errno));
+
+  return ret;
+#else
+  return 0;
+#endif /* HAVE_SETSID */
+}
+
 /* This is here because this is where we figure out whether we (probably)
    have job control.  Just using job_control only does part of it because
    setpgid or setpgrp might not exist on a system without job control.

=== modified file 'gdb/terminal.h'
--- gdb/terminal.h	2008-01-01 22:53:13 +0000
+++ gdb/terminal.h	2008-01-21 02:50:27 +0000
@@ -82,6 +82,8 @@ extern void new_tty (void);
    a given run of GDB.  In inflow.c.  */
 extern int job_control;
 
+extern int create_tty_session (void);
+
 /* Set the process group of the caller to its own pid, or do nothing if
    we lack job control.  */
 extern int gdb_setpgid (void);