From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14611 invoked by alias); 15 Feb 2017 15:54:11 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14485 invoked by uid 89); 15 Feb 2017 15:54:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=revisit, clearer, Determine, sk:termina X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 15 Feb 2017 15:54:06 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 900F2E9A40; Wed, 15 Feb 2017 15:54:06 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1FFs4Rp011792; Wed, 15 Feb 2017 10:54:05 -0500 Subject: Re: [PATCH v3 2/6] Share parts of gdb/terminal.h with gdbserver To: Sergio Durigan Junior , GDB Patches References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <20170208032257.15443-1-sergiodj@redhat.com> <20170208032257.15443-3-sergiodj@redhat.com> Cc: Luis Machado From: Pedro Alves Message-ID: Date: Wed, 15 Feb 2017 15:54:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170208032257.15443-3-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00417.txt.bz2 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