gdbserver-support: Handle gdbserver start failures

  On Tue, 29 Jul 2014, Pedro Alves wrote:

> On 07/28/2014 08:34 PM, Maciej W. Rozycki wrote:
> 
> > gdb-test-gdbserver-start-timeout.diff
> > Index: gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdbserver-support.exp
> > ===================================================================
> > --- gdb-fsf-trunk-quilt.orig/gdb/testsuite/lib/gdbserver-support.exp	2014-05-13 02:52:11.347706187 +0100
> > +++ gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdbserver-support.exp	2014-05-30 01:45:51.658977074 +0100
> > @@ -275,6 +275,7 @@ proc gdbserver_start { options arguments
> >  	# Wait for the server to open its TCP socket, so that GDB can connect.
> >  	expect {
> >  	    -i $server_spawn_id
> > +	    -timeout 120
> >  	    -notransfer
> >  	    -re "Listening on" { }
> >  	    -re "Can't bind address: Address already in use\\.\r\n" {
> > 
> 
> OK.
> 
> Wouldn't it be good to add a 'timeout {...}' case that emits a
> warning or some such?

 Good point.  I actually went further than that.

 As it happens we have a board that fails a gdb.base/gcore-relro.exp test 
case reproducibly and moreover the case appears to trigger a kernel bug 
making the it less than usable.  Specifically the board remains responsive 
to some extent, however processes do not appear to be able to successfully 
complete termination anymore and perhaps more importantly further 
gdbserver processes can be started, but they never reach the stage of 
listening on the RSP socket.

 This change handles timeouts in gdbserver start properly, by throwing a 
TCL error exception when gdbserver does not report listening on the RSP 
socket in time.  This is then caught at the outer level and reported, and 
2 rather than 1 is returned so that the caller may tell the failure to 
start gdbserver and other issues apart and act accordingly (or do 
nothing).

 I thought letting the exception unwind further on might be a good idea 
for any test harnesses out there to break outright where a gdbserver start 
error is silently ignored right now, however I figured out the calls to 
gdbserver-support.exp are buried down too deep in the GDB test suite for 
such a change to be made easily.  I think returning a distinct return 
value is good enough (the API says "non-zero", so 2 is as good as 1) and 
we can always make the error harder in a later step if required.

 With config/gdbserver.exp being used this change remains transparent to 
the target board, the return value is passed up by gdb_reload and the 
error exception unwinds through gdbserver_gdb_load and is caught and 
handled by mi_gdb_target_load.  A call to perror is still made, reporting 
the timeout, and in the case of mi_gdb_target_load the procedure returns a 
value denoting unsuccessful completion.  An unsuccessful completion of 
gdb_reload is already handled elsewhere.

 An alternative gdbserver board configuration can interpret the return 
value in its gdb_reload implementation and catch the error in 
gdbserver_gdb_load in an attempt to recover a target board that has gone 
astray, for example by rebooting the board somehow.  This has proved 
effective with our failing board, that now completes the remaining test 
cases with no further hiccups.

 I pushed it through regression testing with the powerpc-linux-gnu target 
and a some half a dozen of multilibs (including ones that trip over the 
faulty kernel on the problematic board) and there were no issues.  The 
full list of the offending test cases is as follows:

FAIL: gdb.base/gcore-relro.exp: save a corefile
FAIL: gdb.base/print-symbol-loading.exp: save a corefile
FAIL: gdb.threads/gcore-thread.exp: save a corefile

so essentially the same stuff in different places.

 OK to apply?

2014-08-26  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/testsuite/
	* lib/gdbserver-support.exp (gdbserver_start): Throw an error
	exception on timeout.
	(gdbserver_run): Catch any `gdbserver_spawn' error exceptions.
	(gdbserver_start_extended): Catch any `gdbserver_start' error
	exceptions.
	(gdbserver_start_multi, mi_gdbserver_start_multi): Likewise.
	* lib/mi-support.exp (mi_gdb_target_load): Catch any
	`gdbserver_gdb_load' error exceptions.

  Maciej

gdb-test-gdbserver-start-timeout-handle.diff
  

On Tue, 26 Aug 2014, Maciej W. Rozycki wrote:

> > Wouldn't it be good to add a 'timeout {...}' case that emits a
> > warning or some such?
> 
>  Good point.  I actually went further than that.
> 
>  As it happens we have a board that fails a gdb.base/gcore-relro.exp test 
> case reproducibly and moreover the case appears to trigger a kernel bug 
> making the it less than usable.  Specifically the board remains responsive 
> to some extent, however processes do not appear to be able to successfully 
> complete termination anymore and perhaps more importantly further 
> gdbserver processes can be started, but they never reach the stage of 
> listening on the RSP socket.
> 
>  This change handles timeouts in gdbserver start properly, by throwing a 
> TCL error exception when gdbserver does not report listening on the RSP 
> socket in time.  This is then caught at the outer level and reported, and 
> 2 rather than 1 is returned so that the caller may tell the failure to 
> start gdbserver and other issues apart and act accordingly (or do 
> nothing).
> 
>  I thought letting the exception unwind further on might be a good idea 
> for any test harnesses out there to break outright where a gdbserver start 
> error is silently ignored right now, however I figured out the calls to 
> gdbserver-support.exp are buried down too deep in the GDB test suite for 
> such a change to be made easily.  I think returning a distinct return 
> value is good enough (the API says "non-zero", so 2 is as good as 1) and 
> we can always make the error harder in a later step if required.
> 
>  With config/gdbserver.exp being used this change remains transparent to 
> the target board, the return value is passed up by gdb_reload and the 
> error exception unwinds through gdbserver_gdb_load and is caught and 
> handled by mi_gdb_target_load.  A call to perror is still made, reporting 
> the timeout, and in the case of mi_gdb_target_load the procedure returns a 
> value denoting unsuccessful completion.  An unsuccessful completion of 
> gdb_reload is already handled elsewhere.
> 
>  An alternative gdbserver board configuration can interpret the return 
> value in its gdb_reload implementation and catch the error in 
> gdbserver_gdb_load in an attempt to recover a target board that has gone 
> astray, for example by rebooting the board somehow.  This has proved 
> effective with our failing board, that now completes the remaining test 
> cases with no further hiccups.
> 
>  I pushed it through regression testing with the powerpc-linux-gnu target 
> and a some half a dozen of multilibs (including ones that trip over the 
> faulty kernel on the problematic board) and there were no issues.  The 
> full list of the offending test cases is as follows:
> 
> FAIL: gdb.base/gcore-relro.exp: save a corefile
> FAIL: gdb.base/print-symbol-loading.exp: save a corefile
> FAIL: gdb.threads/gcore-thread.exp: save a corefile
> 
> so essentially the same stuff in different places.
> 
>  OK to apply?
> 
> 2014-08-26  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gdb/testsuite/
> 	* lib/gdbserver-support.exp (gdbserver_start): Throw an error
> 	exception on timeout.
> 	(gdbserver_run): Catch any `gdbserver_spawn' error exceptions.
> 	(gdbserver_start_extended): Catch any `gdbserver_start' error
> 	exceptions.
> 	(gdbserver_start_multi, mi_gdbserver_start_multi): Likewise.
> 	* lib/mi-support.exp (mi_gdb_target_load): Catch any
> 	`gdbserver_gdb_load' error exceptions.

 Ping!

  Maciej
  

Hi Maciej,

On 08/26/2014 06:14 PM, Maciej W. Rozycki wrote:

>>> Index: gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdbserver-support.exp
>>> ===================================================================
>>> --- gdb-fsf-trunk-quilt.orig/gdb/testsuite/lib/gdbserver-support.exp	2014-05-13 02:52:11.347706187 +0100
>>> +++ gdb-fsf-trunk-quilt/gdb/testsuite/lib/gdbserver-support.exp	2014-05-30 01:45:51.658977074 +0100
>>> @@ -275,6 +275,7 @@ proc gdbserver_start { options arguments
>>>  	# Wait for the server to open its TCP socket, so that GDB can connect.
>>>  	expect {
>>>  	    -i $server_spawn_id
>>> +	    -timeout 120
>>>  	    -notransfer
>>>  	    -re "Listening on" { }
>>>  	    -re "Can't bind address: Address already in use\\.\r\n" {
>>>
>>
>> OK.
>>
>> Wouldn't it be good to add a 'timeout {...}' case that emits a
>> warning or some such?
> 
>  Good point.  I actually went further than that.
> 
>  As it happens we have a board that fails a gdb.base/gcore-relro.exp test 
> case reproducibly and moreover the case appears to trigger a kernel bug 
> making the it less than usable.  Specifically the board remains responsive 
> to some extent, however processes do not appear to be able to successfully 
> complete termination anymore and perhaps more importantly further 
> gdbserver processes can be started, but they never reach the stage of 
> listening on the RSP socket.
> 
>  This change handles timeouts in gdbserver start properly, by throwing a 
> TCL error exception when gdbserver does not report listening on the RSP 
> socket in time.  This is then caught at the outer level and reported, and 
> 2 rather than 1 is returned so that the caller may tell the failure to 
> start gdbserver and other issues apart and act accordingly (or do 
> nothing).
> 
>  I thought letting the exception unwind further on might be a good idea 
> for any test harnesses out there to break outright where a gdbserver start 
> error is silently ignored right now, however I figured out the calls to 
> gdbserver-support.exp are buried down too deep in the GDB test suite for 
> such a change to be made easily.  I think returning a distinct return 
> value is good enough (the API says "non-zero", so 2 is as good as 1) and 
> we can always make the error harder in a later step if required.
> 
>  With config/gdbserver.exp being used this change remains transparent to 
> the target board, the return value is passed up by gdb_reload and the 
> error exception unwinds through gdbserver_gdb_load and is caught and 
> handled by mi_gdb_target_load.  A call to perror is still made, reporting 
> the timeout, and in the case of mi_gdb_target_load the procedure returns a 
> value denoting unsuccessful completion.  An unsuccessful completion of 
> gdb_reload is already handled elsewhere.
> 
>  An alternative gdbserver board configuration can interpret the return 
> value in its gdb_reload implementation and catch the error in 
> gdbserver_gdb_load in an attempt to recover a target board that has gone 
> astray, for example by rebooting the board somehow.  This has proved 
> effective with our failing board, that now completes the remaining test 
> cases with no further hiccups.
> 
>  I pushed it through regression testing with the powerpc-linux-gnu target 
> and a some half a dozen of multilibs (including ones that trip over the 
> faulty kernel on the problematic board) and there were no issues.  The 
> full list of the offending test cases is as follows:
> 
> FAIL: gdb.base/gcore-relro.exp: save a corefile
> FAIL: gdb.base/print-symbol-loading.exp: save a corefile
> FAIL: gdb.threads/gcore-thread.exp: save a corefile
> 
> so essentially the same stuff in different places.
> 
>  OK to apply?

OK.

Thanks,
Pedro Alves
  

On Thu, 4 Sep 2014, Pedro Alves wrote:

> >  I pushed it through regression testing with the powerpc-linux-gnu target 
> > and a some half a dozen of multilibs (including ones that trip over the 
> > faulty kernel on the problematic board) and there were no issues.  The 
> > full list of the offending test cases is as follows:
> > 
> > FAIL: gdb.base/gcore-relro.exp: save a corefile
> > FAIL: gdb.base/print-symbol-loading.exp: save a corefile
> > FAIL: gdb.threads/gcore-thread.exp: save a corefile
> > 
> > so essentially the same stuff in different places.
> > 
> >  OK to apply?
> 
> OK.

 Thanks for your review, I have applied this change now too.

  Maciej