Re: [PATCH] [gdb/testsuite] Fix gdb.tui/wrap-line.exp

[Andrew Burgess <aburgess@redhat.com>]

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

> On 5/21/23 10:51, Andrew Burgess wrote:
>> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> PR testsuite/30458 reports the following FAIL:
>>> ...
>>> PASS: gdb.tui/wrap-line.exp: width-auto-detected: cli: wrap
>>> ^CQuit
>>> (gdb) WARNING: timeout in accept_gdb_output
>>> Screen Dump (size 50 columns x 24 rows, cursor at column 6, row 3):
>>>      0 Quit
>>>      1 (gdb) 7890123456789012345678901234567890123456789
>>>      2 W^CQuit
>>>      3 (gdb)
>>>    ...
>>> FAIL: gdb.tui/wrap-line.exp: width-auto-detected: cli: prompt after wrap
>>> ...
>>>
>>> The problem is that the regexp doesn't account for the ^C:
>>> ...
>>>      gdb_assert { [Term::wait_for "^WQuit"] } "prompt after wrap"
>>> ...
>>>
>>> Fix this by updating the regexp, and likewise in another place in the
>>> test-case where we use ^C.
>> 
>> Do we know why we sometimes manage to see '^C'?  I guess it's a timing
>> thing, but right now I'm at a loss for how we manage to see it.  It
>> appears that we print the wrapping line, that ends with 'W', and then
>> wait for this to be displayed.
>> 
>> At this point GDB should be in a stable state waiting in the
>> event-loop.
>> 
>> When we send \003 this should trigger an event, which should trigger
>> async_request_quit, which should result in the 'Quit' exception being
>> thrown, caught, and printed.
>> 
>> I think the '^C' must be getting printed from tui_redisplay_readline
>> maybe, but I can't see how that gets triggered with \003 in the input
>> buffer.
>> 
>> I only ask because if we understand why '^C' is sometimes printed then
>> we might be able to decide if this should always be printed, or never be
>> printed, and change GDB accordingly...
>> 
>
> Hi Andrew,
>
> yes, that's a good question.
>
> [ Note that it's a TUI test-case, but the FAIL we're observing is in the 
> cli part, before activating TUI, so tui_redisplay_readline has nothing 
> to do with the FAIL. ]
>
> I've added an assert in rl_echo_signal_char and managed to trigger it to 
> generate a core file corresponding to the FAIL condition (more details 
> in the PR).
>
> My guess at what happens is the following:
> - we send a W to gdb
> - readline handles this, and echoes it
> - after readline echoing it, expect notices this and we send a ^C to gdb
> - at this point readline is still in the code handling the W, and
>    handles the ^C by echoing it.
>
> Usually, at this point we're already back in gdb and handle the ^C 
> without echoing it.

Thanks for the breakdown.  I agree with your assessment.  If I apply this
patch:

## START ##

diff --git a/readline/readline/readline.c b/readline/readline/readline.c
index 0e33587f234..e5825a0a9b0 100644
--- a/readline/readline/readline.c
+++ b/readline/readline/readline.c
@@ -678,6 +678,9 @@ readline_internal_charloop (void)
       else if (rl_mark_active_p ())
         rl_deactivate_mark ();
 
+      if (getenv ("RL_CHAR_DELAY") != NULL)
+	sleep (1);
+
       _rl_internal_char_cleanup ();
 
 #if defined (READLINE_CALLBACKS)

## END ##

Then run GDB with the RL_CHAR_DELAY environment variable set, it is now
possible to type a character and quickly hit Ctrl-C in order to always
see the '^C' displayed.

Given the following assumptions:

An application using readline in callback mode will spend most of its
time outside of the readline code, and will therefore mostly have its
own signal handlers installed.

And, the documentation for the readline function rl_echo_signal_char
says:

     If an application wishes to install its own signal handlers, but
     still have readline display characters that generate signals,
     calling this function with SIG set to 'SIGINT', 'SIGQUIT', or
     'SIGTSTP' will display the character generating that signal.

I wonder if the single call to 'rl_echo_signal_char' which can be found
in readline/readline/signals.c should be wrapped in an `#if` such that
this call is disabled when readline is used in callback mode?  Like this
patch:

## START ##

diff --git a/readline/readline/signals.c b/readline/readline/signals.c
index 8fedc370a1a..f10534c6872 100644
--- a/readline/readline/signals.c
+++ b/readline/readline/signals.c
@@ -271,7 +271,9 @@ _rl_handle_signal (int sig)
 	sigprocmask (SIG_BLOCK, &set, &oset);
 #endif
 
+#if !defined (READLINE_CALLBACKS)
       rl_echo_signal_char (sig);
+#endif
       rl_cleanup_after_signal ();
 
       /* At this point, the application's signal handler, if any, is the

## END ##

My reasoning would be that, when using in callback mode, it is up to the
application's signal handler to ensure that rl_echo_signal_char is
called if the application actually wants '^C' to be printed.  If must be
doing this or mostly '^C' would not (currently) be printed.

If we hit this race condition then the application is now going to print
a double '^C^C' string, which is also a bug.

And if the applications signal handler doesn't cause rl_echo_signal_char
to be called (like GDB) then it feels weird that in this one corner case
we do end up calling it.

In conclusion, I think I am arguing that what we have here is a readline
bug.

I'm happy to present this on the readline mailing list, but I wanted to
discuss this with you first -- to see if I've convinced you?

Thanks,
Andrew