[PATCH 7/8] Remove readline hack from gdb_select

[PATCH 7/8] Remove readline hack from gdb_select

[Tom Tromey]

As discussed on gdb-patches, this removes the readline hack from the
mingw-hdep.c version of gdb_select.  It's believed that this is not
needed any more.

2019-08-04  Tom Tromey  <tom@tromey.com>

	* mingw-hdep.c (gdb_select): Remove readline hack.
---
 gdb/ChangeLog    | 4 ++++
 gdb/mingw-hdep.c | 9 ---------
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/gdb/mingw-hdep.c b/gdb/mingw-hdep.c
index 8ed4b44ddce..44fb22e9a16 100644
--- a/gdb/mingw-hdep.c
+++ b/gdb/mingw-hdep.c
@@ -23,7 +23,6 @@
 #include "event-loop.h"
 
 #include "gdb_select.h"
-#include "readline/readline.h"
 
 #include <windows.h>
 
@@ -167,14 +166,6 @@ gdb_select (int n, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	}
     }
 
-  /* With multi-threaded SIGINT handling, there is a race between the
-     readline signal handler and GDB.  It may still be in
-     rl_prep_terminal in another thread.  Do not return until it is
-     done; we can check the state here because we never longjmp from
-     signal handlers on Windows.  */
-  while (RL_ISSTATE (RL_STATE_SIGHANDLER))
-    Sleep (1);
-
   return num_ready;
 }
 
-- 
2.17.2

[PATCH 0/8] Upgrade readline

[Tom Tromey]

Here's the series to upgrade readline.  This has been sitting around
for a while and I thought I would finally send it.

I built it on a Windows machine at AdaCore, but although readline is
built and gdb links against it, I can't get it to work.  Maybe this is
due to whatever remote terminal I am using -- I don't know.

Tested on x86-64 Fedora 28. Also, note that the major distros use
--with-system-readline, which I think is further evidence that this
works.

This was too big to test on the buildbot.

Let me know what you think.

Tom

[PATCH 8/8] Require readline 7 or newer

[Tom Tromey]

This changes gdb to require readline 7 or newer at build time.

2019-04-21  Tom Tromey  <tom@tromey.com>

	* README: Update.
	* event-top.c: Require readline 7.

gdb/doc/ChangeLog
2019-04-21  Tom Tromey  <tom@tromey.com>

	* gdb.texinfo (Configure Options): Document minimum version of
	readline.
---
 gdb/ChangeLog       | 5 +++++
 gdb/README          | 3 ++-
 gdb/doc/ChangeLog   | 5 +++++
 gdb/doc/gdb.texinfo | 3 ++-
 gdb/event-top.c     | 3 +++
 5 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/gdb/README b/gdb/README
index 8a91aab2a4c..8883a8a09e3 100644
--- a/gdb/README
+++ b/gdb/README
@@ -439,7 +439,8 @@ more obscure GDB `configure' options are not listed here.
 
 `--with-system-readline'
      Use the readline library installed on the host, rather than the
-     library supplied as part of GDB.
+     library supplied as part of GDB.  Readline 7 or newer is required;
+     this is enforced by the build system.
 
 `--with-system-zlib
      Use the zlib library installed on the host, rather than the
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 89b1eda2c17..e384718fc11 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -36897,7 +36897,8 @@ details.
 
 @item --with-system-readline
 Use the readline library installed on the host, rather than the
-library supplied as part of @value{GDBN}.
+library supplied as part of @value{GDBN}.  Readline 7 or newer is
+required; this is enforced by the build system.
 
 @item --with-system-zlib
 Use the zlib library installed on the host, rather than the library
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 2132fb550dc..07cedc42584 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -48,6 +48,9 @@
 /* readline defines this.  */
 #undef savestring
 
+/* gdb requires readline 7 now.  */
+gdb_static_assert (RL_VERSION_MAJOR >= 7);
+
 static std::string top_level_prompt ();
 
 /* Signal handlers.  */
-- 
2.17.2

[PATCH 5/8] Fix gdb's selftest.exp after readline import

[Tom Tromey]

From: Patrick Palka <patrick@parcs.ath.cx>

After the sync there is one testsuite regression, the test
"signal SIGINT" in gdb.gdb/selftest.exp which now FAILs.  Previously,
the readline 6.2 SIGINT handler would temporarily reinstall the
underlying application's SIGINT handler and immediately re-raise SIGINT
so that the orginal handler gets invoked.  But now (since readline 6.3)
its SIGINT handler does not re-raise SIGINT or directly invoke the
original handler; it now sets a flag marking that SIGINT was raised, and
waits until readline explicitly has control to call the application's
SIGINT handler.  Anyway, because SIGINT is no longer re-raised from
within readline's SIGINT handler, doing "signal SIGINT" with a stopped
inferior gdb process will no longer resume and then immediately stop the
process (since there is no 2nd SIGINT to immediately catch).  Instead,
the inferior gdb process will now just print "Quit" and continue to run.
So with this commit, this particular test case is adjusted to reflect
this change in behavior (we now have to send a 2nd SIGINT manually to
stop it).

gdb/testsuite/ChangeLog
2015-07-25  Patrick Palka  <patrick@parcs.ath.cx>

	* gdb.gdb/selftest.exp (test_with_self): Update test to now
	expect the GDB inferior to no longer immediately stop after
	being resumed with "signal SIGINT".
---
 gdb/testsuite/ChangeLog            |  6 ++++++
 gdb/testsuite/gdb.gdb/selftest.exp | 23 ++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.gdb/selftest.exp b/gdb/testsuite/gdb.gdb/selftest.exp
index 7fdb3b226e1..9651561fafe 100644
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -110,9 +110,26 @@ proc test_with_self { } {
     }
     
     set description "send SIGINT signal to child process"
-    gdb_test "signal SIGINT" \
-	"Continuing with signal SIGINT.*" \
-	"$description"
+    gdb_test_multiple "signal SIGINT" "$description" {
+	-re "^signal SIGINT\r\nContinuing with signal SIGINT.\r\nQuit\r\n.* $" {
+	    pass "$description"
+	}
+    }
+
+    set description "send ^C to child process again"
+    send_gdb "\003"
+    gdb_expect {
+	-re "Program received signal SIGINT.*$gdb_prompt $" {
+	    pass "$description"
+	}
+	-re ".*$gdb_prompt $" {
+	    fail "$description"
+	}
+	timeout {
+	    fail "$description (timeout)"
+	}
+    }
+
 
     # Switch back to the GDB thread if Guile support is linked in.
     # "signal SIGINT" could also switch the current thread.
-- 
2.17.2

[PATCH 2/8] Remove gdb workaround from readline/complete.c

[Tom Tromey]

This removes a gdb-local patch from readline's get_y_or_n.  The code
references a gdb test that continues to work when I remove this patch.
So, I think it is not needed any more.

2018-10-07  Tom Tromey  <tom@tromey.com>

	* complete.c (get_y_or_n): Remove gdb workaround.
---
 readline/ChangeLog.gdb | 4 ++++
 readline/complete.c    | 5 -----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/readline/complete.c b/readline/complete.c
index ac54d76a255..0a81129b877 100644
--- a/readline/complete.c
+++ b/readline/complete.c
@@ -528,17 +528,12 @@ get_y_or_n (for_pager)
 {
   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. */
 #if defined (READLINE_CALLBACKS)
   if (RL_ISSTATE (RL_STATE_CALLBACK))
     return 1;
-#endif
 #endif
 
   for (;;)
-- 
2.17.2

[PATCH 3/8] Remove gdb workaround from readline/emacs_keymap.c

[Tom Tromey]

There is a gdb-local patch in readline/emacs_keymap.c that says:

  /* Temporary - this is a bug in readline 5.1 that should be fixed in
     readline 5.2.  */

So, I think this can be removed now.  I have no way to test this, as
the patch was specific to mingw.

2018-10-07  Tom Tromey  <tom@tromey.com>

	* emacs_keymap.c: Remove gdb workaround.
---
 readline/ChangeLog.gdb  | 4 ++++
 readline/emacs_keymap.c | 6 ------
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/readline/emacs_keymap.c b/readline/emacs_keymap.c
index 9f816581efa..cb6e140a217 100644
--- a/readline/emacs_keymap.c
+++ b/readline/emacs_keymap.c
@@ -277,13 +277,7 @@ KEYMAP_ENTRY_ARRAY emacs_standard_keymap = {
   { 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 */
-- 
2.17.2

[PATCH 4/8] Remove gdb workaround from readline/xfree.c

[Tom Tromey]

There is a gdb-local patch to deal with interrupts during completion.
The original thread adding this patch is here:

https://sourceware.org/ml/gdb-patches/2011-06/msg00147.html

I believe readline now implements the approach suggested by
Chet Ramey:

https://sourceware.org/ml/gdb-patches/2011-06/msg00493.html

So, I believe this patch can be removed.

2018-10-07  Tom Tromey  <tom@tromey.com>

	* Makefile.in (xfree.o): Don't depend on readline.h.
	* xfree.c (xfree): Remove gdb workaround.
	* xmalloc.h (xfree): Remove #define.
---
 readline/ChangeLog.gdb | 6 ++++++
 readline/Makefile.in   | 2 +-
 readline/xfree.c       | 7 -------
 readline/xmalloc.h     | 3 ---
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/readline/Makefile.in b/readline/Makefile.in
index 0916d33e065..1adfc286b81 100644
--- a/readline/Makefile.in
+++ b/readline/Makefile.in
@@ -437,7 +437,7 @@ vi_mode.o: rldefs.h ${BUILD_DIR}/config.h rlconf.h
 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 readline.h
+xfree.o: ansi_stdlib.h
 xmalloc.o: ${BUILD_DIR}/config.h
 xmalloc.o: ansi_stdlib.h
 
diff --git a/readline/xfree.c b/readline/xfree.c
index d3af7d9aef0..37a81e6c236 100644
--- a/readline/xfree.c
+++ b/readline/xfree.c
@@ -31,10 +31,7 @@
 #  include "ansi_stdlib.h"
 #endif /* HAVE_STDLIB_H */
 
-#include <stdio.h>
-
 #include "xmalloc.h"
-#include "readline.h"
 
 /* **************************************************************** */
 /*								    */
@@ -48,10 +45,6 @@ void
 xfree (string)
      PTR_T string;
 {
-  /* Leak a bit.  */
-  if (RL_ISSTATE(RL_STATE_SIGHANDLER))
-    return;
-
   if (string)
     free (string);
 }
diff --git a/readline/xmalloc.h b/readline/xmalloc.h
index 0fb6a1960e1..f40d7a596a2 100644
--- a/readline/xmalloc.h
+++ b/readline/xmalloc.h
@@ -38,9 +38,6 @@
 
 #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 *));
-- 
2.17.2

Re: [PATCH 8/8] Require readline 7 or newer

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>
> Date: Tue,  6 Aug 2019 14:43:34 -0600
> 
> This changes gdb to require readline 7 or newer at build time.
> 
> 2019-04-21  Tom Tromey  <tom@tromey.com>
> 
> 	* README: Update.
> 	* event-top.c: Require readline 7.
> 
> gdb/doc/ChangeLog
> 2019-04-21  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.texinfo (Configure Options): Document minimum version of
> 	readline.

The documentation parts are OK, thanks.

Re: [PATCH 0/8] Upgrade readline

[Kevin Buettner]

On Tue,  6 Aug 2019 14:43:26 -0600
Tom Tromey <tom@tromey.com> wrote:

> Here's the series to upgrade readline.  This has been sitting around
> for a while and I thought I would finally send it.
> 
> I built it on a Windows machine at AdaCore, but although readline is
> built and gdb links against it, I can't get it to work.  Maybe this is
> due to whatever remote terminal I am using -- I don't know.
> 
> Tested on x86-64 Fedora 28. Also, note that the major distros use
> --with-system-readline, which I think is further evidence that this
> works.
> 
> This was too big to test on the buildbot.
> 
> Let me know what you think.

I glanced at patches 2-5, and 8.  For some reason, 1 and 6 haven't
shown up yet.  The ones I've looked at look reasonable.

I tried building GDB on one of the BSDs earlier this year, which lacked
an obvious system readline.  (It was available, but I either had to
install it or tell configure where it was, maybe both.)  In any case,
I had build problems with the in-tree readline.  I then tried building
GDB on F29 with system readline disabled and ran into the same
problems.  So I'm in favor of anything which fixes that breakage.
So, basically, if it builds, I think it should go in.

Kevin

Re: [PATCH 0/8] Upgrade readline

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> I glanced at patches 2-5, and 8.  For some reason, 1 and 6 haven't
Kevin> shown up yet.  The ones I've looked at look reasonable.

I think patch 1 is probably too big.

Patch 6 was rejected by the MTA since I didn't write it & so it had the
wrong "From".

I'm appending patch 6 here, and I'll try to send patch 1 compressed.

Otherwise the easiest way is to fetch from my github.  The branch is
"submit/readline-upgrade".

Tom

commit 6d6a1c51e46421b05ebb8e3596f85e6e5c952785
Author: Patrick Palka <patrick@parcs.ath.cx>
Date:   Sun Mar 17 08:32:16 2019 -0600

    Fix gdb's selftest.exp after readline import
    
    After the sync there is one testsuite regression, the test
    "signal SIGINT" in gdb.gdb/selftest.exp which now FAILs.  Previously,
    the readline 6.2 SIGINT handler would temporarily reinstall the
    underlying application's SIGINT handler and immediately re-raise SIGINT
    so that the orginal handler gets invoked.  But now (since readline 6.3)
    its SIGINT handler does not re-raise SIGINT or directly invoke the
    original handler; it now sets a flag marking that SIGINT was raised, and
    waits until readline explicitly has control to call the application's
    SIGINT handler.  Anyway, because SIGINT is no longer re-raised from
    within readline's SIGINT handler, doing "signal SIGINT" with a stopped
    inferior gdb process will no longer resume and then immediately stop the
    process (since there is no 2nd SIGINT to immediately catch).  Instead,
    the inferior gdb process will now just print "Quit" and continue to run.
    So with this commit, this particular test case is adjusted to reflect
    this change in behavior (we now have to send a 2nd SIGINT manually to
    stop it).
    
    gdb/testsuite/ChangeLog
    2015-07-25  Patrick Palka  <patrick@parcs.ath.cx>
    
            * gdb.gdb/selftest.exp (test_with_self): Update test to now
            expect the GDB inferior to no longer immediately stop after
            being resumed with "signal SIGINT".

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 49783ebac31..202bc9fb0e3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-07-25  Patrick Palka  <patrick@parcs.ath.cx>
+
+	* gdb.gdb/selftest.exp (test_with_self): Update test to now
+	expect the GDB inferior to no longer immediately stop after
+	being resumed with "signal SIGINT".
+
 2019-08-06  Tom Tromey  <tom@tromey.com>
 
 	* gdb.base/style.exp: Add disassemble test.
diff --git a/gdb/testsuite/gdb.gdb/selftest.exp b/gdb/testsuite/gdb.gdb/selftest.exp
index 7fdb3b226e1..9651561fafe 100644
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -110,9 +110,26 @@ proc test_with_self { } {
     }
     
     set description "send SIGINT signal to child process"
-    gdb_test "signal SIGINT" \
-	"Continuing with signal SIGINT.*" \
-	"$description"
+    gdb_test_multiple "signal SIGINT" "$description" {
+	-re "^signal SIGINT\r\nContinuing with signal SIGINT.\r\nQuit\r\n.* $" {
+	    pass "$description"
+	}
+    }
+
+    set description "send ^C to child process again"
+    send_gdb "\003"
+    gdb_expect {
+	-re "Program received signal SIGINT.*$gdb_prompt $" {
+	    pass "$description"
+	}
+	-re ".*$gdb_prompt $" {
+	    fail "$description"
+	}
+	timeout {
+	    fail "$description (timeout)"
+	}
+    }
+
 
     # Switch back to the GDB thread if Guile support is linked in.
     # "signal SIGINT" could also switch the current thread.

Re: [PATCH 0/8] Upgrade readline

[Tom Tromey]

[-- Attachment #1: Type: text/plain, Size: 22 bytes --]

Here's patch 1.

Tom


[-- Attachment #2: patch #1 --]
[-- Type: application/x-xz, Size: 135196 bytes --]

Re: [PATCH 7/8] Remove readline hack from gdb_select

[Pedro Alves]

On 8/6/19 9:43 PM, Tom Tromey wrote:
> As discussed on gdb-patches, this removes the readline hack from the
> mingw-hdep.c version of gdb_select.  It's believed that this is not
> needed any more.

I may be hard to find the discussion later on.
Please add a link to the discussion to the commit log
pointing here:

 https://sourceware.org/ml/gdb-patches/2019-03/msg00465.html

Thanks,
Pedro Alves

> 
> 2019-08-04  Tom Tromey  <tom@tromey.com>
> 
> 	* mingw-hdep.c (gdb_select): Remove readline hack.
> ---
>  gdb/ChangeLog    | 4 ++++
>  gdb/mingw-hdep.c | 9 ---------
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/mingw-hdep.c b/gdb/mingw-hdep.c
> index 8ed4b44ddce..44fb22e9a16 100644
> --- a/gdb/mingw-hdep.c
> +++ b/gdb/mingw-hdep.c
> @@ -23,7 +23,6 @@
>  #include "event-loop.h"
>  
>  #include "gdb_select.h"
> -#include "readline/readline.h"
>  
>  #include <windows.h>
>  
> @@ -167,14 +166,6 @@ gdb_select (int n, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	}
>      }
>  
> -  /* With multi-threaded SIGINT handling, there is a race between the
> -     readline signal handler and GDB.  It may still be in
> -     rl_prep_terminal in another thread.  Do not return until it is
> -     done; we can check the state here because we never longjmp from
> -     signal handlers on Windows.  */
> -  while (RL_ISSTATE (RL_STATE_SIGHANDLER))
> -    Sleep (1);
> -
>    return num_ready;
>  }
>  
> 

Re: [PATCH 8/8] Require readline 7 or newer

[Pedro Alves]

On 8/6/19 9:43 PM, Tom Tromey wrote:

>  @item --with-system-readline
>  Use the readline library installed on the host, rather than the
> -library supplied as part of @value{GDBN}.
> +library supplied as part of @value{GDBN}.  Readline 7 or newer is
> +required; this is enforced by the build system.
>  

> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -48,6 +48,9 @@
>  /* readline defines this.  */
>  #undef savestring
>  
> +/* gdb requires readline 7 now.  */
> +gdb_static_assert (RL_VERSION_MAJOR >= 7);
> +

I'd be much better user experience if this were done at by the
build system, at configure time, with AC_TRY_COMPILE, IMO.  Something
similar to the "GNU regex" check should do it.

As is, it's plausible that the build would error out failing to compile
some other .c file that happened to use some readline symbol/struct/function/etc.
that only exists in the supported readline.  Alternatively, we could have
some gdb_readline.h wrapper header and do the check there, though a configure
check seems natural to me and should be simple.

Thanks,
Pedro Alves

Re: [PATCH 8/8] Require readline 7 or newer

[Pedro Alves]

On 8/6/19 9:43 PM, Tom Tromey wrote:
> This changes gdb to require readline 7 or newer at build time.
> 
> 2019-04-21  Tom Tromey  <tom@tromey.com>
> 
> 	* README: Update.
> 	* event-top.c: Require readline 7.
> 
> gdb/doc/ChangeLog
> 2019-04-21  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.texinfo (Configure Options): Document minimum version of
> 	readline.

This should be mentioned in gdb/NEWS too, IMO.

Thanks,
Pedro Alves

Re: [PATCH 0/8] Upgrade readline

[Sergio Durigan Junior]

On Tuesday, August 06 2019, Tom Tromey wrote:

> Here's the series to upgrade readline.  This has been sitting around
> for a while and I thought I would finally send it.
>
> I built it on a Windows machine at AdaCore, but although readline is
> built and gdb links against it, I can't get it to work.  Maybe this is
> due to whatever remote terminal I am using -- I don't know.
>
> Tested on x86-64 Fedora 28. Also, note that the major distros use
> --with-system-readline, which I think is further evidence that this
> works.

As we discussed on #gdb yesterday, both Fedora and Debian build GDB
using --with-system-readline, so, in a way, for the majority of users
our local copy of readline doesn't matter.

I'm in favour of bumping the readline version to 7 (note that Debian
oldstable, i.e., wheezy, which was released 4+ years ago, already ships
with readline 7), and (eventually) just get rid of our local copy.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 0/8] Upgrade readline

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> I'm in favour of bumping the readline version to 7 (note that Debian
Sergio> oldstable, i.e., wheezy, which was released 4+ years ago, already ships
Sergio> with readline 7), and (eventually) just get rid of our local copy.

We talked about that briefly on irc yesterday too.

I wonder if we really could get rid of the local copy.  I mean,
obviously we could, but would it be a problem for anybody?

We could treat it a few ways.  One would be like libiconv: keep the
top-level configury around so it's possible to drop the readline sources
into the tree and then build.

Another way would be to use something like guix for these dependencies.
I don't know if that works on all the hosts that we care about.

The guix way is attractive since it seems vaguely analogous to using
"cargo" in the Rust world.  In particular if we could do something like
this, maybe we could be less conservative about bringing in new
dependencies.


I think either of these solutions would also fix the bug we found with
moving gdbsupport to the top level (i.e. that it interacts poorly with
--with-system-readline).  (I never got a response to that note, so if
you're reading this, I'd appreciate a quick look at that as well.)

thanks,
Tom

Re: [PATCH 7/8] Remove readline hack from gdb_select

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 8/6/19 9:43 PM, Tom Tromey wrote:
>> As discussed on gdb-patches, this removes the readline hack from the
>> mingw-hdep.c version of gdb_select.  It's believed that this is not
>> needed any more.

Pedro> I may be hard to find the discussion later on.
Pedro> Please add a link to the discussion to the commit log
Pedro> pointing here:

Pedro>  https://sourceware.org/ml/gdb-patches/2019-03/msg00465.html

Thanks, I did this.

Also, Christian Biesinger managed to build this branch on Windows and
try it, which is very good news.

Tom

Re: [PATCH 7/8] Remove readline hack from gdb_select

[Christian Biesinger via gdb-patches]

On Wed, Aug 7, 2019 at 5:03 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> On 8/6/19 9:43 PM, Tom Tromey wrote:
> >> As discussed on gdb-patches, this removes the readline hack from the
> >> mingw-hdep.c version of gdb_select.  It's believed that this is not
> >> needed any more.
>
> Pedro> I may be hard to find the discussion later on.
> Pedro> Please add a link to the discussion to the commit log
> Pedro> pointing here:
>
> Pedro>  https://sourceware.org/ml/gdb-patches/2019-03/msg00465.html
>
> Thanks, I did this.
>
> Also, Christian Biesinger managed to build this branch on Windows and
> try it, which is very good news.

Yep, it worked fine (up/down arrow worked, Ctrl+R worked).

For reference, I compiled in cygwin but with mingw, using:
 ../configure --disable-binutils --disable-gas --disable-gold
--disable-gprof --disable-ld --disable-werror --disable-nls

I had to copy libwinpthread-1.dll to out/gdb.

Christian

Re: [PATCH 8/8] Require readline 7 or newer

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'd be much better user experience if this were done at by the
Pedro> build system, at configure time, with AC_TRY_COMPILE, IMO.  Something
Pedro> similar to the "GNU regex" check should do it.

Makes sense.  Here's a new patch that addresses this and the NEWS thing.

Tom

commit 332eb34e3d21c1a3de2b4f6c874912321da1b3a4
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Apr 21 13:58:49 2019 -0600

    Require readline 7 or newer
    
    This changes gdb to require readline 7 or newer at build time.
    
    gdb/ChangeLog
    2019-08-07  Tom Tromey  <tom@tromey.com>
    
            * configure: Rebuild.
            * configure.ac: Check for readline 7.
            * NEWS: Mention readline 7 requirement.
            * README: Update.
    
    gdb/doc/ChangeLog
    2019-04-21  Tom Tromey  <tom@tromey.com>
    
            * gdb.texinfo (Configure Options): Document minimum version of
            readline.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2e05431607c..54d35df7a01 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-08-07  Tom Tromey  <tom@tromey.com>
+
+	* configure: Rebuild.
+	* configure.ac: Check for readline 7.
+	* NEWS: Mention readline 7 requirement.
+	* README: Update.
+
 2019-08-04  Tom Tromey  <tom@tromey.com>
 
 	* mingw-hdep.c (gdb_select): Remove readline hack.
diff --git a/gdb/NEWS b/gdb/NEWS
index fa01adf6e89..9f37e7c1079 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -299,6 +299,11 @@ maint show test-options-completion-result
   Using another implementation of the make program or an earlier version of
   GNU make to build GDB or GDBserver is not supported.
 
+* Building GDB and GDBserver now requires GNU readline >= 7.0.
+
+  GDB now bundles GNU readline 8.0, but if you choose to use
+  --with-system-readline, only readline >= 7.0 can be used.
+
 *** Changes in GDB 8.3
 
 * GDB and GDBserver now support access to additional registers on
diff --git a/gdb/README b/gdb/README
index 8a91aab2a4c..8883a8a09e3 100644
--- a/gdb/README
+++ b/gdb/README
@@ -439,7 +439,8 @@ more obscure GDB `configure' options are not listed here.
 
 `--with-system-readline'
      Use the readline library installed on the host, rather than the
-     library supplied as part of GDB.
+     library supplied as part of GDB.  Readline 7 or newer is required;
+     this is enforced by the build system.
 
 `--with-system-zlib
      Use the zlib library installed on the host, rather than the
diff --git a/gdb/configure b/gdb/configure
index 9206f0e7f88..2832c836177 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -8952,6 +8952,38 @@ fi
 
 
 if test "$with_system_readline" = yes; then
+   { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether system readline is new enough" >&5
+$as_echo_n "checking whether system readline is new enough... " >&6; }
+if ${gdb_cv_readline_ok+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#include <stdio.h>
+#include <readline/readline.h>
+int
+main ()
+{
+#if RL_VERSION_MAJOR < 7
+# error "readline version 7 required"
+#endif
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  gdb_cv_readline_ok=yes
+else
+  gdb_cv_readline_ok=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gdb_cv_readline_ok" >&5
+$as_echo "$gdb_cv_readline_ok" >&6; }
+  if test "$gdb_cv_readline_ok" != yes; then
+    as_fn_error $? "system readline is not new enough" "$LINENO" 5
+  fi
+
   READLINE=-lreadline
   READLINE_DEPS=
   READLINE_CFLAGS=
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 05b722b7f11..0979109d976 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -581,6 +581,20 @@ AC_ARG_WITH([system-readline],
                   [use installed readline library])])
 
 if test "$with_system_readline" = yes; then
+   AC_CACHE_CHECK([whether system readline is new enough],
+     [gdb_cv_readline_ok],
+     [AC_TRY_COMPILE(
+       [#include <stdio.h>
+#include <readline/readline.h>],
+       [#if RL_VERSION_MAJOR < 7
+# error "readline version 7 required"
+#endif],
+    gdb_cv_readline_ok=yes,
+    gdb_cv_readline_ok=no)])
+  if test "$gdb_cv_readline_ok" != yes; then
+    AC_MSG_ERROR([system readline is not new enough])
+  fi
+
   READLINE=-lreadline
   READLINE_DEPS=
   READLINE_CFLAGS=
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 702bb7c7a02..f25b468468b 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2019-04-21  Tom Tromey  <tom@tromey.com>
+
+	* gdb.texinfo (Configure Options): Document minimum version of
+	readline.
+
 2019-08-07  Alan Hayward  <alan.hayward@arm.com>
 
 	* gdb.texinfo (AArch64 Pointer Authentication): New subsection.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7f8c0aff1cd..e36b2d59745 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -36905,7 +36905,8 @@ details.
 
 @item --with-system-readline
 Use the readline library installed on the host, rather than the
-library supplied as part of @value{GDBN}.
+library supplied as part of @value{GDBN}.  Readline 7 or newer is
+required; this is enforced by the build system.
 
 @item --with-system-zlib
 Use the zlib library installed on the host, rather than the library

Re: [PATCH 8/8] Require readline 7 or newer

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
> Date: Wed, 07 Aug 2019 16:30:56 -0600
> 
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I'd be much better user experience if this were done at by the
> Pedro> build system, at configure time, with AC_TRY_COMPILE, IMO.  Something
> Pedro> similar to the "GNU regex" check should do it.
> 
> Makes sense.  Here's a new patch that addresses this and the NEWS thing.

OK for the documentation parts.

Thanks.

Re: [PATCH 8/8] Require readline 7 or newer

[Pedro Alves]

On 8/7/19 11:30 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I'd be much better user experience if this were done at by the
> Pedro> build system, at configure time, with AC_TRY_COMPILE, IMO.  Something
> Pedro> similar to the "GNU regex" check should do it.
> 
> Makes sense.  Here's a new patch that addresses this and the NEWS thing.

Looks good, thanks.

Pedro Alves

Re: [PATCH 8/8] Require readline 7 or newer

[Pedro Alves]

On 8/7/19 11:30 PM, Tom Tromey wrote:
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -299,6 +299,11 @@ maint show test-options-completion-result
>    Using another implementation of the make program or an earlier version of
>    GNU make to build GDB or GDBserver is not supported.
>  
> +* Building GDB and GDBserver now requires GNU readline >= 7.0.

Actually, the "and GDBserver" part is incorrect, since GDBserver
does not depend on readline.  Better remove that IMO, lest someone
mistakenly thinks that building gdbserver (without building gdb)
now requires readline.

Thanks,
Pedro Alves

Re: [PATCH 8/8] Require readline 7 or newer

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 8/7/19 11:30 PM, Tom Tromey wrote:
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -299,6 +299,11 @@ maint show test-options-completion-result
>> Using another implementation of the make program or an earlier version of
>> GNU make to build GDB or GDBserver is not supported.
>> 
>> +* Building GDB and GDBserver now requires GNU readline >= 7.0.

Pedro> Actually, the "and GDBserver" part is incorrect, since GDBserver
Pedro> does not depend on readline.  Better remove that IMO, lest someone
Pedro> mistakenly thinks that building gdbserver (without building gdb)
Pedro> now requires readline.

Oops, bad cut-and-paste. I fixed this.

Tom

Re: [PATCH 0/8] Upgrade readline

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> Here's the series to upgrade readline.  This has been sitting around
Tom> for a while and I thought I would finally send it.

I'm checking this in now.

Tom

Re: [PATCH 0/8] Upgrade readline

[Sergio Durigan Junior]

On Wednesday, August 07 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> I'm in favour of bumping the readline version to 7 (note that Debian
> Sergio> oldstable, i.e., wheezy, which was released 4+ years ago, already ships
> Sergio> with readline 7), and (eventually) just get rid of our local copy.
>
> We talked about that briefly on irc yesterday too.

Yes (for a different value of "yesterday" now).

> I wonder if we really could get rid of the local copy.  I mean,
> obviously we could, but would it be a problem for anybody?

[ /me puts his downstream hat ]

I guess it depends.  If the person is building GDB on a system that
doesn't offer readline-dev or a similar package, then it can be a
"problem" in the sense that he or she will have to compile readline by
hand, probably.

Otherwise, I don't see how it can be a problem.  As I pointed out during
the IRC discussion, the two major GNU distros (Fedora and Debian) are
already compiling their GDB packages using --with-system-readline, so,
in a way, our readline copy is not needed.  These distros also provide
readline-dev, which makes it very easy for users to compile their own
GDBs without using our local readline copy.

OOC, I went to check how Arch[GNU/]Linux compiles GDB, and they are also
using --with-system-readline.

> We could treat it a few ways.  One would be like libiconv: keep the
> top-level configury around so it's possible to drop the readline sources
> into the tree and then build.

That'd be a good compromise, IMO.

> Another way would be to use something like guix for these dependencies.
> I don't know if that works on all the hosts that we care about.
>
> The guix way is attractive since it seems vaguely analogous to using
> "cargo" in the Rust world.  In particular if we could do something like
> this, maybe we could be less conservative about bringing in new
> dependencies.

The guix idea seems awesome, but that's because I like guix ;-).  If I'm
honest, I don't like the idea of keeping readline in-tree at all; I'd
prefer to have guix manage some "obscure" dependency that can be missing
in some system.  But that's me and my "let's not turn everything into a
flatpak" feeling ;-).

> I think either of these solutions would also fix the bug we found with
> moving gdbsupport to the top level (i.e. that it interacts poorly with
> --with-system-readline).  (I never got a response to that note, so if
> you're reading this, I'd appreciate a quick look at that as well.)

I'll take a look, thanks for mentioning!

Cheers,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 0/8] Upgrade readline

[Christian Biesinger via gdb-patches]

On Mon, Aug 12, 2019 at 2:46 PM Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Wednesday, August 07 2019, Tom Tromey wrote:
> >>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
> >
> > Sergio> I'm in favour of bumping the readline version to 7 (note that Debian
> > Sergio> oldstable, i.e., wheezy, which was released 4+ years ago, already ships
> > Sergio> with readline 7), and (eventually) just get rid of our local copy.
> >
> > We talked about that briefly on irc yesterday too.
>
> Yes (for a different value of "yesterday" now).
>
> > I wonder if we really could get rid of the local copy.  I mean,
> > obviously we could, but would it be a problem for anybody?
>
> [ /me puts his downstream hat ]
>
> I guess it depends.  If the person is building GDB on a system that
> doesn't offer readline-dev or a similar package, then it can be a
> "problem" in the sense that he or she will have to compile readline by
> hand, probably.

Windows may be the OS where that's the hardest. But maybe that doesn't
matter too much -- cygwin/mingw/etc could just provide a prebuilt
libreadline as well?

Christian

[committed][gdb/testsuite] Fix gdb.gdb/selftest.exp regexp

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

[ was Re: [PATCH 5/8] Fix gdb's selftest.exp after readline import ]

On 06-08-19 22:43, Tom Tromey wrote:
> From: Patrick Palka <patrick@parcs.ath.cx>
> 
> +    gdb_expect {
> +	-re "Program received signal SIGINT.*$gdb_prompt $" {
> +	    pass "$description"
> +	}

I've fixed this regexp to also accept "Thread" instead of "Program".

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Fix-gdb.gdb-selftest.exp-regexp.patch --]
[-- Type: text/x-patch, Size: 1350 bytes --]

[gdb/testsuite] Fix gdb.gdb/selftest.exp regexp

With gdb.gdb/selftest.exp, we get:
...
(xgdb) PASS: gdb.gdb/selftest.exp: send SIGINT signal to child process
^M
Thread 1 "xgdb" received signal SIGINT, Interrupt.^M
0x00007ffff5bf01db in poll () from /lib64/libc.so.6^M
(gdb) FAIL: gdb.gdb/selftest.exp: send ^C to child process again
...

The failure is due to gdb printing 'Thread 1 "xgdb" received signal SIGINT',
but the regexp in the test-case expecting 'Program received signal SIGINT'.

Fix this by updating the regexp, similar to how that is done earlier in the
test-case.

gdb/testsuite/ChangeLog:

2019-08-13  Tom de Vries  <tdevries@suse.de>

	* gdb.gdb/selftest.exp (send ^C to child process again): Accept also
	Thread.

---
 gdb/testsuite/gdb.gdb/selftest.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.gdb/selftest.exp b/gdb/testsuite/gdb.gdb/selftest.exp
index 9651561faf..2f481ec404 100644
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -119,7 +119,7 @@ proc test_with_self { } {
     set description "send ^C to child process again"
     send_gdb "\003"
     gdb_expect {
-	-re "Program received signal SIGINT.*$gdb_prompt $" {
+	-re "(Thread .*|Program) received signal SIGINT.*$gdb_prompt $" {
 	    pass "$description"
 	}
 	-re ".*$gdb_prompt $" {

Re: [PATCH 0/8] Upgrade readline

[Tom de Vries]

On 07-08-19 15:40, Tom Tromey wrote:
> Here's patch 1.

This caused a regression with check-read1 in gdb.tui/basic.exp:
...
FAIL: gdb.tui/basic.exp: list main
XPASS: gdb.tui/basic.exp: source box
FAIL: gdb.tui/basic.exp: asm window shows main
FAIL: gdb.tui/basic.exp: split layout contents
FAIL: gdb.tui/basic.exp: asm box in split layout (ul corner)
...

Thanks,
- Tom

Re: [PATCH 0/8] Upgrade readline

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> On 07-08-19 15:40, Tom Tromey wrote:
>> Here's patch 1.

Tom> This caused a regression with check-read1 in gdb.tui/basic.exp:
Tom> ...
Tom> FAIL: gdb.tui/basic.exp: list main
Tom> XPASS: gdb.tui/basic.exp: source box
Tom> FAIL: gdb.tui/basic.exp: asm window shows main
Tom> FAIL: gdb.tui/basic.exp: split layout contents
Tom> FAIL: gdb.tui/basic.exp: asm box in split layout (ul corner)
Tom> ...

Thanks.  Was it this patch or one of the TUI patches?

Anyway, the best thing would be if you could:

    runtest -v gdb.tui/basic.exp

... and then send me the resulting gdb.log.  Often that has enough
information to diagnose TUI bugs.

thanks,
Tom

Re: [PATCH 0/8] Upgrade readline

[Tom de Vries]

On 15-08-19 15:45, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> On 07-08-19 15:40, Tom Tromey wrote:
>>> Here's patch 1.
> 
> Tom> This caused a regression with check-read1 in gdb.tui/basic.exp:
> Tom> ...
> Tom> FAIL: gdb.tui/basic.exp: list main
> Tom> XPASS: gdb.tui/basic.exp: source box
> Tom> FAIL: gdb.tui/basic.exp: asm window shows main
> Tom> FAIL: gdb.tui/basic.exp: split layout contents
> Tom> FAIL: gdb.tui/basic.exp: asm box in split layout (ul corner)
> Tom> ...
> 
> Thanks.  Was it this patch or one of the TUI patches?
> 

This patch.

> Anyway, the best thing would be if you could:
> 
>     runtest -v gdb.tui/basic.exp
> 
> ... and then send me the resulting gdb.log.  Often that has enough
> information to diagnose TUI bugs.

Filed https://sourceware.org/bugzilla/show_bug.cgi?id=24917 and attached
the log there.

Thanks,
- Tom