Re: [PATCH 2/6] Share parts of gdb/terminal.h with gdbserver

[Luis Machado <lgustavo at codesourcery dot com>]

On 12/22/2016 09:39 PM, Sergio Durigan Junior wrote:
> As part of the bigger work of sharing fork_inferior with gdbserver,
> some parts of gdb/terminal.h also needed to be moved to a common
> place.  These parts are:
>
> - The code responsible for determining some terminal-based define's
>   based on available features;
>
> - job control;
>
> - terminal-related functions needed by fork_inferior;
>
> gdb/ChangeLog:
> 2016-12-22  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* Makefile.in (HFILES_NO_SRCDIR): Add "common/common-terminal.h".
> 	* common/common-terminal.h: New file, with parts of "terminal.h".
> 	* terminal.h: Move terminal-related defines to
> 	"common/common-terminal.h".
> 	(new_tty_prefork): Move to "common/common-terminal.h".
> 	(new_tty): Likewise.
> 	(new_tty_postfork): Likewise.
> 	(job_control): Likewise.
> 	(create_tty_session): Likewise.
> 	(gdb_setpgid): Likewise.
> 	* utils.c: Include "terminal.h".
>
> gdb/ChangeLog:

gdb/gdbserver/ChangeLog

> 2016-12-22  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* terminal.c: New file.
> ---
>  gdb/Makefile.in                              |  1 +
>  gdb/{terminal.h => common/common-terminal.h} | 55 +++++++++++++--------
>  gdb/gdbserver/terminal.c                     | 72 +++++++++++++++++++++++++++
>  gdb/terminal.h                               | 73 +---------------------------
>  gdb/utils.c                                  |  1 +
>  5 files changed, 109 insertions(+), 93 deletions(-)
>  copy gdb/{terminal.h => common/common-terminal.h} (66%)
>  create mode 100644 gdb/gdbserver/terminal.c
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 051f07d..51c0f73 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1469,6 +1469,7 @@ HFILES_NO_SRCDIR = \
>  	common/common-regcache.h \
>  	common/common-types.h \
>  	common/common-utils.h \
> +	common/common-terminal.h \
>  	common/errors.h \
>  	common/environ.h \
>  	common/fileio.h \
> diff --git a/gdb/terminal.h b/gdb/common/common-terminal.h
> similarity index 66%
> copy from gdb/terminal.h
> copy to gdb/common/common-terminal.h

Creating a new common/common-terminal.[h|c] may be cleaner than 
git-copying and then modifying it. With a new file we get all the 
included changes, whereas with git-copying we get a number of unrelated 
changes that need to be removed.


> index 0deced4..0ec8a23 100644
> --- a/gdb/terminal.h
> +++ b/gdb/common/common-terminal.h
> @@ -16,9 +16,8 @@
>     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 !defined (TERMINAL_H)
> -#define TERMINAL_H 1
> -
> +#ifndef COMMON_TERMINAL_H
> +#define COMMON_TERMINAL_H
>
>  /* If we're using autoconf, it will define HAVE_TERMIOS_H,
>     HAVE_TERMIO_H and HAVE_SGTTY_H for us.  One day we can rewrite
> @@ -76,37 +75,51 @@
>  #endif /* sgtty */
>  #endif
>
> -struct inferior;
> +#include <sys/types.h>
> +
> +/* Do we have job control?  Can be assumed to always be the same
> +   within a given run of GDB.  Use in gdb/inflow.c and
> +   common/common-inflow.c.  */
>
> -extern void new_tty_prefork (const char *);
> +extern int job_control;
>
>  extern void new_tty (void);
>
> -extern void new_tty_postfork (void);
> +/* NEW_TTY_PREFORK is called before forking a new child process,
> +   so we can record the state of ttys in the child to be formed.
> +   TTYNAME is null if we are to share the terminal with gdb;

This mentions gdb, but it may be used for gdbserver as well? We may need 
generic wording here to account for that fact. This happens in other 
places as well...

> +   or points to a string containing the name of the desired tty.
>
> -extern void copy_terminal_info (struct inferior *to, struct inferior *from);
> +   NEW_TTY is called in new child processes under Unix, which will
> +   become debugger target processes.  This actually switches to
> +   the terminal specified in the NEW_TTY_PREFORK call.  */
>
> -/* Do we have job control?  Can be assumed to always be the same within
> -   a given run of GDB.  In inflow.c.  */
> -extern int job_control;
> +extern void new_tty_prefork (const char *ttyname);
>
> -extern pid_t create_tty_session (void);
> +/* NEW_TTY_POSTFORK is called after forking a new child process, and
> +   adding it to the inferior table, to store the TTYNAME being used by
> +   the child, or null if it sharing the terminal with gdb.  */
>
> -/* Set the process group of the caller to its own pid, or do nothing if
> -   we lack job control.  */
> -extern int gdb_setpgid (void);
> +extern void new_tty_postfork (void);
>
> -/* Set up a serial structure describing standard input.  In inflow.c.  */
> -extern void initialize_stdin_serial (void);
> +/* 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.

... like here.

> -extern void gdb_save_tty_state (void);
> +   Returns the session id of the new session, 0 if no session was created
> +   or -1 if an error occurred.  */
>
> -/* Take a snapshot of our initial tty state before readline/ncurses
> -   have had a chance to alter it.  */
> -extern void set_initial_gdb_ttystate (void);
> +extern pid_t create_tty_session (void);
>
>  /* Set the process group of the caller to its own pid, or do nothing
>     if we lack job control.  */
> +

Spurious newline?

>  extern int gdb_setpgid (void);
>
> -#endif /* !defined (TERMINAL_H) */
> +/* Determine whether we have job control, and set variable JOB_CONTROL
> +   accordingly.  */
> +
> +extern void have_job_control (void);
> +
> +#endif /* ! COMMON_TERMINAL_H */
> diff --git a/gdb/gdbserver/terminal.c b/gdb/gdbserver/terminal.c
> new file mode 100644
> index 0000000..9813018
> --- /dev/null
> +++ b/gdb/gdbserver/terminal.c
> @@ -0,0 +1,72 @@
> +/* Terminal interface definitions for the GDB remote server.
> +   Copyright (C) 2016 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 "server.h"
> +#include "common-terminal.h"
> +
> +/* See common/common-terminal.h.  */
> +
> +void
> +new_tty (void)
> +{
> +  /* To be implemented.  */
> +}
> +
> +/* See common/common-terminal.h.  */
> +
> +void
> +new_tty_prefork (const char *ttyname)
> +{
> +  /* To be implemented.  */
> +}
> +
> +/* See common/common-terminal.h.  */
> +
> +void
> +new_tty_postfork (void)
> +{
> +  /* To be implemented.  */
> +}

Are these going to be implemented at some point or is this something 
that may not be addressed until much later on?

It wouldn't be great to have a number of these lying around with no 
clear plan to have them addressed.

Are these counterparts of what gdb always has? Does it make sense to 
unify those functions instead of adding placeholders for a potentially 
different implementation?

> +
> +/* See common/common-terminal.h.  */
> +
> +pid_t
> +create_tty_session (void)
> +{
> +  /* For now we just return a dummy value.  This function will be
> +     properly implemented later.  */
> +  return (pid_t) 1;
> +}
> +
> +/* See common/common-inferior.h.  */
> +
> +void
> +set_inferior_io_terminal (const char *terminal_name)
> +{
> +  /* To be implemented.  */
> +}
> +
> +/* See common/common-inferior.h.  */
> +
> +const char *
> +get_inferior_io_terminal (void)
> +{
> +  /* Return a dummy value.  This function will be properly implemented
> +     later.  */
> +  return NULL;
> +}
> diff --git a/gdb/terminal.h b/gdb/terminal.h
> index 0deced4..810b597 100644
> --- a/gdb/terminal.h
> +++ b/gdb/terminal.h
> @@ -19,83 +19,12 @@
>  #if !defined (TERMINAL_H)
>  #define TERMINAL_H 1
>
> -
> -/* If we're using autoconf, it will define HAVE_TERMIOS_H,
> -   HAVE_TERMIO_H and HAVE_SGTTY_H for us.  One day we can rewrite
> -   ser-unix.c and inflow.c to inspect those names instead of
> -   HAVE_TERMIOS, HAVE_TERMIO and the implicit HAVE_SGTTY (when neither
> -   HAVE_TERMIOS or HAVE_TERMIO is set).  Until then, make sure that
> -   nothing has already defined the one of the names, and do the right
> -   thing.  */
> -
> -#if !defined (HAVE_TERMIOS) && !defined(HAVE_TERMIO) && !defined(HAVE_SGTTY)
> -#if defined(HAVE_TERMIOS_H)
> -#define HAVE_TERMIOS
> -#else /* ! defined (HAVE_TERMIOS_H) */
> -#if defined(HAVE_TERMIO_H)
> -#define HAVE_TERMIO
> -#else /* ! defined (HAVE_TERMIO_H) */
> -#if defined(HAVE_SGTTY_H)
> -#define HAVE_SGTTY
> -#endif /* ! defined (HAVE_SGTTY_H) */
> -#endif /* ! defined (HAVE_TERMIO_H) */
> -#endif /* ! defined (HAVE_TERMIOS_H) */
> -#endif /* !defined (HAVE_TERMIOS) && !defined (HAVE_TERMIO) &&
> -	  !defined (HAVE_SGTTY) */
> -
> -#if defined(HAVE_TERMIOS)
> -#include <termios.h>
> -#endif
> -
> -#if !defined(_WIN32) && !defined (HAVE_TERMIOS)
> -
> -/* Define a common set of macros -- BSD based -- and redefine whatever
> -   the system offers to make it look like that.  FIXME: serial.h and
> -   ser-*.c deal with this in a much cleaner fashion; as soon as stuff
> -   is converted to use them, can get rid of this crap.  */
> -
> -#ifdef HAVE_TERMIO
> -
> -#include <termio.h>
> -
> -#undef TIOCGETP
> -#define TIOCGETP TCGETA
> -#undef TIOCSETN
> -#define TIOCSETN TCSETA
> -#undef TIOCSETP
> -#define TIOCSETP TCSETAF
> -#define TERMINAL struct termio
> -
> -#else /* sgtty */
> -
> -#include <fcntl.h>
> -#include <sgtty.h>
> -#include <sys/ioctl.h>
> -#define TERMINAL struct sgttyb
> -
> -#endif /* sgtty */
> -#endif

Did the above defines get moved to common/common-terminal.h or did they 
vanish? The git-copying doesn't make that too obvious. I suppose they 
are still in common/common-terminal.h, they just did not get changed, 
and thus we get no mention of this in the patch.

> +#include "common-terminal.h"
>
>  struct inferior;
>
> -extern void new_tty_prefork (const char *);
> -
> -extern void new_tty (void);
> -
> -extern void new_tty_postfork (void);
> -
>  extern void copy_terminal_info (struct inferior *to, struct inferior *from);
>
> -/* Do we have job control?  Can be assumed to always be the same within
> -   a given run of GDB.  In inflow.c.  */
> -extern int job_control;
> -
> -extern pid_t 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);
> -
>  /* Set up a serial structure describing standard input.  In inflow.c.  */
>  extern void initialize_stdin_serial (void);
>
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 3a88e2a..bc1dfe5 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -53,6 +53,7 @@
>  #include "top.h"
>  #include "main.h"
>  #include "solist.h"
> +#include "terminal.h"
>
>  #include "inferior.h"		/* for signed_pointer_to_address */

Otherwise looks OK to me.