This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 2/6] Share parts of gdb/terminal.h with gdbserver
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Cc: Luis Machado <lgustavo at codesourcery dot com>
- Date: Wed, 15 Feb 2017 15:54:03 +0000
- Subject: Re: [PATCH v3 2/6] Share parts of gdb/terminal.h with gdbserver
- Authentication-results: sourceware.org; auth=none
- References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170208032257.15443-1-sergiodj@redhat.com> <20170208032257.15443-3-sergiodj@redhat.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