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

Pedro Alves
2017-11-02T19:27:26Z

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