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

  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.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
---
 gdb/testsuite/gdb.tui/wrap-line.exp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)


base-commit: 0cc8cc5e6f82b8d3d8e3803c6f7f5e63f0c866ad
  

On 18/05/2023 08:10, Tom de Vries via Gdb-patches wrote:
> 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.
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
> ---

Hi Tom,

I can't seem to reproduce that bug, but this change also doesn't add any 
failures so it seems fine to me.

Tested-By: Bruno Larsen <blarsen@redhat.com>
  

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

Thanks,
Andrew

>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
> ---
>  gdb/testsuite/gdb.tui/wrap-line.exp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.tui/wrap-line.exp b/gdb/testsuite/gdb.tui/wrap-line.exp
> index 4587517504c..2bfe342e04d 100644
> --- a/gdb/testsuite/gdb.tui/wrap-line.exp
> +++ b/gdb/testsuite/gdb.tui/wrap-line.exp
> @@ -25,6 +25,8 @@ set cols 50
>  set lines 24
>  set dims [list $lines $cols]
>  
> +set re_control_c "(\\^C)?Quit"
> +
>  # Fill line, assuming we start after the gdb prompt.
>  proc fill_line { width } {
>      set res ""
> @@ -47,7 +49,7 @@ proc fill_line { width } {
>  proc test_wrap { wrap_width } {
>      # Generate a prompt and parse it.
>      send_gdb "\003"
> -    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )Quit"] } "start line"
> +    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )$::re_control_c"] } "start line"
>  
>      # Fill the line to just before wrapping.
>      set str [fill_line $wrap_width]
> @@ -64,7 +66,7 @@ proc test_wrap { wrap_width } {
>  
>      # Generate a prompt and parse it.
>      send_gdb "\003"
> -    gdb_assert { [Term::wait_for "^WQuit"] } "prompt after wrap"
> +    gdb_assert { [Term::wait_for "^W$::re_control_c"] } "prompt after wrap"
>  }
>  
>  # Test wrapping in both CLI and TUI.
>
> base-commit: 0cc8cc5e6f82b8d3d8e3803c6f7f5e63f0c866ad
> -- 
> 2.35.3
  

On Mai 21 2023, Andrew Burgess via Gdb-patches 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'?

Isn't that printed by the kernel (N_TTY ldisc)?
  

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,
- Tom

> Thanks,
> Andrew
> 
>>
>> Tested on x86_64-linux.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
>> ---
>>   gdb/testsuite/gdb.tui/wrap-line.exp | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.tui/wrap-line.exp b/gdb/testsuite/gdb.tui/wrap-line.exp
>> index 4587517504c..2bfe342e04d 100644
>> --- a/gdb/testsuite/gdb.tui/wrap-line.exp
>> +++ b/gdb/testsuite/gdb.tui/wrap-line.exp
>> @@ -25,6 +25,8 @@ set cols 50
>>   set lines 24
>>   set dims [list $lines $cols]
>>   
>> +set re_control_c "(\\^C)?Quit"
>> +
>>   # Fill line, assuming we start after the gdb prompt.
>>   proc fill_line { width } {
>>       set res ""
>> @@ -47,7 +49,7 @@ proc fill_line { width } {
>>   proc test_wrap { wrap_width } {
>>       # Generate a prompt and parse it.
>>       send_gdb "\003"
>> -    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )Quit"] } "start line"
>> +    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )$::re_control_c"] } "start line"
>>   
>>       # Fill the line to just before wrapping.
>>       set str [fill_line $wrap_width]
>> @@ -64,7 +66,7 @@ proc test_wrap { wrap_width } {
>>   
>>       # Generate a prompt and parse it.
>>       send_gdb "\003"
>> -    gdb_assert { [Term::wait_for "^WQuit"] } "prompt after wrap"
>> +    gdb_assert { [Term::wait_for "^W$::re_control_c"] } "prompt after wrap"
>>   }
>>   
>>   # Test wrapping in both CLI and TUI.
>>
>> base-commit: 0cc8cc5e6f82b8d3d8e3803c6f7f5e63f0c866ad
>> -- 
>> 2.35.3
>
  

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Mai 21 2023, Andrew Burgess via Gdb-patches 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'?
>
> Isn't that printed by the kernel (N_TTY ldisc)?

Not sure what the last part means.  There's for sure code in readline
which seems to be responsible for printing '^C' in some cases, so I
suspect the kernel doesn't always print this.  Maybe this kernel echoing
is turned off in our situation?

Thanks,
Andrew


>
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
  

On 5/21/23 18:48, Tom de Vries wrote:
> 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.
> 

Andrew, any comment?

Thanks,
- Tom

> Thanks,
> - Tom
> 
>> Thanks,
>> Andrew
>>
>>>
>>> Tested on x86_64-linux.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
>>> ---
>>>   gdb/testsuite/gdb.tui/wrap-line.exp | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/testsuite/gdb.tui/wrap-line.exp 
>>> b/gdb/testsuite/gdb.tui/wrap-line.exp
>>> index 4587517504c..2bfe342e04d 100644
>>> --- a/gdb/testsuite/gdb.tui/wrap-line.exp
>>> +++ b/gdb/testsuite/gdb.tui/wrap-line.exp
>>> @@ -25,6 +25,8 @@ set cols 50
>>>   set lines 24
>>>   set dims [list $lines $cols]
>>> +set re_control_c "(\\^C)?Quit"
>>> +
>>>   # Fill line, assuming we start after the gdb prompt.
>>>   proc fill_line { width } {
>>>       set res ""
>>> @@ -47,7 +49,7 @@ proc fill_line { width } {
>>>   proc test_wrap { wrap_width } {
>>>       # Generate a prompt and parse it.
>>>       send_gdb "\003"
>>> -    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )Quit"] } "start 
>>> line"
>>> +    gdb_assert { [Term::wait_for "(^|$::gdb_prompt 
>>> )$::re_control_c"] } "start line"
>>>       # Fill the line to just before wrapping.
>>>       set str [fill_line $wrap_width]
>>> @@ -64,7 +66,7 @@ proc test_wrap { wrap_width } {
>>>       # Generate a prompt and parse it.
>>>       send_gdb "\003"
>>> -    gdb_assert { [Term::wait_for "^WQuit"] } "prompt after wrap"
>>> +    gdb_assert { [Term::wait_for "^W$::re_control_c"] } "prompt 
>>> after wrap"
>>>   }
>>>   # Test wrapping in both CLI and TUI.
>>>
>>> base-commit: 0cc8cc5e6f82b8d3d8e3803c6f7f5e63f0c866ad
>>> -- 
>>> 2.35.3
>>
>
  

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

> On 5/21/23 18:48, Tom de Vries wrote:
>> 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.
>> 
>
> Andrew, any comment?

Sorry, lost track of this thread.  Will take a look today.

Thanks,
Andrew

>
> Thanks,
> - Tom
>
>> Thanks,
>> - Tom
>> 
>>> Thanks,
>>> Andrew
>>>
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30458
>>>> ---
>>>>   gdb/testsuite/gdb.tui/wrap-line.exp | 6 ++++--
>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/testsuite/gdb.tui/wrap-line.exp 
>>>> b/gdb/testsuite/gdb.tui/wrap-line.exp
>>>> index 4587517504c..2bfe342e04d 100644
>>>> --- a/gdb/testsuite/gdb.tui/wrap-line.exp
>>>> +++ b/gdb/testsuite/gdb.tui/wrap-line.exp
>>>> @@ -25,6 +25,8 @@ set cols 50
>>>>   set lines 24
>>>>   set dims [list $lines $cols]
>>>> +set re_control_c "(\\^C)?Quit"
>>>> +
>>>>   # Fill line, assuming we start after the gdb prompt.
>>>>   proc fill_line { width } {
>>>>       set res ""
>>>> @@ -47,7 +49,7 @@ proc fill_line { width } {
>>>>   proc test_wrap { wrap_width } {
>>>>       # Generate a prompt and parse it.
>>>>       send_gdb "\003"
>>>> -    gdb_assert { [Term::wait_for "(^|$::gdb_prompt )Quit"] } "start 
>>>> line"
>>>> +    gdb_assert { [Term::wait_for "(^|$::gdb_prompt 
>>>> )$::re_control_c"] } "start line"
>>>>       # Fill the line to just before wrapping.
>>>>       set str [fill_line $wrap_width]
>>>> @@ -64,7 +66,7 @@ proc test_wrap { wrap_width } {
>>>>       # Generate a prompt and parse it.
>>>>       send_gdb "\003"
>>>> -    gdb_assert { [Term::wait_for "^WQuit"] } "prompt after wrap"
>>>> +    gdb_assert { [Term::wait_for "^W$::re_control_c"] } "prompt 
>>>> after wrap"
>>>>   }
>>>>   # Test wrapping in both CLI and TUI.
>>>>
>>>> base-commit: 0cc8cc5e6f82b8d3d8e3803c6f7f5e63f0c866ad
>>>> -- 
>>>> 2.35.3
>>>
>>
  

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
  

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
  

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
  

On 6/1/23 13:12, Andrew Burgess via Gdb-patches wrote:
> Reported this issue on the readline list:
> 
>    https://lists.gnu.org/archive/html/bug-readline/2023-06/msg00000.html

I've updated the patch to mention this issue, and committed.

Thanks,
- Tom
  

On 6/21/23 16:19, Tom de Vries wrote:
> On 6/1/23 13:12, Andrew Burgess via Gdb-patches wrote:
>> Reported this issue on the readline list:
>>
>>    https://lists.gnu.org/archive/html/bug-readline/2023-06/msg00000.html
> 
> I've updated the patch to mention this issue, and committed.
> 

Also, I've filed a PR such that we can keep track of this issue from the 
gdb perspective ( https://sourceware.org/bugzilla/show_bug.cgi?id=30572 ).

Thanks,
- Tom