diff mbox

Don't handle timeout inside gdb_test_multiple

Message ID 1480640845-3564-1-git-send-email-lgustavo@codesourcery.com
State New, archived
Headers

Commit Message

Luis Machado Dec. 2, 2016, 1:07 a.m. UTC 
  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(-)

Comments

Joel Brobecker Dec. 2, 2016, 2:42 a.m. UTC | #1 
> 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.

Thanks, Luis. This looks good to me, so go ahead and push (nice to
see these go!)
Luis Machado Dec. 2, 2016, 4:08 a.m. UTC | #2 
On 12/01/2016 08:42 PM, Joel Brobecker wrote:
>> 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.
>
> Thanks, Luis. This looks good to me, so go ahead and push (nice to
> see these go!)
>

Thanks Joel. Pushed now as 018572b88885ae67d22612937fa1e4fd98d5f5ad.
Pedro Alves Dec. 2, 2016, 11:04 a.m. UTC | #3 
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.

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

Thanks,
Pedro Alves
Luis Machado Dec. 2, 2016, 3:24 p.m. UTC | #4 
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.

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!)

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.

Should i put this back? Maybe investigate if we need that code at all?
Pedro Alves Dec. 2, 2016, 3:42 p.m. UTC | #5 
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
diff mbox

Patch

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"
-		    }
 		}
 		gdb_test "shell rm -f msymbols_output2" ".*" \
 		    "shell rm -f msymbols_output2"
@@ -307,17 +304,11 @@  gdb_test_multiple "maint print msymbols msymbols_output2 ${testfile}" "maint pri
 	    -re ".*$gdb_prompt $" {
 		fail "maint print msymbols"
 	    }
-	    timeout {
-	    	fail "(timeout) maint print msymbols"
-	    }
 	}
     }
     -re ".*$gdb_prompt $" {
 	fail "maint print msymbols"
     }
-    timeout {
-	fail "(timeout) maint print msymbols"
-    }
 }
 gdb_test "cd ${mydir}" \
     "Working directory [string_to_regexp ${mydir}]\..*" \
diff --git a/gdb/testsuite/gdb.cp/gdb2495.exp b/gdb/testsuite/gdb.cp/gdb2495.exp
index b4304fb..ac74382 100644
--- a/gdb/testsuite/gdb.cp/gdb2495.exp
+++ b/gdb/testsuite/gdb.cp/gdb2495.exp
@@ -117,10 +117,7 @@  if ![runto_main] then {
 # behaviour; it should not.  Test both on and off states.
 
 # Turn on unwind on signal behaviour.
-gdb_test_multiple "set unwindonsignal on" "turn unwindonsignal on" {
-    -re "$gdb_prompt $" {pass "set unwindonsignal on"}
-    timeout {fail "(timeout) set unwindonsignal on"}
-}
+gdb_test_no_output "set unwindonsignal on"
 
 # Check that it is turned on.
 gdb_test "show unwindonsignal" \
@@ -133,10 +130,7 @@  gdb_test "p exceptions.raise_signal(1)" \
     "To change this behavior use \"set unwindonsignal off\".*"
 
 # And reverse - turn off again.
-gdb_test_multiple "set unwindonsignal off" "turn unwindonsignal off" {
-    -re "$gdb_prompt $" {pass "set unwindonsignal off"}
-    timeout {fail "(timeout) set unwindonsignal off"}
-}
+gdb_test_no_output "set unwindonsignal off"
 
 # Check that it is actually turned off.
 gdb_test "show unwindonsignal" \
diff --git a/gdb/testsuite/gdb.cp/ovldbreak.exp b/gdb/testsuite/gdb.cp/ovldbreak.exp
index 96d3bd3..b6c5efd 100644
--- 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
-        }
     }
 }