[patch] Fix testsuite annotate-quit race (PR 544)

[patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

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

Hi,

PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)

It gets reliably reproducible for me (Fedora 8 x86_64) if GDB gets straced:
	mv gdb gdb-orig
	wget -O gdb http://cvs.jankratochvil.net/viewcvs/nethome/src/gdb-wrap?rev=HEAD

The problem is that if usually CTRL-C is hit during poll(2) and everything is
fine.  But if CTRL-C is hit inside readline processing its RL_SIGNAL_HANDLER
gets called which calls RL_FREE_LINE_STATE and $gdb_prompt gets displayed twice
and GDB_EXPECT or GDB_EXPECT_LIST are not patient enough to see later
^Z^Zerror-begin+Quit+^Z^Zquit.

Unaware how to easily detect a failure without timing out.


Regards,
Jan

[-- Attachment #2: gdb-cvs-annota23-quit.patch --]
[-- Type: text/plain, Size: 1584 bytes --]

2008-03-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR 544
	* gdb.cp/annota2.exp, gdb.cp/annota3.exp: Fix a race while recognizing
	`annotate-quit'.

--- ./gdb/testsuite/gdb.cp/annota2.exp	28 Feb 2008 16:24:25 -0000	1.10
+++ ./gdb/testsuite/gdb.cp/annota2.exp	18 Mar 2008 22:41:28 -0000
@@ -205,14 +205,13 @@ gdb_expect {
 # test:
 # annotate-quit
 #
-# This test sometimes fails, but not reproducibly.  See gdb/544.
+# We may get "$gdb_prompt$" earlier as it may get printed several times during
+# CTRL-C if we catch its output early enough.
 #
 send_gdb "\003"
 gdb_expect {
     -re "\r\n\032\032error-begin\r\nQuit\r\n\r\n\032\032quit\r\n$gdb_prompt$" \
 	    { pass "annotate-quit" }
-    -re "$gdb_prompt$" { kfail "gdb/544" "annotate-quit" }
-    -re ".*$gdb_prompt$" { fail "annotate-quit" }
     timeout { fail "annotate-quit (timeout)" }
 }
 
--- ./gdb/testsuite/gdb.cp/annota3.exp	1 Jan 2008 22:53:19 -0000	1.11
+++ ./gdb/testsuite/gdb.cp/annota3.exp	18 Mar 2008 22:41:28 -0000
@@ -208,13 +208,14 @@ gdb_expect {
 # test:
 # annotate-quit
 #
-# This test sometimes fails, but not reproducibly.  See gdb/544.
+# We may get "$gdb_prompt$" earlier as it may get printed several times during
+# CTRL-C if we catch its output early enough.
 #
 send_gdb "\003"
-gdb_expect_list "annotate-quit" "$gdb_prompt$" {
-    "\r\n\032\032error-begin\r\n"
-    "Quit\r\n"
-    "\r\n\032\032quit\r\n"
+gdb_expect {
+    -re "\r\n\032\032error-begin\r\nQuit\r\n\r\n\032\032quit\r\n$gdb_prompt$" \
+	    { pass "annotate-quit" }
+    timeout { fail "annotate-quit (timeout)" }
 }
 
 #

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Tue, Mar 18, 2008 at 11:54:36PM +0100, Jan Kratochvil wrote:
> Hi,
> 
> PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
> 	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)
> 
> It gets reliably reproducible for me (Fedora 8 x86_64) if GDB gets straced:
> 	mv gdb gdb-orig
> 	wget -O gdb http://cvs.jankratochvil.net/viewcvs/nethome/src/gdb-wrap?rev=HEAD
> 
> The problem is that if usually CTRL-C is hit during poll(2) and everything is
> fine.  But if CTRL-C is hit inside readline processing its RL_SIGNAL_HANDLER
> gets called which calls RL_FREE_LINE_STATE and $gdb_prompt gets displayed twice
> and GDB_EXPECT or GDB_EXPECT_LIST are not patient enough to see later
> ^Z^Zerror-begin+Quit+^Z^Zquit.

Isn't this a bug in GDB, not a bug in the test?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

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

Hi,

original post:
  http://sourceware.org/ml/gdb-patches/2008-03/msg00262.html

On Wed, 19 Mar 2008 00:17:38 +0100, Daniel Jacobowitz wrote:
> On Tue, Mar 18, 2008 at 11:54:36PM +0100, Jan Kratochvil wrote:
> > Hi,
> > 
> > PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
> > 	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)
...
> Isn't this a bug in GDB, not a bug in the test?

You are right it is probably better to fix it in readline.
There are two excessive GDB prompts displayed as seen in strace:

write(1, "\n\32\32pre-prompt\n(gdb) \n\32\32prompt\n", 30) = 30
--- SIGINT (Interrupt) @ 0 (0) ---
write(1, "\n\32\32pre-prompt\n(gdb) \n\32\32prompt\n\n\32\32pre-prompt\n(gdb) \n\32\32prompt\n", 60) = 60

One can put `sleep (1)' at the end of _RL_OUTPUT_SOME_CHARS and type
	p 1<enter><ctrl-c>
to abort the prompt printing. Before the patch:
	(gdb) p 1
	$1 = 1
	Quit) (gdb) 
	(gdb) _
After the patch:
	[bash]jkratoch@host0.dyn.jankratochvil.net:/home/jkratoch/redhat/sources/readline# ../gdb/gdb -nx -silent(gdb) p 1
	$1 = 1
	(gdb) Quit
	(gdb) _

There are no testsuite regressions on Fedora 8 x86_64.
The problem reproduced the same on readline-5.2.
There will be needed a GDB patch to close PR 544.


Regards,
Jan

[-- Attachment #2: readline-display-blockint.patch --]
[-- Type: text/plain, Size: 1735 bytes --]

--- readline/display.c	5 May 2006 18:26:12 -0000	1.11
+++ readline/display.c	19 Mar 2008 05:10:16 -0000
@@ -463,6 +463,10 @@ rl_redisplay ()
   if (!readline_echoing_p)
     return;
 
+  /* Signals are blocked through this function as the global data structures
+     could get corrupted upon modifications from an invoked signal handler. */
+  block_sigint ();
+
   if (!rl_display_prompt)
     rl_display_prompt = "";
 
@@ -1139,6 +1143,8 @@ rl_redisplay ()
     else
       visible_wrap_offset = wrap_offset;
   }
+
+  release_sigint ();
 }
 
 /* PWP: update_line() is based on finding the middle difference of each
--- readline/rltty.c	5 May 2006 18:26:12 -0000	1.9
+++ readline/rltty.c	19 Mar 2008 05:10:16 -0000
@@ -52,8 +52,8 @@ extern int errno;
 rl_vintfunc_t *rl_prep_term_function = rl_prep_terminal;
 rl_voidfunc_t *rl_deprep_term_function = rl_deprep_terminal;
 
-static void block_sigint PARAMS((void));
-static void release_sigint PARAMS((void));
+void block_sigint PARAMS((void));
+void release_sigint PARAMS((void));
 
 static void set_winsize PARAMS((int));
 
@@ -75,7 +75,7 @@ static int sigint_blocked;
 
 /* Cause SIGINT to not be delivered until the corresponding call to
    release_sigint(). */
-static void
+void
 block_sigint ()
 {
   if (sigint_blocked)
@@ -100,7 +100,7 @@ block_sigint ()
 }
 
 /* Allow SIGINT to be delivered. */
-static void
+void
 release_sigint ()
 {
   if (sigint_blocked == 0)
--- readline/rltty.h	5 May 2006 18:26:12 -0000	1.5
+++ readline/rltty.h	19 Mar 2008 05:10:16 -0000
@@ -79,4 +79,7 @@ typedef struct _rl_tty_chars {
   unsigned char t_status;
 } _RL_TTY_CHARS;
 
+extern void block_sigint PARAMS((void));
+extern void release_sigint PARAMS((void));
+
 #endif /* _RLTTY_H_ */

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Nick Roberts]

 > > > PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
 > > > 	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)
 > ...
 > > Isn't this a bug in GDB, not a bug in the test?
 > 
 > You are right it is probably better to fix it in readline.

Annotations are being deprecated and the original bug report is nearly six
years old.  As as variation of the old adage, I would suggest that "Since
no-one appears to mind it's broke, don't fix it".  The danger of making a fix
in readline, of course, is that you break annotations elsewhere, where it's
more important.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

On Wed, 19 Mar 2008 09:46:25 +0100, Nick Roberts wrote:
>  > > > PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
>  > > > 	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)
>  > ...
>  > > Isn't this a bug in GDB, not a bug in the test?
>  > 
>  > You are right it is probably better to fix it in readline.
> 
> Annotations are being deprecated and the original bug report is nearly six
> years old.  As as variation of the old adage, I would suggest that "Since
> no-one appears to mind it's broke, don't fix it".  The danger of making a fix
> in readline, of course, is that you break annotations elsewhere, where it's
> more important.

* I still face testsuite results flutter making the day-to-day regressions
  evaluations expensive.  Most of the flutters are fixed in gdb-6.8 but still
  some of them remain, this fix is for one of them.

  * I do not care much if it FAILs or PASSes but it must not flutter.

* I found it is a known PR 544 only after I fixed it.

* I fixed it as it was a nuisance reported by the courtesy of Roland McGrath.

* As the SIGINT blocking in RL_REDISPLAY fixes the race (1 of 10 cases to 0 of
  350 cases during my test) I find it a proof there must be some race.
  In fact if you check that RL_REDISPLAY modifies a lot of global variables
  being tangled by SIGINT-handler-called RL_FREE_LINE_STATE it is clear some
  locking is missing there.

* Sure I am fine to keep the patch in the local branch.


Best Regards,
Jan

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Nick Roberts]

 > * I still face testsuite results flutter making the day-to-day regressions
 >   evaluations expensive.  Most of the flutters are fixed in gdb-6.8 but still
 >   some of them remain, this fix is for one of them.
 > 
 >   * I do not care much if it FAILs or PASSes but it must not flutter.
 > 
 > * I found it is a known PR 544 only after I fixed it.
 > 
 > * I fixed it as it was a nuisance reported by the courtesy of Roland McGrath.
 > 
 > * As the SIGINT blocking in RL_REDISPLAY fixes the race (1 of 10 cases to 0 of
 >   350 cases during my test) I find it a proof there must be some race.
 >   In fact if you check that RL_REDISPLAY modifies a lot of global variables
 >   being tangled by SIGINT-handler-called RL_FREE_LINE_STATE it is clear some
 >   locking is missing there.
 > 
 > * Sure I am fine to keep the patch in the local branch.

I find it to resolve such matters easier by talking person to person than
communicating by e-mail, but here goes:

You say "I am fine to keep the patch in the local branch." but from the
preceding points it sounds anything but fine.  I don't have the understanding
of readline to make a judgement on your patch but I am just worried it will
break something for Emacs just as a previous change that you and Daniel made to
readline did.

It appear to me that the only problem this FAIL is causing is with the actual
running of the testsuite.  I would be quite happy if this test was just
removed.  Would that work for everyone else?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

On Wed, 19 Mar 2008 10:55:39 +0100, Nick Roberts wrote:
> I am just worried it will break something for Emacs just as a previous change
> that you and Daniel made to readline did.

http://sourceware.org/ml/gdb-patches/2006-11/threads.html#00234
http://sourceware.org/ml/gdb-patches/2006-12/threads.html#00009


> It appear to me that the only problem this FAIL is causing is with the actual
> running of the testsuite.  I would be quite happy if this test was just
> removed.  Would that work for everyone else?

The problem is unrelated to the annotations and there should be a testcase just
with the "(gdb) " prompt after the annotations support gets obsoleted/removed.

Currently the test described at
	http://sourceware.org/ml/gdb-patches/2008-03/msg00270.html
has different output depending on whether you type CTRL-C early enough (during
the "(gdb) " printing) or just later (in poll(2) waiting for input).

With the readline patch proposed there the output is always the same.

I find it a bug as without the artifical `sleep (1)' delay it is very racy.


> but I am just worried it will break something for Emacs just as a previous
> change that you and Daniel made to readline did.

My original patch requiring a readline change AFAIK did not break anything,
unfortunately it did not get accepted for readline.  Daniel created an IMO
complicated patch to workaround the readline problem without forking readline
but unfortunately it brought that regression.  I find the regression was in
fact only a consequence of the readline interface deficiency.


Regards,
Jan

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

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

On Wed, 19 Mar 2008 09:10:56 +0100, Jan Kratochvil wrote:
> On Wed, 19 Mar 2008 00:17:38 +0100, Daniel Jacobowitz wrote:
> > On Tue, Mar 18, 2008 at 11:54:36PM +0100, Jan Kratochvil wrote:
> > > Hi,
> > > 
> > > PR 544 des gdb.cp/annota2.exp and gdb.cp/annota3.exp sometimes FAIL with:
> > > 	FAIL: gdb.cp/annota3.exp: annotate-quit (pattern 1)
> ...
> > Isn't this a bug in GDB, not a bug in the test?
> 
> You are right it is probably better to fix it in readline.

The patch was approved by the readline maintainer:

On Fri, 21 Mar 2008 19:37:31 +0100, Chet Ramey wrote:
> I will add something like your block_sigint/release_sigint changes around
> the guts of rl_redisplay.  That's the right thing to do anyway.  It will
> probably not come out as a patch for readline-5.2; you can use your
> current patch (though the names will change to _rl_block_sigint and
> _rl_release_sigint -- fair warning).

It could be probably also coded only in GDB by modifying the function pointer
RL_REDISPLAY_FUNCTION instead to a custom wrapper calling RL_REDISPLAY but it
would need the SIGINT blocking/handling done in GDB.

OK to commit as a readline fork before readline-6 is here?


Regards,
Jan

[-- Attachment #2: readline-display-blockint-v2.patch --]
[-- Type: text/plain, Size: 2043 bytes --]

2008-03-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/544
	* display.c (rl_redisplay): Wrap the function by the calls to
	BLOCK_SIGINT and RELEASE_SIGINT.
	* rltty.c (block_sigint, release_sigint): Make the functions global.
	* rltty.h (block_sigint, release_sigint): New prototypes.

--- ./readline/display.c	5 May 2006 18:26:12 -0000	1.11
+++ ./readline/display.c	19 Mar 2008 05:10:16 -0000
@@ -463,6 +463,10 @@ rl_redisplay ()
   if (!readline_echoing_p)
     return;
 
+  /* Signals are blocked through this function as the global data structures
+     could get corrupted upon modifications from an invoked signal handler. */
+  block_sigint ();
+
   if (!rl_display_prompt)
     rl_display_prompt = "";
 
@@ -1139,6 +1143,8 @@ rl_redisplay ()
     else
       visible_wrap_offset = wrap_offset;
   }
+
+  release_sigint ();
 }
 
 /* PWP: update_line() is based on finding the middle difference of each
--- ./readline/rltty.c	5 May 2006 18:26:12 -0000	1.9
+++ ./readline/rltty.c	19 Mar 2008 05:10:16 -0000
@@ -52,8 +52,8 @@ extern int errno;
 rl_vintfunc_t *rl_prep_term_function = rl_prep_terminal;
 rl_voidfunc_t *rl_deprep_term_function = rl_deprep_terminal;
 
-static void block_sigint PARAMS((void));
-static void release_sigint PARAMS((void));
+void block_sigint PARAMS((void));
+void release_sigint PARAMS((void));
 
 static void set_winsize PARAMS((int));
 
@@ -75,7 +75,7 @@ static int sigint_blocked;
 
 /* Cause SIGINT to not be delivered until the corresponding call to
    release_sigint(). */
-static void
+void
 block_sigint ()
 {
   if (sigint_blocked)
@@ -100,7 +100,7 @@ block_sigint ()
 }
 
 /* Allow SIGINT to be delivered. */
-static void
+void
 release_sigint ()
 {
   if (sigint_blocked == 0)
--- ./readline/rltty.h	5 May 2006 18:26:12 -0000	1.5
+++ ./readline/rltty.h	19 Mar 2008 05:10:16 -0000
@@ -79,4 +79,7 @@ typedef struct _rl_tty_chars {
   unsigned char t_status;
 } _RL_TTY_CHARS;
 
+extern void block_sigint PARAMS((void));
+extern void release_sigint PARAMS((void));
+
 #endif /* _RLTTY_H_ */

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Fri, Mar 21, 2008 at 09:45:06PM +0100, Jan Kratochvil wrote:
> The patch was approved by the readline maintainer:

Great!  Thank you for pursuing this.

> On Fri, 21 Mar 2008 19:37:31 +0100, Chet Ramey wrote:
> > I will add something like your block_sigint/release_sigint changes around
> > the guts of rl_redisplay.  That's the right thing to do anyway.  It will
> > probably not come out as a patch for readline-5.2; you can use your
> > current patch (though the names will change to _rl_block_sigint and
> > _rl_release_sigint -- fair warning).
> 
> It could be probably also coded only in GDB by modifying the function pointer
> RL_REDISPLAY_FUNCTION instead to a custom wrapper calling RL_REDISPLAY but it
> would need the SIGINT blocking/handling done in GDB.
> 
> OK to commit as a readline fork before readline-6 is here?

We have a --with-system-readline option, which many distributors use
instead of the internal readline.  So I would rather fix it in GDB.
I did it just the way you suggested.

I'm running some stress tests with this patch.  So far it hasn't
failed once.

-- 
Daniel Jacobowitz
CodeSourcery

2008-03-21  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/544
	Suggested by Jan Kratochvil:
	* top.c (gdb_rl_operate_and_get_next_completion): Call
	rl_redisplay_function.
	(gdb_rl_redisplay): New.
	(init_main): Set rl_redisplay_function.

Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.135
diff -u -p -r1.135 top.c
--- top.c	21 Mar 2008 14:39:22 -0000	1.135
+++ top.c	21 Mar 2008 21:01:37 -0000
@@ -921,7 +921,7 @@ gdb_rl_operate_and_get_next_completion (
   operate_saved_history = -1;
 
   /* readline doesn't automatically update the display for us.  */
-  rl_redisplay ();
+  rl_redisplay_function ();
 
   after_char_processing_hook = NULL;
   rl_pre_input_hook = NULL;
@@ -956,6 +956,29 @@ gdb_rl_operate_and_get_next (int count, 
 
   return rl_newline (1, key);
 }
+
+/* Readline 5.2 and earlier do not block SIGINT while redrawing the prompt.
+   This can lead to corrupted internal state.  As long as we do not require
+   a newer readline version, compensate for it.  */
+static void
+gdb_rl_redisplay (void)
+{
+#if HAVE_SIGPROCMASK
+  sigset_t sigint_set, sigint_oset;
+
+  sigemptyset (&sigint_set);
+  sigemptyset (&sigint_oset);
+  sigaddset (&sigint_set, SIGINT);
+  sigprocmask (SIG_BLOCK, &sigint_set, &sigint_oset);
+#endif
+
+  rl_redisplay ();
+
+#if HAVE_SIGPROCMASK
+  sigprocmask (SIG_SETMASK, &sigint_oset, (sigset_t *)NULL);
+#endif
+}
+
 \f
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
@@ -1581,6 +1604,7 @@ init_main (void)
   rl_completer_quote_characters = get_gdb_completer_quote_characters ();
   rl_readline_name = "gdb";
   rl_terminal_name = getenv ("TERM");
+  rl_redisplay_function = gdb_rl_redisplay;
 
   /* The name for this defun comes from Bash, where it originated.
      15 is Control-o, the same binding this function has in Bash.  */

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Fri, Mar 21, 2008 at 05:03:24PM -0400, Daniel Jacobowitz wrote:
> We have a --with-system-readline option, which many distributors use
> instead of the internal readline.  So I would rather fix it in GDB.
> I did it just the way you suggested.
> 
> I'm running some stress tests with this patch.  So far it hasn't
> failed once.

Checked in as so.

-- 
Daniel Jacobowitz
CodeSourcery

2008-03-21  Daniel Jacobowitz  <dan@codesourcery.com>

	PR gdb/544
	Suggested by Jan Kratochvil:
	* top.c (gdb_rl_operate_and_get_next_completion): Call
	rl_redisplay_function.
	(gdb_rl_redisplay): New.
	(init_main): Set rl_redisplay_function.

2008-03-21  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.cp/annota2.exp, gdb.cp/annota3.exp: Remove KFAIL for
	fixed PR gdb/544.

Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.135
diff -u -p -r1.135 top.c
--- top.c	21 Mar 2008 14:39:22 -0000	1.135
+++ top.c	21 Mar 2008 21:01:37 -0000
@@ -921,7 +921,7 @@ gdb_rl_operate_and_get_next_completion (
   operate_saved_history = -1;
 
   /* readline doesn't automatically update the display for us.  */
-  rl_redisplay ();
+  rl_redisplay_function ();
 
   after_char_processing_hook = NULL;
   rl_pre_input_hook = NULL;
@@ -956,6 +956,29 @@ gdb_rl_operate_and_get_next (int count, 
 
   return rl_newline (1, key);
 }
+
+/* Readline 5.2 and earlier do not block SIGINT while redrawing the prompt.
+   This can lead to corrupted internal state.  As long as we do not require
+   a newer readline version, compensate for it.  */
+static void
+gdb_rl_redisplay (void)
+{
+#if HAVE_SIGPROCMASK
+  sigset_t sigint_set, sigint_oset;
+
+  sigemptyset (&sigint_set);
+  sigemptyset (&sigint_oset);
+  sigaddset (&sigint_set, SIGINT);
+  sigprocmask (SIG_BLOCK, &sigint_set, &sigint_oset);
+#endif
+
+  rl_redisplay ();
+
+#if HAVE_SIGPROCMASK
+  sigprocmask (SIG_SETMASK, &sigint_oset, (sigset_t *)NULL);
+#endif
+}
+
 \f
 /* Read one line from the command input stream `instream'
    into the local static buffer `linebuffer' (whose current length
@@ -1581,6 +1604,7 @@ init_main (void)
   rl_completer_quote_characters = get_gdb_completer_quote_characters ();
   rl_readline_name = "gdb";
   rl_terminal_name = getenv ("TERM");
+  rl_redisplay_function = gdb_rl_redisplay;
 
   /* The name for this defun comes from Bash, where it originated.
      15 is Control-o, the same binding this function has in Bash.  */
Index: testsuite/gdb.cp/annota2.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/annota2.exp,v
retrieving revision 1.10
diff -u -p -r1.10 annota2.exp
--- testsuite/gdb.cp/annota2.exp	28 Feb 2008 16:24:25 -0000	1.10
+++ testsuite/gdb.cp/annota2.exp	21 Mar 2008 21:43:11 -0000
@@ -205,13 +205,10 @@ gdb_expect {
 # test:
 # annotate-quit
 #
-# This test sometimes fails, but not reproducibly.  See gdb/544.
-#
 send_gdb "\003"
 gdb_expect {
     -re "\r\n\032\032error-begin\r\nQuit\r\n\r\n\032\032quit\r\n$gdb_prompt$" \
 	    { pass "annotate-quit" }
-    -re "$gdb_prompt$" { kfail "gdb/544" "annotate-quit" }
     -re ".*$gdb_prompt$" { fail "annotate-quit" }
     timeout { fail "annotate-quit (timeout)" }
 }
Index: testsuite/gdb.cp/annota3.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/annota3.exp,v
retrieving revision 1.11
diff -u -p -r1.11 annota3.exp
--- testsuite/gdb.cp/annota3.exp	1 Jan 2008 22:53:19 -0000	1.11
+++ testsuite/gdb.cp/annota3.exp	21 Mar 2008 21:43:11 -0000
@@ -208,8 +208,6 @@ gdb_expect {
 # test:
 # annotate-quit
 #
-# This test sometimes fails, but not reproducibly.  See gdb/544.
-#
 send_gdb "\003"
 gdb_expect_list "annotate-quit" "$gdb_prompt$" {
     "\r\n\032\032error-begin\r\n"

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Nick Roberts]

 > > I'm running some stress tests with this patch.  So far it hasn't
 > > failed once.
 > 
 > Checked in as so.
 > 
 > -- 
 > Daniel Jacobowitz
 > CodeSourcery
 > 
 > 2008-03-21  Daniel Jacobowitz  <dan@codesourcery.com>
 > 
 > 	PR gdb/544
 > 	Suggested by Jan Kratochvil:
 > 	* top.c (gdb_rl_operate_and_get_next_completion): Call
 > 	rl_redisplay_function.
 > 	(gdb_rl_redisplay): New.
 > 	(init_main): Set rl_redisplay_function.

This change breaks things for Emacs (I have this strange feeling of deja
vu...).

Did your stress tests include running Gdb on a dumb terminal?

I haven't the faintest idea what the problem is and everything seems fine on
an xterm:

nickrob@kahikatea:~$ src-head/build/gdb/gdb -ann=3 myprog
GNU gdb 6.8.50.20080323-cvs
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu"...

^Z^Zpre-prompt
(gdb) 
^Z^Zprompt

but if I do the same from within Emacs, the annotations don't appear.

If you've not got Emacs 22.1, you can see it in an earlier version by doing
M-x shell and "gdb -ann=3" in the *shell* buffer.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Sun, Mar 23, 2008 at 11:00:27PM +1200, Nick Roberts wrote:
> This change breaks things for Emacs (I have this strange feeling of deja
> vu...).
> 
> Did your stress tests include running Gdb on a dumb terminal?

I don't know how to test that.  Can it be added to the testsuite
somehow?

I tried running gdb under nohup and it worked fine.  I tried it in
Emacs too, and that worked fine:

drow@caradoc:/space/fsf/commit/src/gdb% /space/fsf/x86-64/commit-gdb/gdb/gdb -ann=3
/space/fsf/x86-64/commit-gdb/gdb/gdb -ann=3
GNU gdb 6.8.50.20080321-cvs
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".

^Z^Zpre-prompt
(gdb)
^Z^Zprompt

Does reverting the patch fix the problem?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

On Sun, 23 Mar 2008 17:30:27 +0100, Daniel Jacobowitz wrote:
...
> I tried running gdb under nohup and it worked fine.  I tried it in
> Emacs too, and that worked fine:

Confirmed the patch broke emacs-22.1-8.fc8.x86_64 (M-x shell ...).  readline
checks for custom RL_REDISPLAY_FUNCTION and behaves differently in such case.
Sorry for suggesting RL_REDISPLAY_FUNCTION - I did not verify it more.
Now just to find out how to supply GDB_RL_REDISPLAY simulating the original
behavior.

latest GDB:
rt_sigaction(SIGWINCH, {0x5f9025, [], SA_RESTORER|SA_RESTART, 0x3b98630f30}, {0x4ab21a, [], SA_RESTORER, 0x3b98630f30}, 8) = 0
rt_sigprocmask(SIG_BLOCK, [INT], [CHLD], 8) = 0
rt_sigprocmask(SIG_SETMASK, [CHLD], NULL, 8) = 0
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
poll([{fd=3, events=POLLIN}, {fd=0, events=POLLIN, revents=POLLIN}], 2, -1) = 1
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
read(0, "q", 1)                         = 1

reverted patch:
rt_sigaction(SIGWINCH, {0x5f8fb5, [], SA_RESTORER|SA_RESTART, 0x3b98630f30}, {0x4ab1aa, [], SA_RESTORER, 0x3b98630f30}, 8) = 0
write(1, "\n\32\32pre-prompt\n(gdb) \n\32\32prompt\n", 30) = 30
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
poll([{fd=3, events=POLLIN}, {fd=0, events=POLLIN, revents=POLLIN}], 2, -1) = 1
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
rt_sigprocmask(SIG_BLOCK, NULL, [CHLD], 8) = 0
read(0, "q", 1)                         = 1


> Does reverting the patch fix the problem?

Yes.


Regards,
Jan

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Sun, Mar 23, 2008 at 05:43:31PM +0100, Jan Kratochvil wrote:
> On Sun, 23 Mar 2008 17:30:27 +0100, Daniel Jacobowitz wrote:
> ...
> > I tried running gdb under nohup and it worked fine.  I tried it in
> > Emacs too, and that worked fine:
> 
> Confirmed the patch broke emacs-22.1-8.fc8.x86_64 (M-x shell ...).  readline
> checks for custom RL_REDISPLAY_FUNCTION and behaves differently in such case.
> Sorry for suggesting RL_REDISPLAY_FUNCTION - I did not verify it more.
> Now just to find out how to supply GDB_RL_REDISPLAY simulating the original
> behavior.

Weird, it didn't break in my Debian emacs 22.1+1-3.  I don't
understand the difference.

Anyway, since the patch is clearly at fault I have reverted it.
Nick, Jan, thank you.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Nick Roberts]

 > Weird, it didn't break in my Debian emacs 22.1+1-3.  I don't
 > understand the difference.

It appears the fact that Emacs uses a dumb terminal was a dumb statement,
or at least a red herring.  I think the cause of the problem is just that
terminals in Emacs don't echo input characters (at least on my (compiled
from CVS) and probably Jan's version:

nickrob@kahikatea:~$ stty -a
speed 38400 baud; rows 0; columns 0; line = 0;
intr = ^C; quit = ^\; erase = <undef>; kill = <undef>; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-parenb -parodd cs8 -hupcl -cstopb cread -clocal -crtscts
-ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl ixon -ixoff
-iuclc -ixany -imaxbel -iutf8
opost -olcuc -ocrnl -onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
isig icanon iexten -echo echoe echok -echonl -noflsh -xcase -tostop -echoprt
echoctl echoke     ^^^^^

 > Anyway, since the patch is clearly at fault I have reverted it.
 > Nick, Jan, thank you.

With this setting, for some reason this patch means that Gdb doesn't display
the prompt but everything else gets displayed:


nickrob@kahikatea:~$ src-head/build/gdb/gdb
GNU gdb 6.8.50.20080323-cvs
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
pwd
Working directory /home/nickrob.
path
Executable and object file path: .:/home/nickrob/bin/:/home/nickrob/bin:.:/home/nickrob/bin/:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
quit
nickrob@kahikatea:~$ 

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Mon, Mar 24, 2008 at 12:06:30PM +1200, Nick Roberts wrote:
>  > Anyway, since the patch is clearly at fault I have reverted it.
>  > Nick, Jan, thank you.
> 
> With this setting, for some reason this patch means that Gdb doesn't display
> the prompt but everything else gets displayed:

Thanks, that's very helpful.

What I think is happening is that rl_redisplay has some optimizations
which cause it not to redisplay.  And since we have a custom function
installed readline isn't calling the alternate hooks that force
display.  So rl_redisplay does not actually meet the interface
associated with rl_redisplay_function despite being its default
value.  Nasty.

We can work around that, probably by forcing redisplay and by
accepting slightly less optimized redraw, but it seems a bit lame...

We could patch our local readline with Jan's patch, which would fix it
for anyone using the included readline binary.  Or just KFAIL the test
until we merge a new readline version.  Thoughts?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

On Mon, 24 Mar 2008 03:58:36 +0100, Daniel Jacobowitz wrote:
...
> We could patch our local readline with Jan's patch, which would fix it
> for anyone using the included readline binary.

It happened in Fedora (and thus fixing GDB there):
	http://cvs.fedora.redhat.com/viewcvs/*checkout*/rpms/readline/devel/readline-5.2-redisplay-sigint.patch

> Or just KFAIL the test until we merge a new readline version.  Thoughts?

In fact I do not find the failing gdb.cp/annota[23].exp testcases as the main
problem now, besides also many other GDB testcases sending CTRL-C I saw people
are using GDB in automated ways from programs and this may (or may not) be
a reason causing them problems.


Regards,
Jan

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Mon, Mar 24, 2008 at 08:41:58AM +0100, Jan Kratochvil wrote:
> On Mon, 24 Mar 2008 03:58:36 +0100, Daniel Jacobowitz wrote:
> ...
> > We could patch our local readline with Jan's patch, which would fix it
> > for anyone using the included readline binary.
> 
> It happened in Fedora (and thus fixing GDB there):
> 	http://cvs.fedora.redhat.com/viewcvs/*checkout*/rpms/readline/devel/readline-5.2-redisplay-sigint.patch

OK.  Could you apply it to our local readline and ChangeLog.gdb,
please?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Jan Kratochvil]

On Mon, 24 Mar 2008 13:00:20 +0100, Daniel Jacobowitz wrote:
> On Mon, Mar 24, 2008 at 08:41:58AM +0100, Jan Kratochvil wrote:
> > On Mon, 24 Mar 2008 03:58:36 +0100, Daniel Jacobowitz wrote:
> > ...
> > > We could patch our local readline with Jan's patch, which would fix it
> > > for anyone using the included readline binary.
> > 
> > It happened in Fedora (and thus fixing GDB there):
> > 	http://cvs.fedora.redhat.com/viewcvs/*checkout*/rpms/readline/devel/readline-5.2-redisplay-sigint.patch
> 
> OK.  Could you apply it to our local readline and ChangeLog.gdb,
> please?

Committed.
	http://sourceware.org/ml/gdb-cvs/2008-03/msg00128.html


Thanks,
Jan

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Mon, Mar 24, 2008 at 02:01:51PM +0100, Jan Kratochvil wrote:
> Committed.
> 	http://sourceware.org/ml/gdb-cvs/2008-03/msg00128.html

Thanks.  I reapplied the testsuite changes too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Chet Ramey]

Daniel Jacobowitz wrote:

> What I think is happening is that rl_redisplay has some optimizations
> which cause it not to redisplay.  And since we have a custom function
> installed readline isn't calling the alternate hooks that force
> display.  So rl_redisplay does not actually meet the interface
> associated with rl_redisplay_function despite being its default
> value.  Nasty.

That's interesting, if true.  Can anyone add detail confirming this?

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Daniel Jacobowitz]

On Mon, Mar 24, 2008 at 09:01:30PM -0400, Chet Ramey wrote:
> Daniel Jacobowitz wrote:
>
>> What I think is happening is that rl_redisplay has some optimizations
>> which cause it not to redisplay.  And since we have a custom function
>> installed readline isn't calling the alternate hooks that force
>> display.  So rl_redisplay does not actually meet the interface
>> associated with rl_redisplay_function despite being its default
>> value.  Nasty.
>
> That's interesting, if true.  Can anyone add detail confirming this?

Sure.  There's a check "rl_redisplay_function == rl_redisplay" in the
readline source, and another in the definition of
CUSTOM_REDISPLAY_FUNCTION.  So that function is inherently special.

From what Nick and Jan have found, stty -echo and then running GDB
with a custom redisplay function that just calls rl_redisplay
will not display the GDB prompt.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix testsuite annotate-quit race (PR 544)

[Chet Ramey]

Daniel Jacobowitz wrote:
> On Mon, Mar 24, 2008 at 09:01:30PM -0400, Chet Ramey wrote:
>> Daniel Jacobowitz wrote:
>>
>>> What I think is happening is that rl_redisplay has some optimizations
>>> which cause it not to redisplay.  And since we have a custom function
>>> installed readline isn't calling the alternate hooks that force
>>> display.  So rl_redisplay does not actually meet the interface
>>> associated with rl_redisplay_function despite being its default
>>> value.  Nasty.
>> That's interesting, if true.  Can anyone add detail confirming this?
> 
> Sure.  There's a check "rl_redisplay_function == rl_redisplay" in the
> readline source, and another in the definition of
> CUSTOM_REDISPLAY_FUNCTION.  So that function is inherently special.
> 
> From what Nick and Jan have found, stty -echo and then running GDB
> with a custom redisplay function that just calls rl_redisplay
> will not display the GDB prompt.

You were pretty close to the answer; had you looked at the source
around the check you identified, you would have found it
(readline.c:readline_internal_setup()):

   /* If we're not echoing, we still want to at least print a prompt, because
      rl_redisplay will not do it for us.  If the calling application has a
      custom redisplay function, though, let that function handle it. */
   if (readline_echoing_p == 0 && rl_redisplay_function == rl_redisplay)
     {
       if (rl_prompt && rl_already_prompted == 0)
         {
           nprompt = _rl_strip_prompt (rl_prompt);
           fprintf (_rl_out_stream, "%s", nprompt);
           fflush (_rl_out_stream);
           free (nprompt);
         }
     }

You've turned off echo, and the redisplay code won't print anything in that
case.  Readline, when using its internal redisplay function, chooses to
print a prompt to at least visually clue the user that it's around.
Having accepted responsibility for handling redisplay, gdb can do whatever
it deems appropriate.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		       Live Strong.  No day but today.
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/