[gdb FYI-patch] callback-mode readline-6.0 regression

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Chet,

FSF GDB currently ships bundled with readline-5.2 which works fine.
But using --with-system-readline and readline-6.0-patchlevel4 has
a regression:

readline-5.2: Run `gdb -nx -q' and type CTRL-C:
(gdb) Quit
(gdb) _

readline-6.0: Run `gdb -nx -q' and type CTRL-C:
(gdb) _
 = nothing happens (it gets buffered and executed later)
	(It does also FAIL on gdb.gdb/selftest.exp.)

It is because GDB waits in its own poll() mainloop and readline uses via
rl_callback_handler_install and rl_callback_handler_remove.  This way the
readline internal variable _rl_interrupt_immediately remains 0 and CTRL-C gets
only stored to _rl_caught_signal but not executed.

Seen in rl_signal_handler even if _rl_interrupt_immediately is set and
_rl_handle_signal is called then the signal is still stored to
_rl_caught_signal.  In the _rl_interrupt_immediately case it should not be
stored when it was already processed.

rl_signal_handler does `_rl_interrupt_immediately = 0;' - while I am not aware
of its meaning it breaks the nest-counting of other routines which do
`_rl_interrupt_immediately++;' and `_rl_interrupt_immediately--;' possibly
creating problematic `_rl_interrupt_immediately == -1'.

`_rl_interrupt_immediately' is an internal variable, how it could be accessed
by a readline application? (OK, maybe it should not be used.)

Attaching a current GDB-side patch but it must access readline internal
variable _rl_caught_signal and it is generally just a workaround.  Could you
please include support for signals in this asynchronous mode in readline-6.1?
I find it would be enough to make RL_CHECK_SIGNALS public?


GDB: No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
But this is not a patch intended to be accepted.


Thanks,
Jan


gdb/
2009-11-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* config.in, configure: Regenerate.
	* configure.ac (for readline_echoing_p): Move inside $LIBS change.
	(for _rl_caught_signal): New.
	* event-loop.c: Include readline/readline.h.
	(gdb_do_one_event) [HAVE_READLINE_CAUGHT_SIGNAL]: New.

gdb/testsuite/
2009-11-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.gdb/selftest.exp (backtrace through signal handler): Move before
	SIGINT pass, drop the timeout case.
	(send SIGINT signal to child process): Use gdb_test.
	(backtrace through readline handler): New.

--- a/gdb/config.in
+++ b/gdb/config.in
@@ -351,6 +351,9 @@
 /* Define if Python interpreter is being linked in. */
 #undef HAVE_PYTHON
 
+/* readline-6.0 workaround of blocked signals. */
+#undef HAVE_READLINE_CAUGHT_SIGNAL
+
 /* Define to 1 if you have the `realpath' function. */
 #undef HAVE_REALPATH
 
--- a/gdb/configure
+++ b/gdb/configure
@@ -8688,10 +8688,10 @@ if test "$with_system_readline" = yes; then
   # readline-6.0 started to use the name `_rl_echoing_p'.
   # `$(READLINE_DIR)/' of bundled readline would not resolve in configure.
 
-  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for readline_echoing_p" >&5
-$as_echo_n "checking for readline_echoing_p... " >&6; }
   save_LIBS=$LIBS
   LIBS="$LIBS $READLINE"
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for readline_echoing_p" >&5
+$as_echo_n "checking for readline_echoing_p... " >&6; }
   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 
@@ -8714,9 +8714,35 @@ $as_echo "#define readline_echoing_p _rl_echoing_p" >>confdefs.h
 fi
 rm -f core conftest.err conftest.$ac_objext \
     conftest$ac_exeext conftest.$ac_ext
-  LIBS="$save_LIBS"
   { $as_echo "$as_me:${as_lineno-$LINENO}: result: $READLINE_ECHOING_P" >&5
 $as_echo "$READLINE_ECHOING_P" >&6; }
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for _rl_caught_signal" >&5
+$as_echo_n "checking for _rl_caught_signal... " >&6; }
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+extern int volatile _rl_caught_signal;
+				    return _rl_caught_signal;
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  READLINE_CAUGHT_SIGNAL=yes
+
+$as_echo "#define HAVE_READLINE_CAUGHT_SIGNAL /**/" >>confdefs.h
+
+else
+  READLINE_CAUGHT_SIGNAL=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+    conftest$ac_exeext conftest.$ac_ext
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $READLINE_CAUGHT_SIGNAL" >&5
+$as_echo "$READLINE_CAUGHT_SIGNAL" >&6; }
+  LIBS="$save_LIBS"
 else
   READLINE='$(READLINE_DIR)/libreadline.a'
   READLINE_DEPS='$(READLINE)'
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -539,17 +539,25 @@ if test "$with_system_readline" = yes; then
   # readline-6.0 started to use the name `_rl_echoing_p'.
   # `$(READLINE_DIR)/' of bundled readline would not resolve in configure.
 
-  AC_MSG_CHECKING([for readline_echoing_p])
   save_LIBS=$LIBS
   LIBS="$LIBS $READLINE"
+  AC_MSG_CHECKING([for readline_echoing_p])
   AC_LINK_IFELSE(AC_LANG_PROGRAM(,[[extern int readline_echoing_p;
 				    return readline_echoing_p;]]),
 		 [READLINE_ECHOING_P=yes],
 		 [READLINE_ECHOING_P=no
 		  AC_DEFINE([readline_echoing_p], [_rl_echoing_p],
 			    [readline-6.0 started to use different name.])])
-  LIBS="$save_LIBS"
   AC_MSG_RESULT([$READLINE_ECHOING_P])
+  AC_MSG_CHECKING([for _rl_caught_signal])
+  AC_LINK_IFELSE(AC_LANG_PROGRAM(,[[extern int volatile _rl_caught_signal;
+				    return _rl_caught_signal;]]),
+		 [READLINE_CAUGHT_SIGNAL=yes
+		  AC_DEFINE([HAVE_READLINE_CAUGHT_SIGNAL],,
+			    [readline-6.0 workaround of blocked signals.])],
+		 [READLINE_CAUGHT_SIGNAL=no])
+  AC_MSG_RESULT([$READLINE_CAUGHT_SIGNAL])
+  LIBS="$save_LIBS"
 else
   READLINE='$(READLINE_DIR)/libreadline.a'
   READLINE_DEPS='$(READLINE)'
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -37,6 +37,7 @@
 #include "exceptions.h"
 #include "gdb_assert.h"
 #include "gdb_select.h"
+#include "readline/readline.h"
 
 /* Data point to pass to the event handler.  */
 typedef union event_data
@@ -411,6 +412,9 @@ gdb_do_one_event (void *data)
   static int event_source_head = 0;
   const int number_of_sources = 3;
   int current = 0;
+#ifdef HAVE_READLINE_CAUGHT_SIGNAL
+  extern int volatile _rl_caught_signal;
+#endif
 
   /* Any events already waiting in the queue?  */
   if (process_event ())
@@ -455,6 +459,16 @@ gdb_do_one_event (void *data)
   if (gdb_wait_for_event (1) < 0)
     return -1;
 
+#ifdef HAVE_READLINE_CAUGHT_SIGNAL
+  if (async_command_editing_p && RL_ISSTATE (RL_STATE_CALLBACK)
+      && _rl_caught_signal)
+    {
+      /* Call RL_CHECK_SIGNALS this way.  */
+      rl_callback_handler_remove ();
+      rl_callback_handler_install (NULL, input_handler);
+    }
+#endif
+
   /* Handle any new events occurred while waiting.  */
   if (process_event ())
     return 1;
--- a/gdb/testsuite/gdb.gdb/selftest.exp
+++ b/gdb/testsuite/gdb.gdb/selftest.exp
@@ -464,31 +464,42 @@ GDB.*Copyright \[0-9\]+ Free Software Foundation, Inc..*$gdb_prompt $"\
 	    fail "$description (timeout)"
 	}
     }
-    
-    set description "send SIGINT signal to child process"
-    send_gdb "signal SIGINT\n"
-    gdb_expect {
-	-re "Continuing with signal SIGINT.*$gdb_prompt $" {
+
+    # get a stack trace with the poll function
+    #
+    # This fails on some linux systems for unknown reasons.  On the
+    # systems where it fails, sometimes it works fine when run manually.
+    # The testsuite failures may not be limited to just aout systems.
+    setup_xfail "i*86-pc-linuxaout-gnu"
+    set description "backtrace through signal handler"
+    gdb_test_multiple "backtrace" $description {
+	-re "#0.*(read|poll).*in main \\(.*\\) at .*gdb\\.c.*$gdb_prompt $" {
 	    pass "$description"
 	}
 	-re ".*$gdb_prompt $" {
+	    # On the alpha, we hit the infamous problem about gdb
+	    # being unable to get the frame pointer (mentioned in
+	    # gdb/README).  As it is intermittent, there is no way to
+	    # XFAIL it which will give us an XPASS if the problem goes
+	    # away.
+	    setup_xfail "alpha*-*-osf*"
 	    fail "$description"
 	}
-	timeout {
-	    fail "$description (timeout)"
-	}
     }
     
-    # get a stack trace
+    gdb_test "signal SIGINT" "Continuing with signal SIGINT.*" \
+	     "send SIGINT signal to child process"
+    
+    # get a stack trace being redelivered by readline
     #
     # This fails on some linux systems for unknown reasons.  On the
     # systems where it fails, sometimes it works fine when run manually.
     # The testsuite failures may not be limited to just aout systems.
+    # Optional system readline may not have symbols to be shown.
     setup_xfail "i*86-pc-linuxaout-gnu"
-    set description "backtrace through signal handler"
-    send_gdb "backtrace\n"
-    gdb_expect {
-	-re "#0.*(read|poll).*in main \\(.*\\) at .*gdb\\.c.*$gdb_prompt $" {
+    set description "backtrace through readline handler"
+    gdb_test_multiple "backtrace" $description {
+	-re "#0.*gdb_do_one_event.*in main \\(.*\\) at .*gdb\\.c.*$gdb_prompt $" {
 	    pass "$description"
 	}
 	-re ".*$gdb_prompt $" {
@@ -500,9 +510,6 @@ GDB.*Copyright \[0-9\]+ Free Software Foundation, Inc..*$gdb_prompt $"\
 	    setup_xfail "alpha*-*-osf*"
 	    fail "$description"
 	}
-	timeout {
-	    fail "$description (timeout)"
-	}
     }