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

[Luis Machado <lgustavo@codesourcery.com>]

On 01/03/2017 03:14 PM, Sergio Durigan Junior wrote:
> 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.
>

No need. It just looked slightly off. But i got it once i noticed that 
git handled the copy on its own.

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

Ok. Thanks for clarifying this. GDB's codebase seems to be inconsistent 
here. Some places add a newline while others don't.

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

The rule is that the patch sender automatically volunteers for 
additional bits of work. :-P

Honestly, if it gets too complicated, then it should be fine to have the 
placeholders. But then it would be nice to add some more interesting 
comments on how these ought to be implemented in the future, along with 
bits on how these should be synched with what gdb already supports.

Just an idea.