Re: [PATCH 1/3] Assume termios is available, remove support for termio and sgtty

[Pedro Alves <palves at redhat dot com>]

On 11/02/2017 06:54 PM, Sergio Durigan Junior wrote:
> On Thursday, November 02 2017, Pedro Alves wrote:
> 
>> This commit garbage collects the termio and sgtty support.
>>
>> GDB's terminal handling code still has support for the old termio and
>> sgtty interfaces in addition to termios.  However, I think it's pretty
>> safe to assume that for a long, long time, Unix-like systems provide
>> termios.  GNU/Linux, Solaris, Cygwin, AIX, DJGPP, macOS and the BSDs
>> all have had termios.h for many years.  Looking around the web, I
>> found discussions about FreeBSD folks trying to get rid of old sgtty.h
>> a decade ago:
>>
>>   https://lists.freebsd.org/pipermail/freebsd-hackers/2007-March/019983.html
>>
>> So I think support for termio and sgtty in GDB is just dead code that
>> is never compiled anywhere and is just getting in the way.  For
>> example, serial_noflush_set_tty_state and the raw<->cooked concerns
>> mentioned in inflow.c only exist because of sgtty (see
>> hardwire_noflush_set_tty_state).
>>
>> Regtested on GNU/Linux.
>>
>> Confirmed that I can still build Solaris, DJGPP and AIX GDB and that
>> that GDB still includes the termios.h-guarded code.  Confirmed
> 
> "that that"

Eh, that was actually on purpose.  It's "I confirmed that $something",
with "$something == that GDB (the one I built) still includes".
The first 'that' is the subordinating that, and the second 'that' is 
a demonstrative pronoun.  It's not unlike:
"I think that this thing is blue, and
 I think that that other thing is white".

> 
>> mingw-w64 GDB still builds and skips the termios.h-guarded code.
> 
> Thanks for doing that.
> 
> You may remember that I also stumbled upon this while doing the
> startup-with-shell, but I didn't have the necessary background knowledge
> to do a deep cleanup.  It's always nice to see old code being removed.

Well, it was only after banging my head against the terminal
handling for a couple weeks that I realized that this is
all dead code.  :-P

>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -729,12 +729,9 @@ XMLFILES = \
>>  	$(srcdir)/features/traceframe-info.dtd \
>>  	$(srcdir)/features/xinclude.dtd
>>  
>> -# This is ser-unix.o for any system which supports a v7/BSD/SYSV/POSIX
>> -# interface to the serial port.  Hopefully if get ported to OS/2, VMS,
>> -# etc., then there will be (as part of the C library or perhaps as
>> -# part of libiberty) a POSIX interface.  But at least for now the
>> -# host-dependent makefile fragment might need to use something else
>> -# besides ser-unix.o
>> +# This is ser-unix.o for any system which supports a POSIX interface
>> +# to the serial port.  The host-dependent makefile fragment might need
>> +# to use something else besides ser-unix.o.
>>  SER_HARDWIRE = @SER_HARDWIRE@
>>  
>>  # The `remote' debugging target is supported for most architectures,
> 
> You also need to remove common/gdb_termios.h from HFILES_NO_SRCDIR here.

Good point.  Consider it done.

>> -#if defined (HAVE_TERMIOS) || defined (TIOCGPGRP)
>>  #ifdef HAVE_SETPGID
>>        /* The call setpgid (0, 0) is supposed to work and mean the same
>>           thing as this, but on Ultrix 4.2A it fails with EPERM (and
>> @@ -56,7 +58,6 @@ gdb_setpgid ()
>>  #endif
>>  #endif /* HAVE_SETPGRP */
>>  #endif /* HAVE_SETPGID */
>> -#endif /* defined (HAVE_TERMIOS) || defined (TIOCGPGRP) */
> 
> Did you decide to remove the check for HAVE_TERMIOS (instead of
> replacing it with an "#ifdef HAVE_TERMIOS_H") because we're already
> checking for HAVE_SETPGID?

Yes.

>> --- a/gdb/gdbserver/remote-utils.c
>> +++ b/gdb/gdbserver/remote-utils.c
>> @@ -17,7 +17,9 @@
>>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>>  
>>  #include "server.h"
>> -#include "gdb_termios.h"
>> +#if HAVE_TERMIOS_H
>> +#include <termios.h>
>> +#endif
>>  #include "target.h"
>>  #include "gdbthread.h"
>>  #include "tdesc.h"
>> @@ -325,7 +327,7 @@ remote_open (const char *name)
>>        if (remote_desc < 0)
>>  	perror_with_name ("Could not open remote device");
>>  
>> -#ifdef HAVE_TERMIOS
>> +#if HAVE_TERMIOS_H
> 
> Why s/#ifdef/#if/?  I prefer #ifdef BTW.

Yeah, I used #if here because it's what the file is already
using for the other headers:

#if HAVE_SYS_IOCTL_H
#include <sys/ioctl.h>
#endif
#if HAVE_SYS_FILE_H
#include <sys/file.h>
#endif

etc.

> As far as I could, I reviewed all the changes, and they seem OK to me
> modulo the few nits I pointed.

Thanks for the review!

-- 
Pedro Alves