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

[Pedro Alves <palves at redhat dot com>]

On 02/08/2017 03:22 AM, 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;
> 


> diff --git a/gdb/common/common-terminal.h b/gdb/common/common-terminal.h
> new file mode 100644
> index 0000000..956bfcc
> --- /dev/null
> +++ b/gdb/common/common-terminal.h
> @@ -0,0 +1,125 @@


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

We try to keep common/ self-contained wrt autoconf -- i.e.,
corresponding autoconf checks should be added to common/common.m4.

> +
> +#if !defined (HAVE_TERMIOS) && !defined(HAVE_TERMIO) && !defined(HAVE_SGTTY)
> +#if defined(HAVE_TERMIOS_H)
> +#define HAVE_TERMIOS


> +/* 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 int job_control;
> +

...

> +/* Set the process group of the caller to its own pid, or do nothing
> +   if we lack job control.  */
> +extern int gdb_setpgid (void);
> +
> +/* Determine whether we have job control, and set variable JOB_CONTROL
> +   accordingly.  */
> +extern void have_job_control (void);

It had been clearer if these three were moved in the patch that
moves the corresponding .c code.  I was going to comment in that
patch that the "have_job_control" documentation should
mention that it must be called early, before "job_control"
is ever used.  And that that patch would have better been the one
that adds the have_job_control() calls to the appropriate
places.  With the split as is, this detail ends up easily missed.

> +
> +#endif /* ! COMMON_TERMINAL_H */
> diff --git a/gdb/gdbserver/terminal.c b/gdb/gdbserver/terminal.c
> new file mode 100644
> index 0000000..42ac651
> --- /dev/null
> +++ b/gdb/gdbserver/terminal.c
> @@ -0,0 +1,88 @@

> +/* See common/common-terminal.h.  */
> +
> +pid_t
> +create_tty_session (void)
> +{
> +  /* Placeholder needed by fork_inferior.  For now, this function is
> +     not needed nor useful to have on gdbserver.  When/If we properly
> +     handle terminal modes, we can revisit and implement the needed
> +     support.  */
> +  return (pid_t) 1;

Without looking at the caller, it would seem to me that -1 / 0 
would make more sense, and "1" kind of looks like a typo/bug.
Please add a comment mentioning why this returns 1.

> +}
> +
> +/* See common/common-inferior.h.  */
> +
> +void
> +set_inferior_io_terminal (const char *terminal_name)
> +{
> +  /* Placeholder needed by fork_inferior.  For now, this function is
> +     not needed nor useful to have on gdbserver.  When/If we properly
> +     handle terminal modes, we can revisit and implement the needed
> +     support.  */
> +}
> +
> +/* See common/common-inferior.h.  */
> +
> +const char *
> +get_inferior_io_terminal (void)
> +{
> +  /* Placeholder needed by fork_inferior.  For now, this function is
> +     not needed nor useful to have on gdbserver.  When/If we properly
> +     handle terminal modes, we can revisit and implement the needed
> +     support.  */

How about instead of repeating the same comment multiple times,
we add it once at the top:

  /* Placeholders needed by fork_inferior.  For now, these functions are
     not needed nor useful to have on gdbserver.  When/If we properly
     handle terminal modes, we can revisit and implement the needed
     support.  */

?

Thanks,
Pedro Alves