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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hey Luis, thanks for the review.

Comments below.

On Monday, December 26 2016, Luis Machado wrote:

> 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

Thanks, will fix.

>> 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.

I actually created a new file and moved the necessary things by hand.  I
think git marked this as a copy because I have a special setting on my
$HOME/.gitconfig telling it to detect copies on diff's.  But if you look
below, you'll see that gdb/terminal.h is still there.

Anyway, I can send a different diff if you want.

>> 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...

Right, I'll fix these comments.

>> +   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?

Not really.  Even though this is a cosmetic change and could be removed
from the patch, I decided to leave it there.  Our coding standards
explain this:

  https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Blank_Line_After_Block_Comment_Preceding_Variable_Definitions

>>  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?

They're not exactly on my radar, but they're a part of the local/remote
feature parity, so they will be tackled soon, I'd figure.

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

I agree.

> 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?

I'll try to give these a try and implementing them.  My only concern is
that I don't want these to explode into a giant new task to implement
inferior I/O on gdbserver, but it may be possible to just touch the
necessary bits and make it simple.

>> +
>> +/* 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.

They were moved, and the reason why they didn't show up is because of
the git option that I mentioned before.  I'll make sure to list these
changes in the next patch iteration.

>> +#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.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/