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

[Tom de Vries <tdevries@suse.de>]

On 5/30/23 15:45, Andrew Burgess wrote:
> 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?

You did :)

The bit of documentation you quoted suggests to me that it's unintended 
behaviour, thanks for digging that up.

Thanks,
- Tom