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

Pedro Alves
2017-02-15T15:54:11Z

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