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

[Andrew Burgess <aburgess@redhat.com>]

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

> 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.

Reported this issue on the readline list:

  https://lists.gnu.org/archive/html/bug-readline/2023-06/msg00000.html

Thanks,
Andrew