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

Sergio Durigan Junior
2017-01-03T21:14:46Z

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