[PATCH 0/8] Upgrade readline

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/8] Upgrade readline

Tom Tromey-2
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


Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Tom Tromey-2
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Tom Tromey-2
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Tom Tromey-2
From: Patrick Palka <[hidden email]>

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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

[PATCH 7/8] Remove readline hack from gdb_select

Tom Tromey-2
In reply to this post by Tom Tromey-2
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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

[PATCH 8/8] Require readline 7 or newer

Tom Tromey-2
In reply to this post by Tom Tromey-2
This changes gdb to require readline 7 or newer at build time.

2019-04-21  Tom Tromey  <[hidden email]>

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

gdb/doc/ChangeLog
2019-04-21  Tom Tromey  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Tom Tromey <[hidden email]>
> Cc: Tom Tromey <[hidden email]>
> 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  <[hidden email]>
>
> * README: Update.
> * event-top.c: Require readline 7.
>
> gdb/doc/ChangeLog
> 2019-04-21  Tom Tromey  <[hidden email]>
>
> * gdb.texinfo (Configure Options): Document minimum version of
> readline.

The documentation parts are OK, thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] Upgrade readline

Kevin Buettner
In reply to this post by Tom Tromey-2
On Tue,  6 Aug 2019 14:43:26 -0600
Tom Tromey <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] Upgrade readline

Tom Tromey-2
>>>>> "Kevin" == Kevin Buettner <[hidden email]> 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 <[hidden email]>
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  <[hidden email]>
   
            * 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  <[hidden email]>
+
+ * 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  <[hidden email]>
 
  * 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] Upgrade readline

Tom Tromey-2
In reply to this post by Kevin Buettner
Here's patch 1.

Tom


P2.xz (181K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Tom Tromey-2
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  <[hidden email]>
>
> * 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;
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Tom Tromey-2
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
Reply | Threaded
Open this post in threaded view
|

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

Pedro Alves-7
In reply to this post by Tom Tromey-2
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  <[hidden email]>
>
> * README: Update.
> * event-top.c: Require readline 7.
>
> gdb/doc/ChangeLog
> 2019-04-21  Tom Tromey  <[hidden email]>
>
> * gdb.texinfo (Configure Options): Document minimum version of
> readline.

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

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] Upgrade readline

Sergio Durigan Junior
In reply to this post by Tom Tromey-2
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/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] Upgrade readline

Tom Tromey-2
>>>>> "Sergio" == Sergio Durigan Junior <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
On Wed, Aug 7, 2019 at 5:03 PM Tom Tromey <[hidden email]> wrote:

>
> >>>>> "Pedro" == Pedro Alves <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> 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 <[hidden email]>
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  <[hidden email]>
   
            * configure: Rebuild.
            * configure.ac: Check for readline 7.
            * NEWS: Mention readline 7 requirement.
            * README: Update.
   
    gdb/doc/ChangeLog
    2019-04-21  Tom Tromey  <[hidden email]>
   
            * 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  <[hidden email]>
+
+ * configure: Rebuild.
+ * configure.ac: Check for readline 7.
+ * NEWS: Mention readline 7 requirement.
+ * README: Update.
+
 2019-08-04  Tom Tromey  <[hidden email]>
 
  * 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  <[hidden email]>
+
+ * gdb.texinfo (Configure Options): Document minimum version of
+ readline.
+
 2019-08-07  Alan Hayward  <[hidden email]>
 
  * 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
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Tom Tromey <[hidden email]>
> Cc: Tom Tromey <[hidden email]>,  [hidden email]
> Date: Wed, 07 Aug 2019 16:30:56 -0600
>
> >>>>> "Pedro" == Pedro Alves <[hidden email]> 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.
12