Patchwork Upgrading readline

login
register
mail settings
Submitter Tom Tromey
Date Sept. 29, 2018, 11:52 a.m.
Message ID <87efdcikqi.fsf@tromey.com>
Download mbox | patch
Permalink /patch/29582/
State New
Headers show

Comments

Tom Tromey - Sept. 29, 2018, 11:52 a.m.
I took a look at what it would take to upgrade to the latest readline 7
patch release.  I have a few questions, mostly about Windows-related
preprocessor defines.

First I diff'd readline 6.2 patch 1 against gdb's readline.
From the history it appears that this is the correct baseline to use.

Then I diff'd readline 6.2 patch 1 against readline 7.0 patch 5.
This is the latest stable release of readline.

Next, for each hunk in the readline-to-gdb diff, I searched for an
equivalent hunk in the readline-to-readline diff.  If the hunk was
found, I removed it.

This left a patch consisting of changes that were made to the gdb
readline but not upstreamed.  They fall into four categories:

* Changes that seem necessary or at least desirable for gdb.  In the
  patch below this includes the configure.in, hsuser.texi, and
  Makefile.in changes.

* Changes that should be retested.  This includes the, emacs_keymap.c,
  and complete.c changes.

* Changes that were simply not upstream.  In general I don't know why.
  This covers the signals.c and xfree.c changes, though perhaps
  signals.c falls into the "re-test" category.  I think for xfree.c I
  can maybe do a bit of patch archaeology to find the reason.

* Changes that are upstream but in a different form.  display.c and
  terminal.c have different #if conditions and input.c has different
  code:

  display.c

  readline upstream:
  +#ifdef __DJGPP__
  
  gdb:
  +#ifdef __MSDOS__
  or
  +#if defined (__GO32__)

  terminal.c
  readline
  +#ifdef __MSDOS__

  gdb:
  +#ifdef __DJGPP__

  input.c, readline:
    +int
    +win32_isatty (int fd)
    +{
    +  return (_isatty (fd) ? ((((long) (HANDLE) _get_osfhandle (fd)) & 3) == 3) : 0);
    +}

  (for gdb see the patch, the gdb version is longer and calls GetConsoleMode)


For most of the differences, I know what to do.  But, I don't know the
difference between __DJGPP__, __MSDOS__, and __GO32__ -- and the sources
also check __MINGW32__, _WIN32, and __CYGWIN__ at places.  So, I am not
sure how to go about merging those parts, and whether changes need to be
sent upstream.

My other question is how best to go about merging.  In the CVS days I
would have done a cvs import on a vendor branch.  I could simulate this
pretty well in an external git repository I suppose.  It's tempting to
switch entirely to git submodules for external dependencies, but maybe
just having a gdb "import area" git repository would be alright.  The
idea being, do the merge work there, but in the end just check in a
giant patch.  Then the next person to do this could re-use the
repository.


I'm attaching my edited readline->gdb patch so you can see the details
if you want.  The readline 6->7 diff is very big but you can see it by
cloning:

git://git.savannah.gnu.org/readline.git

and diffing like

git diff d0a9f21f36b2bf3ac28e1b11c1962bbb2ca0be2c..origin/master

That revision is, I believe, readline 6.2 patch 1.
Maybe this can also be seen online somewhere, I don't know.

Tom
Eli Zaretskii - Sept. 29, 2018, 1:53 p.m.
> From: Tom Tromey <tom@tromey.com>
> Date: Sat, 29 Sep 2018 05:52:37 -0600
> 
>   input.c, readline:
>     +int
>     +win32_isatty (int fd)
>     +{
>     +  return (_isatty (fd) ? ((((long) (HANDLE) _get_osfhandle (fd)) & 3) == 3) : 0);
>     +}

The Readline version reportedly doesn't work with newer versions of
Windows, so I think we should ask upstream Readline to use the newer
code we have in GDB.

> I don't know the difference between __DJGPP__, __MSDOS__, and
> __GO32__

They are all equivalent, so I suggest to change them all to use
__DJGPP__, unless there's some reason not to.

> the sources also check __MINGW32__, _WIN32, and __CYGWIN__ at
> places.

These are unrelated to DOS/DJGPP/GO32, they are for MinGW builds and
Cygwin builds.

Thanks for doing this.
Pedro Alves - Sept. 29, 2018, 6:19 p.m.
On 09/29/2018 12:52 PM, Tom Tromey wrote:
> I took a look at what it would take to upgrade to the latest readline 7
> patch release.  I have a few questions, mostly about Windows-related
> preprocessor defines.

Thanks for doing this.  Note that Patrick Palka attempted this
a couple of years back.  The update went into master, but
was subsequently reverted because of problems on non-x86 ports.

See:
 https://sourceware.org/ml/gdb-patches/2015-07/msg00422.html
 https://sourceware.org/ml/gdb-patches/2015-07/msg00759.html

And:
 https://sourceware.org/ml/gdb-patches/2016-02/msg00484.html

Thanks,
Pedro Alves
Tom Tromey - Sept. 30, 2018, 8:30 p.m.
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> The Readline version reportedly doesn't work with newer versions of
Eli> Windows, so I think we should ask upstream Readline to use the newer
Eli> code we have in GDB.

I sent a note to bug-readline with the patch.

>> I don't know the difference between __DJGPP__, __MSDOS__, and
>> __GO32__

Eli> They are all equivalent, so I suggest to change them all to use
Eli> __DJGPP__, unless there's some reason not to.

>> the sources also check __MINGW32__, _WIN32, and __CYGWIN__ at
>> places.

Eli> These are unrelated to DOS/DJGPP/GO32, they are for MinGW builds and
Eli> Cygwin builds.

When I did the merge, these changes disappeared, so maybe I was
misreading the diffs somehow.  Or maybe I did the merge wrong, hard to
say.

What I did is make a new git repository and made a branch based on
readline 6.2 patch 1 -- the gdb base revision.  Then I copied the
readline sources from the gdb repository on top of that and committed
it.  Finally, I did a git merge from readline 7.0 patch 5 and fixed up
the conflicts.

You can see the resulting repository here:

    https://github.com/tromey/gdb-readline/tree/gdb-readline

Future merges can be done this way pretty easily, either by just
recreating this process, or by cloning the repository and merging from
the latest readline.

readline 8 is in testing, so perhaps it would be good to try that to see
if there are any new readline bugs affecting gdb.

Tom
Tom Tromey - Oct. 18, 2018, 10:11 p.m.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 09/29/2018 12:52 PM, Tom Tromey wrote:
>> I took a look at what it would take to upgrade to the latest readline 7
>> patch release.  I have a few questions, mostly about Windows-related
>> preprocessor defines.

Pedro> Thanks for doing this.  Note that Patrick Palka attempted this
Pedro> a couple of years back.  The update went into master, but
Pedro> was subsequently reverted because of problems on non-x86 ports.

Pedro> See:
Pedro>  https://sourceware.org/ml/gdb-patches/2015-07/msg00422.html
Pedro>  https://sourceware.org/ml/gdb-patches/2015-07/msg00759.html

Pedro> And:
Pedro>  https://sourceware.org/ml/gdb-patches/2016-02/msg00484.html

Yeah, I saw those - he was trying with 7.0 alpha, and I was hoping maybe
doing it with a later (non-alpha) version would help.

I have a patch.  And, I went through the local changes and I think none
of them are needed, so I've removed them one-by-one in the resulting
series.

However, it's too big to push through "buildbot try".
So, it may be quite some time until I'm able to test it suitably.
If someone wants to help with this, it's in my github repository.

Tom

Patch

diff -u -r ./complete.c /home/tromey/gdb/binutils-gdb/readline/complete.c
--- ./complete.c	2018-09-28 14:41:49.586486451 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/complete.c	2018-09-22 10:37:48.370424749 -0600
@@ -481,6 +482,10 @@ 
 {
   int c;
 
+/* Disabled for GDB due to the gdb.base/readline-ask.exp regression.
+   [patch] testsuite: Test readline-6.2 "ask" regression
+   http://sourceware.org/ml/gdb-patches/2011-05/msg00002.html  */
+#if 0
   /* For now, disable pager in callback mode, until we later convert to state
      driven functions.  Have to wait until next major version to add new
      state definition, since it will change value of RL_STATE_DONE. */
@@ -488,6 +493,7 @@ 
   if (RL_ISSTATE (RL_STATE_CALLBACK))
     return 1;
 #endif
+#endif
 
   for (;;)
     {
diff -u -r ./configure.in /home/tromey/gdb/binutils-gdb/readline/configure.in
--- ./configure.in	2018-09-28 14:41:49.589486475 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/configure.in	2018-09-22 10:37:48.371424756 -0600
@@ -22,19 +22,24 @@ 
 
 AC_REVISION([for Readline 6.2, version 2.67])
 
+m4_include([../config/override.m4])
+
 AC_INIT(readline, 6.2, bug-readline@gnu.org)
 
 dnl make sure we are using a recent autoconf version
 AC_PREREQ(2.50)
 
 AC_CONFIG_SRCDIR(readline.h)
-AC_CONFIG_AUX_DIR(./support)
+dnl GDB LOCAL
+dnl AC_CONFIG_AUX_DIR(./support)
+AC_CONFIG_AUX_DIR(`cd $srcdir;pwd`/..)
 AC_CONFIG_HEADERS(config.h)
 
 dnl update the value of RL_READLINE_VERSION in readline.h when this changes
 LIBVERSION=6.2
 
 AC_CANONICAL_HOST
+AC_CANONICAL_BUILD
 
 dnl configure defaults
 opt_curses=no
@@ -57,10 +62,10 @@ 
 dnl option parsing for optional features
 opt_multibyte=yes
 opt_static_libs=yes
-opt_shared_libs=yes
+opt_shared_libs=no
 
 AC_ARG_ENABLE(multibyte, AC_HELP_STRING([--enable-multibyte], [enable multibyte characters if OS supports them]), opt_multibyte=$enableval)
-AC_ARG_ENABLE(shared, AC_HELP_STRING([--enable-shared], [build shared libraries [[default=YES]]]), opt_shared_libs=$enableval)
+dnl AC_ARG_ENABLE(shared, AC_HELP_STRING([--enable-shared], [build shared libraries [[default=YES]]]), opt_shared_libs=$enableval)
 AC_ARG_ENABLE(static, AC_HELP_STRING([--enable-static], [build static libraries [[default=YES]]]), opt_static_libs=$enableval)
 
 if test $opt_multibyte = no; then
@@ -193,6 +198,9 @@ 
 		TERMCAP_LIB=-ltermcap	#default
 	fi
 fi
+if test "$TERMCAP_LIB" = "-lncurses"; then
+	AC_CHECK_HEADERS(ncurses/termcap.h)
+fi
 
 BASH_CHECK_MULTIBYTE
 
@@ -264,7 +272,7 @@ 
 AC_SUBST(STATIC_INSTALL_TARGET)
 AC_SUBST(SHARED_INSTALL_TARGET)
 
-case "$host_os" in
+case "$build_os" in
 msdosdjgpp*)	BUILD_DIR=`pwd.exe` ;;	# to prevent //d/path/file
 *)		BUILD_DIR=`pwd` ;;
 esac
diff -u -r ./display.c /home/tromey/gdb/binutils-gdb/readline/display.c
--- ./display.c	2018-09-28 14:41:49.591486491 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/display.c	2018-09-22 10:37:48.372424763 -0600
@@ -2056,9 +2060,18 @@ 
     }
   else
     {			/* delta < 0 */
+#ifdef __MSDOS__
+      int row, col;
+
+      fflush (rl_outstream); /* make sure the cursor pos is current! */
+      ScreenGetCursor (&row, &col);
+      ScreenSetCursor (row + delta, col);
+      i = -delta;    /* in case someone wants to use it after the loop */
+#else /* !__MSDOS__ */
       if (_rl_term_up && *_rl_term_up)
 	for (i = 0; i < -delta; i++)
 	  tputs (_rl_term_up, 1, _rl_output_character_function);
+#endif /* !__MSDOS__ */
     }
 
   _rl_last_v_pos = to;		/* Now TO is here */
@@ -2341,10 +2357,15 @@ 
 void
 _rl_clear_screen ()
 {
+#if defined (__GO32__)
+  ScreenClear ();	/* FIXME: only works in text modes */
+  ScreenSetCursor (0, 0);  /* term_clrpag is "cl" which homes the cursor */
+#else
   if (_rl_term_clrpag)
     tputs (_rl_term_clrpag, 1, _rl_output_character_function);
   else
     rl_crlf ();
+#endif
 }
 
 /* Insert COUNT characters from STRING to the output stream at column COL. */
@@ -2353,7 +2374,7 @@ 
      char *string;
      int count, col;
 {
-#if defined (__MSDOS__) || defined (__MINGW32__)
+#if defined (__MSDOS__) || (defined (__MINGW32__) && !defined (NCURSES_VERSION))
   _rl_output_some_chars (string, count);
 #else
   /* DEBUGGING */
@@ -2405,7 +2426,7 @@ 
   if (count > _rl_screenwidth)	/* XXX */
     return;
 
-#if !defined (__MSDOS__) && !defined (__MINGW32__)
+#if !defined (__MSDOS__) && !(defined (__MINGW32__) && !defined (NCURSES_VERSION))
   if (_rl_term_DC && *_rl_term_DC)
     {
       char *buffer;
diff -u -r ./doc/hsuser.texi /home/tromey/gdb/binutils-gdb/readline/doc/hsuser.texi
--- ./doc/hsuser.texi	2018-09-28 14:41:49.601486570 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/doc/hsuser.texi	2018-09-22 10:37:48.373424771 -0600
@@ -26,9 +26,10 @@ 
 @node Using History Interactively
 @chapter Using History Interactively
 
-@ifclear BashFeatures
-@defcodeindex bt
-@end ifclear
+@c GDB bundling modification:
+@c @ifclear BashFeatures
+@c @defcodeindex bt
+@c @end ifclear
 
 @ifset BashFeatures
 This chapter describes how to use the @sc{gnu} History Library
@@ -41,7 +42,8 @@ 
 This chapter describes how to use the @sc{gnu} History Library interactively,
 from a user's standpoint.  It should be considered a user's guide.  For
 information on using the @sc{gnu} History Library in your own programs,
-@pxref{Programming with GNU History}.
+@c GDB bundling modification:
+@pxref{Programming with GNU History, , , history, GNU History Library}.
 @end ifclear
 
 @ifset BashFeatures
diff -u -r ./emacs_keymap.c /home/tromey/gdb/binutils-gdb/readline/emacs_keymap.c
--- ./emacs_keymap.c	2018-09-28 14:39:37.548445474 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/emacs_keymap.c	2018-09-22 10:37:48.377424800 -0600
@@ -277,7 +277,13 @@ 
   { ISFUNC, rl_insert },	/* Latin capital letter Y with acute */
   { ISFUNC, rl_insert },	/* Latin capital letter thorn (Icelandic) */
   { ISFUNC, rl_insert },	/* Latin small letter sharp s (German) */
+#ifndef __MINGW32__
   { ISFUNC, rl_insert },	/* Latin small letter a with grave */
+#else
+  /* Temporary - this is a bug in readline 5.1 that should be fixed in
+     readline 5.2.  */
+  { ISFUNC, 0 },		/* Must leave this unbound for the arrow keys to work.  */
+#endif
   { ISFUNC, rl_insert },	/* Latin small letter a with acute */
   { ISFUNC, rl_insert },	/* Latin small letter a with circumflex */
   { ISFUNC, rl_insert },	/* Latin small letter a with tilde */
diff -u -r ./input.c /home/tromey/gdb/binutils-gdb/readline/input.c
--- ./input.c	2018-09-28 14:41:49.648486940 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/input.c	2018-09-22 10:37:48.382424836 -0600
@@ -86,6 +86,37 @@ 
 static int rl_get_char PARAMS((int *));
 static int rl_gather_tyi PARAMS((void));
 
+#if defined (_WIN32) && !defined (__CYGWIN__)
+
+/* 'isatty' in the Windows runtime returns non-zero for every
+   character device, including the null device.  Repair that.  */
+#include <io.h>
+#include <conio.h>
+#define WIN32_LEAN_AND_MEAN 1
+#include <windows.h>
+
+int w32_isatty (int fd)
+{
+  if (_isatty(fd))
+    {
+      HANDLE h = (HANDLE) _get_osfhandle (fd);
+      DWORD ignored;
+
+      if (h == INVALID_HANDLE_VALUE)
+	{
+	  errno = EBADF;
+	  return 0;
+	}
+      if (GetConsoleMode (h, &ignored) != 0)
+	return 1;
+    }
+  errno = ENOTTY;
+  return 0;
+}
+
+#define isatty(x)  w32_isatty(x)
+#endif
+
 /* **************************************************************** */
 /*								    */
 /*			Character Input Buffering       	    */
diff -u -r ./Makefile.in /home/tromey/gdb/binutils-gdb/readline/Makefile.in
--- ./Makefile.in	2018-09-28 14:41:49.580486404 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/Makefile.in	2018-09-22 10:37:48.368424734 -0600
@@ -211,7 +211,17 @@ 
 
 force:
 
-install:	$(INSTALL_TARGETS)
+## GDB LOCAL
+## Don't mess with people's installed readline's.
+## This tries to install this version of readline over whatever
+## version is already installed on the system (which could be a
+## newer version). There is no real reason for us to install
+## readline along with GDB. GDB links statically against readline,
+## so it doesn't depend on us installing it on the system.
+
+install:
+
+#install:	$(INSTALL_TARGETS)
 
 install-headers: installdirs ${INSTALLED_HEADERS}
 	for f in ${INSTALLED_HEADERS}; do \
@@ -412,7 +422,7 @@ 
 vi_mode.o: readline.h keymaps.h rltypedefs.h chardefs.h tilde.h
 vi_mode.o: history.h ansi_stdlib.h rlstdc.h
 xfree.o: ${BUILD_DIR}/config.h
-xfree.o: ansi_stdlib.h
+xfree.o: ansi_stdlib.h readline.h
 xmalloc.o: ${BUILD_DIR}/config.h
 xmalloc.o: ansi_stdlib.h
 
diff -u -r ./signals.c /home/tromey/gdb/binutils-gdb/readline/signals.c
--- ./signals.c	2018-09-28 14:41:49.653486980 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/signals.c	2018-09-22 10:37:48.386424865 -0600
@@ -580,6 +580,7 @@ 
   sigint_blocked = 0;
 }
 
+#ifdef SIGWINCH
 /* Cause SIGWINCH to not be delivered until the corresponding call to
    release_sigwinch(). */
 void
@@ -627,6 +628,7 @@ 
 
   sigwinch_blocked = 0;
 }
+#endif /* SIGWINCH */
 
 /* **************************************************************** */
 /*								    */
diff -u -r ./terminal.c /home/tromey/gdb/binutils-gdb/readline/terminal.c
--- ./terminal.c	2018-09-28 14:41:49.654486987 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/terminal.c	2018-09-22 10:37:48.387424873 -0600
@@ -661,12 +694,17 @@ 
 	default:
 	  break;
 	case VISIBLE_BELL:
+#ifdef __MSDOS__
+	  ScreenVisualBell ();
+	  break;
+#else
 	  if (_rl_visible_bell)
 	    {
 	      tputs (_rl_visible_bell, 1, _rl_output_character_function);
 	      break;
 	    }
 	  /* FALLTHROUGH */
+#endif
 	case AUDIBLE_BELL:
 	  fprintf (stderr, "\007");
 	  fflush (stderr);
diff -u -r ./xfree.c /home/tromey/gdb/binutils-gdb/readline/xfree.c
--- ./xfree.c	2018-09-28 14:39:37.556445540 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/xfree.c	2018-09-22 10:37:48.389424887 -0600
@@ -31,7 +31,10 @@ 
 #  include "ansi_stdlib.h"
 #endif /* HAVE_STDLIB_H */
 
+#include <stdio.h>
+
 #include "xmalloc.h"
+#include "readline.h"
 
 /* **************************************************************** */
 /*								    */
@@ -45,6 +48,10 @@ 
 xfree (string)
      PTR_T string;
 {
+  /* Leak a bit.  */
+  if (RL_ISSTATE(RL_STATE_SIGHANDLER))
+    return;
+
   if (string)
     free (string);
 }
diff -u -r ./xmalloc.h /home/tromey/gdb/binutils-gdb/readline/xmalloc.h
--- ./xmalloc.h	2018-08-29 22:22:13.425457063 -0600
+++ /home/tromey/gdb/binutils-gdb/readline/xmalloc.h	2018-09-22 10:37:48.389424887 -0600
@@ -38,6 +38,9 @@ 
 
 #endif /* !PTR_T */
 
+/* xmalloc and xrealloc should be also protected from RL_STATE_SIGHANDLER.  */
+#define xfree xfree_readline
+
 extern PTR_T xmalloc PARAMS((size_t));
 extern PTR_T xrealloc PARAMS((void *, size_t));
 extern void xfree PARAMS((void *));