[rfc] Handle lack of non-stop support more gracefully

[rfc] Handle lack of non-stop support more gracefully

[Ulrich Weigand]

Hello,

on spu-elf I'm now seeing:

ERROR: mi-ns-stale-regcache.exp tests suppressed
UNRESOLVED: gdb.mi/mi-nsintrall.exp: Couldn't compile /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.mi/nsintrall.c: unrecognized error

Now, on the SPU we don't support non-stop execution (in fact, the SPU
is always single-threaded anyway).  Therefore, all the non-stop tests
ought to be marked as UNSUPPORTED.  And in fact, the mi_run_to_main
routine does that.

However, various test cases, *in addition* to the actions done by
the mi_run_to_main routine, themselves call "perror":
    perror "mi-ns-stale-regcache.exp tests suppressed"

This seems wrong to me: First of all, an ERROR is supposed to mark some
sort of unexpected failure of the test harness itself, with unpredictable
results on the test outcome.  This is not the case for a feature that is
simply not supported on a target.  Second, because of that, the DejaGNU
main test harness will use the presence of any perror call as signal to
mark the next regular test result as UNRESOLVED instead of whatever the
actual result was -- even if this happens to be in a completely different
test!  (See the example above.)

[ Note that the reason this doesn't show up with any of the other non-stop
tests on SPU is that they already fail during the compile stage due to
the absence of pthreads support.  mi-ns-stale-regcache is the only non-stop
test that does not require pthreads.  ]

It seems to me the correct way to handle this is for mi_run_to_main to
detect the case where the non-stop feature is unsupported, generate an
appropriate test result (UNSUPPORTED), which it already does, and then
have the main test just terminate with no further message.

The following patch implements this, and fixes the above problems on SPU.

Pedro, it seems you originally added the perror calls -- was there any
reason I may be missing why we should need them anyway?

Bye,
Ulrich

ChangeLog:

	* gdb.mi/mi-nonstop.exp: Do not call perror if non-stop mode is
	not supported on the target.
	* gdb.mi/mi-nonstop-exit.exp: Likewise.
	* gdb.mi/mi-ns-stale-regcache.exp: Likewise.
	* gdb.mi/mi-nsintrall.exp: Likewise.
	* gdb.mi/mi-nsmoribund.exp: Likewise.
	* gdb.mi/mi-nsthrexec.exp: Likewise.

Index: gdb/testsuite/gdb.mi/mi-nonstop-exit.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-nonstop-exit.exp,v
retrieving revision 1.4
diff -u -p -r1.4 mi-nonstop-exit.exp
--- gdb/testsuite/gdb.mi/mi-nonstop-exit.exp	26 May 2010 18:12:13 -0000	1.4
+++ gdb/testsuite/gdb.mi/mi-nonstop-exit.exp	13 Jun 2010 17:00:16 -0000
@@ -52,7 +52,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-nonstop-exit.exp tests suppressed"
     continue
 }
 
@@ -63,7 +62,6 @@ mi_expect_stop "exited-normally" "" "" "
 # Run the program again.
 
 if { [mi_run_to_main] < 0 } {
-    fail "run (2)"
     continue
 }
 
Index: gdb/testsuite/gdb.mi/mi-nonstop.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-nonstop.exp,v
retrieving revision 1.12
diff -u -p -r1.12 mi-nonstop.exp
--- gdb/testsuite/gdb.mi/mi-nonstop.exp	26 May 2010 18:12:13 -0000	1.12
+++ gdb/testsuite/gdb.mi/mi-nonstop.exp	13 Jun 2010 17:00:16 -0000
@@ -62,7 +62,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-nonstop.exp tests suppressed"
     continue
 }
 
Index: gdb/testsuite/gdb.mi/mi-ns-stale-regcache.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-ns-stale-regcache.exp,v
retrieving revision 1.2
diff -u -p -r1.2 mi-ns-stale-regcache.exp
--- gdb/testsuite/gdb.mi/mi-ns-stale-regcache.exp	26 May 2010 18:12:13 -0000	1.2
+++ gdb/testsuite/gdb.mi/mi-ns-stale-regcache.exp	13 Jun 2010 17:00:16 -0000
@@ -66,7 +66,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-ns-stale-regcache.exp tests suppressed"
     continue
 }
 
Index: gdb/testsuite/gdb.mi/mi-nsintrall.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-nsintrall.exp,v
retrieving revision 1.7
diff -u -p -r1.7 mi-nsintrall.exp
--- gdb/testsuite/gdb.mi/mi-nsintrall.exp	1 Jun 2010 17:22:33 -0000	1.7
+++ gdb/testsuite/gdb.mi/mi-nsintrall.exp	13 Jun 2010 17:00:16 -0000
@@ -52,7 +52,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-nsintrall.exp tests suppressed"
     continue
 }
 
Index: gdb/testsuite/gdb.mi/mi-nsmoribund.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-nsmoribund.exp,v
retrieving revision 1.6
diff -u -p -r1.6 mi-nsmoribund.exp
--- gdb/testsuite/gdb.mi/mi-nsmoribund.exp	26 May 2010 18:12:13 -0000	1.6
+++ gdb/testsuite/gdb.mi/mi-nsmoribund.exp	13 Jun 2010 17:00:16 -0000
@@ -52,7 +52,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-nsmoribund.exp tests suppressed"
     continue
 }
 
Index: gdb/testsuite/gdb.mi/mi-nsthrexec.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-nsthrexec.exp,v
retrieving revision 1.3
diff -u -p -r1.3 mi-nsthrexec.exp
--- gdb/testsuite/gdb.mi/mi-nsthrexec.exp	26 May 2010 18:12:13 -0000	1.3
+++ gdb/testsuite/gdb.mi/mi-nsthrexec.exp	13 Jun 2010 17:00:16 -0000
@@ -62,7 +62,6 @@ mi_gdb_test "-gdb-set target-async 1" ".
 detect_async
 
 if { [mi_run_to_main] < 0 } {
-    perror "mi-nsthrexec.exp tests suppressed"
     continue
 }
 
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Handle lack of non-stop support more gracefully

[Pedro Alves]

On Monday 14 June 2010 15:13:29, Ulrich Weigand wrote:
> Hello,
> 
> on spu-elf I'm now seeing:
> 
> ERROR: mi-ns-stale-regcache.exp tests suppressed
> UNRESOLVED: gdb.mi/mi-nsintrall.exp: Couldn't compile /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.mi/nsintrall.c: unrecognized error
> 
> Now, on the SPU we don't support non-stop execution (in fact, the SPU
> is always single-threaded anyway).  Therefore, all the non-stop tests
> ought to be marked as UNSUPPORTED.  And in fact, the mi_run_to_main
> routine does that.
> 
> However, various test cases, *in addition* to the actions done by
> the mi_run_to_main routine, themselves call "perror":
>     perror "mi-ns-stale-regcache.exp tests suppressed"
> 
> This seems wrong to me: First of all, an ERROR is supposed to mark some
> sort of unexpected failure of the test harness itself, with unpredictable
> results on the test outcome.  This is not the case for a feature that is
> simply not supported on a target.  Second, because of that, the DejaGNU
> main test harness will use the presence of any perror call as signal to
> mark the next regular test result as UNRESOLVED instead of whatever the
> actual result was -- even if this happens to be in a completely different
> test!  (See the example above.)
> 
> [ Note that the reason this doesn't show up with any of the other non-stop
> tests on SPU is that they already fail during the compile stage due to
> the absence of pthreads support.  mi-ns-stale-regcache is the only non-stop
> test that does not require pthreads.  ]
> 
> It seems to me the correct way to handle this is for mi_run_to_main to
> detect the case where the non-stop feature is unsupported, generate an
> appropriate test result (UNSUPPORTED), which it already does, and then
> have the main test just terminate with no further message.
> 
> The following patch implements this, and fixes the above problems on SPU.
> 
> Pedro, it seems you originally added the perror calls -- was there any
> reason I may be missing why we should need them anyway?

Sorry, I don't recall.  There was probably no good reason.  I probably
copied it from what some CLI tests do:

 if ![runto_main] then {
   perror "Couldn't run to main"
   return -1
 }

I've no objections to your patch.

I took a quick look over mi-support, and I can see how I got a bit confused
about what do the different return codes leading up to mi_run_to_main mean.
It looks like some non-gdbserver targets will still trip on
this problem:

    } elseif { [target_info gdb_protocol] == "remote" } {
	# remote targets
	if { [mi_gdb_target_cmd "remote" [target_info netport]] != 0 } {
	    perror "Unable to connect to remote target"
	    return -1
	}

?

Maybe mi_gdb_target_cmd should return a different error code for
not-supported vs connection error.

I'm still a bit confused over how the non-stop MI handle this.  If
mi_gdb_target_cmd fails to connect, it seems to return 0 anyway, so
the following tests will just cascade in FAILs.  For other, non-remote
targets, mi_gdb_target_load will call perror on connection fail, but
the gdbserver branch at the top doesn't.

-- 
Pedro Alves

Re: [rfc] Handle lack of non-stop support more gracefully

[Ulrich Weigand]

Pedro Alves wrote:
> On Monday 14 June 2010 15:13:29, Ulrich Weigand wrote:
> > Pedro, it seems you originally added the perror calls -- was there any
> > reason I may be missing why we should need them anyway?
> 
> Sorry, I don't recall.  There was probably no good reason.  I probably
> copied it from what some CLI tests do:
> 
>  if ![runto_main] then {
>    perror "Couldn't run to main"
>    return -1
>  }
> 
> I've no objections to your patch.

OK, thanks.  I've checked the patch in now.

> I took a quick look over mi-support, and I can see how I got a bit confused
> about what do the different return codes leading up to mi_run_to_main mean.
> It looks like some non-gdbserver targets will still trip on
> this problem:
> 
>     } elseif { [target_info gdb_protocol] == "remote" } {
> 	# remote targets
> 	if { [mi_gdb_target_cmd "remote" [target_info netport]] != 0 } {
> 	    perror "Unable to connect to remote target"
> 	    return -1
> 	}
> 
> ?
> 
> Maybe mi_gdb_target_cmd should return a different error code for
> not-supported vs connection error.
> 
> I'm still a bit confused over how the non-stop MI handle this.  If
> mi_gdb_target_cmd fails to connect, it seems to return 0 anyway, so
> the following tests will just cascade in FAILs.  For other, non-remote
> targets, mi_gdb_target_load will call perror on connection fail, but
> the gdbserver branch at the top doesn't.

I would suggest that just about any use of perror is wrong here.  If
the underlying library routine detects any condition that makes the
rest of the test execution impossible, it should itself issue an
appropriate test status, which would usually be FAIL (if the condition
is due to a GDB bug), UNSUPPORTED (if it is due to some feature not
available on the platform), or UNRESOLVED (if it is due to some setup
or other external issue, like the target connection failing).

Then, the library should return an error code that causes the main
test case to silently stop any further test execution.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Handle lack of non-stop support more gracefully

[Pedro Alves]

On Monday 14 June 2010 16:49:56, Ulrich Weigand wrote:
> > I'm still a bit confused over how the non-stop MI handle this.  If

(for the record, I meant "the all-stop MI tests" here.)

> > mi_gdb_target_cmd fails to connect, it seems to return 0 anyway, so
> > the following tests will just cascade in FAILs.  For other, non-remote
> > targets, mi_gdb_target_load will call perror on connection fail, but
> > the gdbserver branch at the top doesn't.
> 
> I would suggest that just about any use of perror is wrong here.  If
> the underlying library routine detects any condition that makes the
> rest of the test execution impossible, it should itself issue an
> appropriate test status, which would usually be FAIL (if the condition
> is due to a GDB bug), UNSUPPORTED (if it is due to some feature not
> available on the platform), or UNRESOLVED (if it is due to some setup
> or other external issue, like the target connection failing).
> 
> Then, the library should return an error code that causes the main
> test case to silently stop any further test execution.

Thanks.  Sounds like a good plan.

-- 
Pedro Alves