Re: [PATCH] Don't handle timeout inside gdb_test_multiple

[Pedro Alves <palves at redhat dot com>]

On 12/02/2016 03:24 PM, Luis Machado wrote:
> On 12/02/2016 05:04 AM, Pedro Alves wrote:
>> On 12/02/2016 01:07 AM, Luis Machado wrote:
>>> This fixes a few cases where the testcase is explicitly handling
>>> timeouts
>>> inside gdb_test_multiple when it is not necessary.
>>>
>>> It also converts two gdb_test_multiple calls to gdb_test_no_output calls
>>> (also removing the timeout handling).
>>>
>>> gdb/testsuite/ChangeLog:
>>>
>>> 2016-12-01  Luis Machado  <lgustavo@codesourcery.com>
>>>
>>>     * gdb.base/maint.exp: Remove timeout handling for gdb_test_multiple.
>>>     * gdb.cp/gdb2495.exp: Likewise and convert gdb_test_multiple into
>>>     gdb_test_no_output for a couple of cases.
>>>     * gdb.cp/ovldbreak.exp: Remove timeout handling for
>>> gdb_test_multiple.
>>> ---
>>>  gdb/testsuite/gdb.base/maint.exp   |  9 ---------
>>>  gdb/testsuite/gdb.cp/gdb2495.exp   | 10 ++--------
>>>  gdb/testsuite/gdb.cp/ovldbreak.exp |  4 ----
>>>  3 files changed, 2 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/gdb/testsuite/gdb.base/maint.exp
>>> b/gdb/testsuite/gdb.base/maint.exp
>>> index b07b370..17c606b 100644
>>> --- a/gdb/testsuite/gdb.base/maint.exp
>>> +++ b/gdb/testsuite/gdb.base/maint.exp
>>> @@ -297,9 +297,6 @@ gdb_test_multiple "maint print msymbols
>>> msymbols_output2 ${testfile}" "maint pri
>>>              -re ".*$gdb_prompt $" {
>>>                  fail "maint print msymbols"
>>>              }
>>> -            timeout {
>>> -                fail "(timeout) maint print msymbols"
>>> -            }
>>
>> This was:
>>
>>             gdb_test_multiple "shell grep factorial msymbols_output2"
>> "maint print msymbols" {
>>             -re "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex
>> \\.?factorial.*$gdb_prompt $" {
>>                 pass "maint print msymbols"
>>             }
>>             -re ".*$gdb_prompt $" {
>>                 fail "maint print msymbols"
>>             }
>>             timeout {
>>                 fail "(timeout) maint print msymbols"
>>             }
>>
>> gdb_test_multiple has an internal FAIL for "$gdb_prompt $" too.  So this
>> could be reduced to:
>>
>>             gdb_test_multiple "shell grep factorial msymbols_output2"
>> "maint print msymbols" {
>>             -re "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex
>> \\.?factorial.*$gdb_prompt $" {
>>                 pass "maint print msymbols"
>>             }
>>                 }
>>
>> which can then be reduced to:
>>
>>             gdb_test "shell grep factorial msymbols_output2" \
>>             "\\\[ *$decimal\\\] \[tT\]\[ \t\]+$hex \\.?factorial.*" \
>>                     "maint print msymbols"
>>
>>
>> Looks like a good chunk of the file could be similarly simplified.
>>
> 
> Heh... i did notice those things, but i tried to keep the patch focused.

Considering gdb_test_multiple->gdb_test folding in one step looks pretty
focused to me.  Just removing the timeout may even make it a bit harder
to find such cases later on, as the timeout handling is a good indicator
of old dusty code.

> 
> Actually, i noticed more tests using outdated constructs (gdb_expect)
> without the need to do so. That seemed like a slightly bigger change
> though (but would be nice!)

Yeah.

> 
> I can patch the above up with no problems though.
> 
>>> --- a/gdb/testsuite/gdb.cp/ovldbreak.exp
>>> +++ b/gdb/testsuite/gdb.cp/ovldbreak.exp
>>> @@ -58,10 +58,6 @@ proc take_gdb_out_of_choice_menu {} {
>>>      gdb_test_multiple " " " " {
>>>          -re ".*$gdb_prompt $" {
>>>          }
>>> -        timeout {
>>> -            perror "could not resynchronize to command prompt
>>> (timeout)"
>>> -            continue
>>> -        }
>>
>> Looking at both the description and callers of this procedure, I'd think
>> that the perror here was intended to abort the testcase instead of
>> getting
>> into a potentially long cascading timeout sequence.
> 
> This was a bit cryptic. It wasn't clear whether this was still
> functional or not. And it looks hack-ish.

It's always called when things are south already:

                timeout {
                    fail "set bp $bpnumber on $name $mychoice line $linenumber (timeout)"
                    take_gdb_out_of_choice_menu
                }

            timeout {
                fail "set bp on overload1arg canceled (timeout)"
                take_gdb_out_of_choice_menu
            }

    timeout {
        fail "bp menu for foo::overload1arg choice cancel (timeout)"
        take_gdb_out_of_choice_menu
    }

So if recovering once doesn't work, we just abort the testcase instead
of continuing as if GDB was responsive.

> 
> Should i put this back? Maybe investigate if we need that code at all?

I think we should put it back.  Just removing the timeout handling
from within take_gdb_out_of_choice_menu alone doesn't make sense to me.
Either we remove the whole thing entirely, or it should keep its
timeout handling.  IMO.  Whether this is the best way to avoid
cascading timeouts can be left for another day, of course.

Thanks,
Pedro Alves