RFC: consolidate checks for _Unwind_DebugHook in test suite

RFC: consolidate checks for _Unwind_DebugHook in test suite

[Tom Tromey]

I happened to notice that nextoverthrow.exp checks for the sdt.h probe
points, but jnpe.exp does not.

This patch consolidates the checking code into a new function in
gdb.exp.

Tom

    	* lib/gdb.exp (skip_unwinder_tests): New proc.
    	* gdb.cp/nextoverthrow.exp: Use skip_unwinder_tests.
    	* gdb.java/jnpe.exp: Use skip_unwinder_tests.

diff --git a/gdb/testsuite/gdb.cp/nextoverthrow.exp b/gdb/testsuite/gdb.cp/nextoverthrow.exp
index 53f59d7..488ae87 100644
--- a/gdb/testsuite/gdb.cp/nextoverthrow.exp
+++ b/gdb/testsuite/gdb.cp/nextoverthrow.exp
@@ -34,35 +34,7 @@ if ![runto_main] then {
     continue
 } 
 
-# See whether we have the needed unwinder hooks.
-set ok 1
-gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
-    -re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	# Pass the test so we don't get bogus fails in the results.
-	pass "check for unwinder hook"
-	set ok 0
-    }
-    -re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	pass "check for unwinder hook"
-    }
-    -re "No symbol .* in current context.\r\n$gdb_prompt $" {
-	# Pass the test so we don't get bogus fails in the results.
-	pass "check for unwinder hook"
-	set ok 0
-    }
-}
-if {!$ok} {
-    gdb_test_multiple "info probe" "check for stap probe in unwinder" {
-	-re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
-	    pass "check for stap probe in unwinder"
-	    set ok 1
-	}
-	-re "\r\n$gdb_prompt $" {
-	}
-    }
-}
-
-if {!$ok} {
+if {![skip_unwinder_tests]} {
     unsupported "nextoverthrow.exp could not find _Unwind_DebugHook"
     return -1
 }
diff --git a/gdb/testsuite/gdb.java/jnpe.exp b/gdb/testsuite/gdb.java/jnpe.exp
index f7225d8..b15ee32 100644
--- a/gdb/testsuite/gdb.java/jnpe.exp
+++ b/gdb/testsuite/gdb.java/jnpe.exp
@@ -31,20 +31,7 @@ if ![runto "$testfile.java:$line"] then {
     continue
 } 
 
-# See whether we have the needed unwinder hooks.
-set ok 1
-gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook in java" {
-    -re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	pass "check for unwinder hook in java"
-    }
-    -re "No symbol .* in current context.?\r\n$gdb_prompt $" {
-	# Pass the test so we don't get bogus fails in the results.
-	setup_xfail *-*-*
-	fail "check for unwinder hook in java"
-	set ok 0
-    }
-}
-if {!$ok} {
+if {![skip_unwinder_tests]} {
     unsupported "jnpe.exp could not find _Unwind_DebugHook"
     return -1
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 17e2117..daa50f5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2093,6 +2093,43 @@ proc skip_hw_watchpoint_access_tests {} {
     return 0
 }
 
+# Return 1 if we should skip tests that require the runtime unwinder
+# hook.  This must be invoked while gdb is running, after shared
+# libraries have been loaded.  This is needed because otherwise a
+# shared libgcc won't be visible.
+
+proc skip_unwinder_tests {} {
+    global gdb_prompt
+
+    set ok 1
+    gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
+	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
+	    # Pass the test so we don't get bogus fails in the results.
+	    pass "check for unwinder hook"
+	    set ok 0
+	}
+	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
+	    pass "check for unwinder hook"
+	}
+	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
+	    # Pass the test so we don't get bogus fails in the results.
+	    pass "check for unwinder hook"
+	    set ok 0
+	}
+    }
+    if {!$ok} {
+	gdb_test_multiple "info probe" "check for stap probe in unwinder" {
+	    -re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
+		pass "check for stap probe in unwinder"
+		set ok 1
+	    }
+	    -re "\r\n$gdb_prompt $" {
+	    }
+	}
+    }
+    return $ok
+}
+
 set compiler_info		"unknown"
 set gcc_compiled		0
 set hp_cc_compiler		0

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Yao Qi]

On 08/15/2012 12:20 AM, Tom Tromey wrote:
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 17e2117..daa50f5 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2093,6 +2093,43 @@ proc skip_hw_watchpoint_access_tests {} {
>       return 0
>   }
>
> +# Return 1 if we should skip tests that require the runtime unwinder
> +# hook.  This must be invoked while gdb is running, after shared
> +# libraries have been loaded.  This is needed because otherwise a
> +# shared libgcc won't be visible.
> +
> +proc skip_unwinder_tests {} {
> +    global gdb_prompt
> +
> +    set ok 1
> +    gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
> +	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
> +	    # Pass the test so we don't get bogus fails in the results.
> +	    pass "check for unwinder hook"

Do we really need to put "pass" here?  skip_* proc in lib/gdb.exp is to 
do some check and return a boolean value.  We don't have to generate any 
PASS or FAIL during checking.  I'd like to remove them.

> +	    set ok 0
> +	}
> +	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
> +	    pass "check for unwinder hook"
> +	}
> +	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
> +	    # Pass the test so we don't get bogus fails in the results.
> +	    pass "check for unwinder hook"
> +	    set ok 0
> +	}
> +    }
> +    if {!$ok} {
> +	gdb_test_multiple "info probe" "check for stap probe in unwinder" {
> +	    -re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
> +		pass "check for stap probe in unwinder"
> +		set ok 1
> +	    }
> +	    -re "\r\n$gdb_prompt $" {
> +	    }
> +	}
> +    }
> +    return $ok
> +}
> +


-- 
Yao

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Tom Tromey]

Yao> Do we really need to put "pass" here?  skip_* proc in lib/gdb.exp is
Yao> to do some check and return a boolean value.  We don't have to
Yao> generate any PASS or FAIL during checking.  I'd like to remove them.

I would like that too, but you can't really use gdb_test_multiple
without these, because it may emit fails on its own.

I think that is a bad factoring -- there should be a way to interact
with gdb without muddying affecting the test count.  But right now there
isn't.

I wouldn't mind if someone changed this, but this patch is just a
straightforward refactoring.

Tom

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom>     	* lib/gdb.exp (skip_unwinder_tests): New proc.
Tom>     	* gdb.cp/nextoverthrow.exp: Use skip_unwinder_tests.
Tom>     	* gdb.java/jnpe.exp: Use skip_unwinder_tests.

I'm checking this in.

Tom

[PATCH 2/2] Remove pass in skip_unwinder_tests

[Yao Qi]

Hi,
As we discussed, proc skip_unwinder_tests should not generate any FAIL
or PASS in test summary, so this patch is a follow-up to to remove
'pass', and replace gdb_test_multiple with send_gdb/gdb_expect.  With
this patch applied, these PASS are disappeared in test result.

 -PASS: gdb.cp/nextoverthrow.exp: check for unwinder hook
 -PASS: gdb.cp/nextoverthrow.exp: check for stap probe in unwinder
 -PASS: gdb.java/jnpe.exp: check for unwinder hook
 -PASS: gdb.java/jnpe.exp: check for stap probe in unwinder

gdb/testsuite:

2012-08-23  Yao Qi  <yao@codesourcery.com>

	* lib/gdb.exp (skip_unwinder_tests): Replace 'gdb_test_multiple'
	with 'send_gdb' and 'gdb_expect'.  Remove pass.
---
 gdb/testsuite/lib/gdb.exp |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0b02f76..99d6591 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2102,25 +2102,22 @@ proc skip_unwinder_tests {} {
     global gdb_prompt
 
     set ok 1
-    gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
+
+    send_gdb "print _Unwind_DebugHook\n"
+    gdb_expect {
 	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	    # Pass the test so we don't get bogus fails in the results.
-	    pass "check for unwinder hook"
 	    set ok 0
 	}
 	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	    pass "check for unwinder hook"
 	}
 	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
-	    # Pass the test so we don't get bogus fails in the results.
-	    pass "check for unwinder hook"
 	    set ok 0
 	}
     }
     if {!$ok} {
-	gdb_test_multiple "info probe" "check for stap probe in unwinder" {
+	send_gdb "info probe\n"
+	gdb_expect {
 	    -re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
-		pass "check for stap probe in unwinder"
 		set ok 1
 	    }
 	    -re "\r\n$gdb_prompt $" {
-- 
1.7.7.6

[PATCH 1/2] Append "." in error message.

[Yao Qi]

This patch is to add "." at the end of message "No symbol XXX in
current context", and fix a FAIL in testsuite.

 -FAIL: gdb.java/jnpe.exp: check for unwinder hook
 +PASS: gdb.java/jnpe.exp: check for unwinder hook
 +PASS: gdb.java/jnpe.exp: check for stap probe in unwinder

It is obvious.  I'll check it in.

gdb:

2012-08-23  Yao Qi  <yao@codesourcery.com>

	* jv-exp.y (push_expression_name): Add "." at the end of error
	message.
---
 gdb/jv-exp.y |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/jv-exp.y b/gdb/jv-exp.y
index ee17654..d0fca67 100644
--- a/gdb/jv-exp.y
+++ b/gdb/jv-exp.y
@@ -1399,7 +1399,7 @@ push_expression_name (struct stoken name)
       else if (!have_full_symbols () && !have_partial_symbols ())
 	error (_("No symbol table is loaded.  Use the \"file\" command"));
       else
-	error (_("No symbol \"%s\" in current context"), tmp);
+	error (_("No symbol \"%s\" in current context."), tmp);
     }
 
 }
-- 
1.7.7.6

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Pedro Alves]

On 08/23/2012 10:49 AM, Yao Qi wrote:

> As we discussed, proc skip_unwinder_tests should not generate any FAIL
> or PASS in test summary, so this patch is a follow-up to to remove
> 'pass', and replace gdb_test_multiple with send_gdb/gdb_expect.  With
> this patch applied, these PASS are disappeared in test result.

Do we really need to replace gdb_test_multiple with send_gdb/gdb_expect?
gdb_test_multiple will only issue fails in the internal errors cases.
I find it acceptable that see nothing most the time, and see a FAIL
when something goes really really wrong.  More acceptable than the
timeouts we likely get with send_gdb/gdb_expect in the same really
really wrong cases, that is.

-- 
Pedro Alves

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Yao Qi]

On 08/23/2012 06:51 PM, Pedro Alves wrote:
> Do we really need to replace gdb_test_multiple with send_gdb/gdb_expect?

No.

> gdb_test_multiple will only issue fails in the internal errors cases.
> I find it acceptable that see nothing most the time, and see a FAIL
> when something goes really really wrong.  More acceptable than the
> timeouts we likely get with send_gdb/gdb_expect in the same really
> really wrong cases, that is.

I thought gdb_test_multiple contributes PASS to test summary by
mistake.  Here is a new version, which only removes 'pass' and keep gdb_test_multiple there.

-- 
Yao

gdb/testsuite:

2012-08-23  Yao Qi  <yao@codesourcery.com>

	* lib/gdb.exp (skip_unwinder_tests): Remove pass.
---
 gdb/testsuite/lib/gdb.exp |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0b02f76..97e79af 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2104,23 +2104,17 @@ proc skip_unwinder_tests {} {
     set ok 1
     gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
 	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	    # Pass the test so we don't get bogus fails in the results.
-	    pass "check for unwinder hook"
 	    set ok 0
 	}
 	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	    pass "check for unwinder hook"
 	}
 	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
-	    # Pass the test so we don't get bogus fails in the results.
-	    pass "check for unwinder hook"
 	    set ok 0
 	}
     }
     if {!$ok} {
 	gdb_test_multiple "info probe" "check for stap probe in unwinder" {
 	    -re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
-		pass "check for stap probe in unwinder"
 		set ok 1
 	    }
 	    -re "\r\n$gdb_prompt $" {
-- 
1.7.7.6

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Pedro Alves]

On 08/23/2012 01:29 PM, Yao Qi wrote:

> Here is a new version, which only removes 'pass' and keep gdb_test_multiple there.

Looks good to me.  Thanks.

-- 
Pedro Alves

Re: [PATCH 1/2] Append "." in error message.

[Jan Kratochvil]

On Thu, 23 Aug 2012 11:49:48 +0200, Yao Qi wrote:
> This patch is to add "." at the end of message "No symbol XXX in
> current context", and fix a FAIL in testsuite.
> 
>  -FAIL: gdb.java/jnpe.exp: check for unwinder hook
>  +PASS: gdb.java/jnpe.exp: check for unwinder hook
>  +PASS: gdb.java/jnpe.exp: check for stap probe in unwinder
> 
> It is obvious.  I'll check it in.
[...]
> -	error (_("No symbol \"%s\" in current context"), tmp);
> +	error (_("No symbol \"%s\" in current context."), tmp);

Yes, please, to get out of all the pending regressions.

I hope no other regressions are caused by it.


Thanks,
Jan

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Jan Kratochvil]

On Thu, 23 Aug 2012 11:49:49 +0200, Yao Qi wrote:
> As we discussed, proc skip_unwinder_tests should not generate any FAIL
> or PASS in test summary,

During comparison across releases and testsuite modes it makes the diffs more
difficult to read:

 Running gdb/testsuite/gdb.java/jnpe.exp ...
 PASS: gdb.java/jnpe.exp: compilation jnpe.java
-FAIL: gdb.java/jnpe.exp: check for unwinder hook
 PASS: gdb.java/jnpe.exp: disable SIGSEGV for next-over-NPE
 PASS: gdb.java/jnpe.exp: next over NPE
-PASS: gdb.java/jnpe.exp: continue to success for next-over-NPE
+FAIL: gdb.java/jnpe.exp: continue to success for next-over-NPE


Has the last testcase regressed because the check "check for unwinder hook" is
therefore no longer there?  Does it PASS or FAIL now? etc.


In other case:

#gdb.java/jnpe.exp
 Running gdb/testsuite/gdb.java/jnpe.exp ...
 PASS: gdb.java/jnpe.exp: compilation jnpe.java
-PASS: gdb.java/jnpe.exp: check for unwinder hook
 PASS: gdb.java/jnpe.exp: disable SIGSEGV for next-over-NPE
 PASS: gdb.java/jnpe.exp: next over NPE
 FAIL: gdb.java/jnpe.exp: continue to success for next-over-NPE

It was clear before why the testcase failed - because its
'check for unwinder hook' PASSes which should not in RHEL-5 x86_64 -m32 mode
(as debuginfo from gcc libraries is missing there).

the testcase was doing:

 (gdb) print _Unwind_DebugHook^M
 $1 = {void(void, void)} 0x7ffff78d4a90 <_Unwind_DebugHook>^M
 (gdb) PASS: gdb.java/jnpe.exp: check for unwinder hook
 handle SIGSEGV nostop noprint^M
 Signal        Stop     Print   Pass to program Description^M
 SIGSEGV       No       No      Yes             Segmentation fault^M
 (gdb) PASS: gdb.java/jnpe.exp: disable SIGSEGV for next-over-NPE

Not is that "print _Unwind_DebugHook"
 a part of the "disable SIGSEGV for next-over-NPE" test?

 (gdb) print _Unwind_DebugHook^M
 $1 = {void(void, void)} 0x7ffff78d4a90 <_Unwind_DebugHook>^M
 (gdb) handle SIGSEGV nostop noprint^M
 Signal        Stop     Print   Pass to program Description^M
 SIGSEGV       No       No      Yes             Segmentation fault^M
 (gdb) PASS: gdb.java/jnpe.exp: disable SIGSEGV for next-over-NPE

I do not see removing of useful messages to be much improvement.
I understand it was checked in with a good intention.


Jan

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Jan Kratochvil]

On Tue, 14 Aug 2012 18:20:14 +0200, Tom Tromey wrote:
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
[...]
> +proc skip_unwinder_tests {} {
> +    global gdb_prompt
> +
> +    set ok 1
> +    gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
> +	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
> +	    # Pass the test so we don't get bogus fails in the results.
> +	    pass "check for unwinder hook"
> +	    set ok 0
> +	}
> +	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
> +	    pass "check for unwinder hook"
> +	}
> +	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
> +	    # Pass the test so we don't get bogus fails in the results.
> +	    pass "check for unwinder hook"
> +	    set ok 0
> +	}
> +    }

If gdb_test_multiple FAILs it will leave "ok" as 1 which will run the whole
testfile.  Was it intentional?  Maybe it was although I would expect the
opposite.


Jan

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> If gdb_test_multiple FAILs it will leave "ok" as 1 which will run the whole
Jan> testfile.  Was it intentional?  Maybe it was although I would expect the
Jan> opposite.

Bug inherited from the status quo ante.

What do you think of this?
I'm testing it.

Tom

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 97e79af..0b4c679 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2101,15 +2101,14 @@ proc skip_hw_watchpoint_access_tests {} {
 proc skip_unwinder_tests {} {
     global gdb_prompt
 
-    set ok 1
+    set ok 0
     gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
 	-re "= .*no debug info.*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
-	    set ok 0
 	}
 	-re "= .*_Unwind_DebugHook.*\r\n$gdb_prompt $" {
+	    set ok 1
 	}
 	-re "No symbol .* in current context.\r\n$gdb_prompt $" {
-	    set ok 0
 	}
     }
     if {!$ok} {

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Jan Kratochvil]

On Fri, 24 Aug 2012 15:54:04 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> If gdb_test_multiple FAILs it will leave "ok" as 1 which will run the whole
> Jan> testfile.  Was it intentional?  Maybe it was although I would expect the
> Jan> opposite.
> 
> Bug inherited from the status quo ante.

I see now.


> What do you think of this?

It looks OK to me.


Thanks,
Jan

Re: RFC: consolidate checks for _Unwind_DebugHook in test suite

[Tom Tromey]

Jan> It looks OK to me.

I'm checking it in with this ChangeLog entry.

2012-08-24  Tom Tromey  <tromey@redhat.com>

	* lib/gdb.exp (skip_unwinder_tests): Don't leave 'ok' set if
	gdb_test_multiple fails for other reasons.

Tom

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Pedro Alves]

On 08/24/2012 02:37 PM, Jan Kratochvil wrote:
> On Thu, 23 Aug 2012 11:49:49 +0200, Yao Qi wrote:
>> > As we discussed, proc skip_unwinder_tests should not generate any FAIL
>> > or PASS in test summary,
> During comparison across releases and testsuite modes it makes the diffs more
> difficult to read:
> 
>  Running gdb/testsuite/gdb.java/jnpe.exp ...
>  PASS: gdb.java/jnpe.exp: compilation jnpe.java
> -FAIL: gdb.java/jnpe.exp: check for unwinder hook
>  PASS: gdb.java/jnpe.exp: disable SIGSEGV for next-over-NPE
>  PASS: gdb.java/jnpe.exp: next over NPE
> -PASS: gdb.java/jnpe.exp: continue to success for next-over-NPE
> +FAIL: gdb.java/jnpe.exp: continue to success for next-over-NPE
> 
> 
> Has the last testcase regressed because the check "check for unwinder hook" is
> therefore no longer there?  Does it PASS or FAIL now? etc.

I disagree.  Such cases will always happen.  Tests are removed, changed and
renamed all the time.

Nothing actually FAILed here.  We have lots of precedent for "supports-foo" or
"try this" style functions that issue no FAIL.  It is expected that
some systems won't have the unwinder hooks.  In the absurd, issuing a FAIL for
these cases would be like issuing FAILs when tests are skipped because
a [istarget "foobar-*-*"] returns false.

-- 
Pedro Alves

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Jan Kratochvil]

On Fri, 24 Aug 2012 17:40:55 +0200, Pedro Alves wrote:
> Nothing actually FAILed here.  We have lots of precedent for "supports-foo" or
> "try this" style functions that issue no FAIL.

There are cases which one can be sure they never can fail.  But otherwise
I find it as a testsuitea bug.


> It is expected that
> some systems won't have the unwinder hooks.  In the absurd, issuing a FAIL for
> these cases would be like issuing FAILs when tests are skipped because
> a [istarget "foobar-*-*"] returns false.

If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
displayed on screen during interactive run.

If it even FAILs it is a GDB testsuite problem one should fix.

In summary I find better:

-PASS: gdb.java/jnpe.exp: check for unwinder hook
+FAIL: gdb.java/jnpe.exp: check for unwinder hook

or:

-XFAIL: gdb.java/jnpe.exp: check for unwinder hook
+FAIL: gdb.java/jnpe.exp: check for unwinder hook

I find worse to get in diffs just:

+FAIL: gdb.java/jnpe.exp: check for unwinder hook

Sure the testsuite has much more serious problems than this one, but when we
already discuss it it would be nice to get some consensus and write it to:
	http://sourceware.org/gdb/wiki/GDBTestcaseCookbook


Thanks,
Jan

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Pedro Alves]

On 08/24/2012 05:18 PM, Jan Kratochvil wrote:
> On Fri, 24 Aug 2012 17:40:55 +0200, Pedro Alves wrote:
>> Nothing actually FAILed here.  We have lots of precedent for "supports-foo" or
>> "try this" style functions that issue no FAIL.
> 
> There are cases which one can be sure they never can fail.  But otherwise
> I find it as a testsuitea bug.
> 
> 
>> It is expected that
>> some systems won't have the unwinder hooks.  In the absurd, issuing a FAIL for
>> these cases would be like issuing FAILs when tests are skipped because
>> a [istarget "foobar-*-*"] returns false.
> 
> If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
> displayed on screen during interactive run.

That's not what an XFAIL is for.  XFAIL is when you do
"print 2+2", you expect "4" to come out, but you know that on
some broken systems instead "5" comes out, so you XFAIL on those systems,
as in, to fix that _wrong result_, you need to fix something else, not GDB,
but there _is_ something broken that should be fixed.

The fact is that the check for the unwinder hook actually _succeeded_.
It just happened that the result of the successful test was
"no unwinder hook".

I don't believe this support check should XFAIL on not-top-of-tree-glibc,
just like they shouldn't trigger an XFAIL on Windows or Solaris (if it
ever runs there).  Rather, this is why DejaGNU has UNSUPPORTED -- it also
leaves a line in the .sum file, and is also not displayed on screen during
interactive run (IIUYC).  If testing for the presence of the unwinder hooks
fails (as in, we get some unexpected output, like an internal error), then
we should be issuing in addition an UNRESOLVED instead of UNSUPPORTED
(we don't do that presently):

 "Declares a test to have an unresolved outcome. unresolved writes in the log file a
 message beginning with `UNRESOLVED', appending the argument string. This usually means
 the test did not execute as expected, and a human being must go over results to
 determine if it passed or failed (and to improve the test case). "


> If it even FAILs it is a GDB testsuite problem one should fix.
> 
> In summary I find better:
> 
> -PASS: gdb.java/jnpe.exp: check for unwinder hook
> +FAIL: gdb.java/jnpe.exp: check for unwinder hook
> 
> or:
> 
> -XFAIL: gdb.java/jnpe.exp: check for unwinder hook
> +FAIL: gdb.java/jnpe.exp: check for unwinder hook
> 
> I find worse to get in diffs just:
> 
> +FAIL: gdb.java/jnpe.exp: check for unwinder hook

In isolation, it's mildly worse, because you can't tell whether
it's a regression, or a new failure.  But if you get that new
FAIL, you'll actually see:

FAIL: gdb.java/jnpe.exp: check for unwinder hook
UNSUPPORTED: gdb.java/jnpe.exp: jnpe.exp could not find _Unwind_DebugHook"

and several following

-PASS: ...
-PASS: ...
-PASS: ...

lines missing, corresponding to the tests that were skipped.

Those FAIL+UNSUPPORTED lines look pretty clearly related to me.

And IMO, ideally, we should see:

FAIL: gdb.java/jnpe.exp: check for unwinder hook
UNRESOLVED: gdb.java/jnpe.exp: looking for unwinder hooks failed

> 
> Sure the testsuite has much more serious problems than this one, but when we
> already discuss it it would be nice to get some consensus and write it to:
> 	http://sourceware.org/gdb/wiki/GDBTestcaseCookbook
> 
> 
> Thanks,
> Jan
> 


-- 
Pedro Alves

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Pedro Alves]

On 08/24/2012 05:52 PM, Pedro Alves wrote:
> On 08/24/2012 05:18 PM, Jan Kratochvil wrote:
>> On Fri, 24 Aug 2012 17:40:55 +0200, Pedro Alves wrote:
>>> Nothing actually FAILed here.  We have lots of precedent for "supports-foo" or
>>> "try this" style functions that issue no FAIL.
>>
>> There are cases which one can be sure they never can fail.  But otherwise
>> I find it as a testsuitea bug.
>>
>>
>>> It is expected that
>>> some systems won't have the unwinder hooks.  In the absurd, issuing a FAIL for
>>> these cases would be like issuing FAILs when tests are skipped because
>>> a [istarget "foobar-*-*"] returns false.
>>
>> If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
>> displayed on screen during interactive run.
> 

(last minute editing made me lose a bit here)  For completeness:

> That's not what an XFAIL is for.  XFAIL is when you do
> "print 2+2", you expect "4" to come out, but you know that on
> some broken systems instead "5" comes out, so you XFAIL on those systems,

"some system with broken system libraries or kernel, or some such".  If it
were a GDB bug that only triggers on some systems, but still a GDB bug, it
would be a KFAIL.

> as in, to fix that _wrong result_, you need to fix something else, not GDB,
> but there _is_ something broken that should be fixed.
-- 
Pedro Alves

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Jan Kratochvil]

On Fri, 24 Aug 2012 18:52:46 +0200, Pedro Alves wrote:
> On 08/24/2012 05:18 PM, Jan Kratochvil wrote:
> > If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
> > displayed on screen during interactive run.
> 
> That's not what an XFAIL is for.  XFAIL is when you do
> "print 2+2", you expect "4" to come out, but you know that on
> some broken systems instead "5" comes out, so you XFAIL on those systems,
> as in, to fix that _wrong result_, you need to fix something else, not GDB,
> but there _is_ something broken that should be fixed.

Exactly.  This problem is in libgcc and GDB cannot fix it, so it is is XFAIL.


> The fact is that the check for the unwinder hook actually _succeeded_.
> It just happened that the result of the successful test was
> "no unwinder hook".

This is a matter of opinion how one should look at it.  In your way you can
PASS everything because DejaGNU _succeeded_, just GDB returned wrong result.


> I don't believe this support check should XFAIL on not-top-of-tree-glibc,

This is about gcc, not about glibc.


> just like they shouldn't trigger an XFAIL on Windows or Solaris (if it
> ever runs there).

They should, in the point of view I wrote the testcase.


> Rather, this is why DejaGNU has UNSUPPORTED -- it also
> leaves a line in the .sum file, and is also not displayed on screen during
> interactive run (IIUYC).  If testing for the presence of the unwinder hooks
> fails (as in, we get some unexpected output, like an internal error), then
> we should be issuing in addition an UNRESOLVED instead of UNSUPPORTED
> (we don't do that presently):

But we perfectly understand thy the unwinder hook is missing there, in former
versions of libgcc the hook just was not there.  There is nothing to
investigate on those old libraries.


>  "Declares a test to have an unresolved outcome. unresolved writes in the log file a
>  message beginning with `UNRESOLVED', appending the argument string. This usually means
>  the test did not execute as expected, and a human being must go over results to
>  determine if it passed or failed (and to improve the test case). "

I am aware of these DejaGNU results descriptions.  I read them many times and
I never understood where exactly are the differences between all of
xfail/unsupported/untested/unresolved.  So I no longer try to.

I created some model of how testcases should be written.  Apparently you have
created your model.  Any of such models should be wrritten down and GDB should
stick to it.  I really do not mind which one.


> In isolation, it's mildly worse, because you can't tell whether
> it's a regression, or a new failure.

I spend sometimes hours of time evaluating GDB testsuite results daily, sorry
but after the years I am too well aware what I find easier to evaluate.


> "some system with broken system libraries or kernel, or some such".  If it
> were a GDB bug that only triggers on some systems, but still a GDB bug, it
> would be a KFAIL.

Yes, exactly.


Regards,
Jan

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Yao Qi]

On 08/25/2012 12:18 AM, Jan Kratochvil wrote:
> If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
> displayed on screen during interactive run.

If we do a gdb test, and test is failed because of the misbehaviour, we 
should use XFAIL.  However, in proc 'skip_XXX' in lib/gdb.exp, we check 
the supported feature of system instead of doing a gdb test, so no 
PASS/FAIL/XFAIL should be emitted.

-- 
Yao

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Jan Kratochvil]

On Mon, 27 Aug 2012 12:33:27 +0200, Yao Qi wrote:
> On 08/25/2012 12:18 AM, Jan Kratochvil wrote:
> >If the system does not have unwinder hook it will XFAIL.  XFAIL is not even
> >displayed on screen during interactive run.
> 
> If we do a gdb test, and test is failed because of the misbehaviour,
> we should use XFAIL.

What is "misbehavior" here?


> However, in proc 'skip_XXX' in lib/gdb.exp, we
> check the supported feature of system instead of doing a gdb test,
> so no PASS/FAIL/XFAIL should be emitted.

XFAIL is for "unsupported system feature".

What does XFAIL mean different than "unsupported system feature"?

When will ever be XFAIL emitted if not for "unsupported system feature"?


THanks,
Jan

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Yao Qi]

On 08/27/2012 09:06 PM, Jan Kratochvil wrote:
>> If we do a gdb test, and test is failed because of the misbehaviour,
>> >we should use XFAIL.
> What is "misbehavior" here?
>

I meant "system misbehaviour".

>
>> >However, in proc 'skip_XXX' in lib/gdb.exp, we
>> >check the supported feature of system instead of doing a gdb test,
>> >so no PASS/FAIL/XFAIL should be emitted.
> XFAIL is for "unsupported system feature".
>
> What does XFAIL mean different than "unsupported system feature"?
>

IMO, XFAIL means "unsupported system feature" or "system misbehaviour" 
in my previous reply, but ...

> When will ever be XFAIL emitted if not for "unsupported system feature"?

... it doesn't mean we should use XFAIL for *every* "unsupported system 
feature", IMO.  XFAIL can be used for "unsupported system feature" in 
gdb test, but XFAIL (nor PASS) can not be used in checking the feature 
of system, because checking itself is not a test.  I'd like to call them 
(proc skip_XXX and proc support_XXX in lib/gdb.exp) 'test configuration 
checking' which determines the set of tests to run according to the 
configuration of GDB and other factors.  They should not contribute 
PASS/XFAIL/FAIL/... to test result, because they are not tests.

-- 
Yao

Re: [PATCH 2/2] Remove pass in skip_unwinder_tests

[Jan Kratochvil]

On Mon, 27 Aug 2012 17:14:43 +0200, Yao Qi wrote:
> XFAIL can be used for "unsupported system
> feature" in gdb test, but XFAIL (nor PASS) can not be used in
> checking the feature of system, because checking itself is not a
> test.  I'd like to call them (proc skip_XXX and proc support_XXX in
> lib/gdb.exp) 'test configuration checking' which determines the set
> of tests to run according to the configuration of GDB and other
> factors.  They should not contribute PASS/XFAIL/FAIL/... to test
> result, because they are not tests.

OK, this is a different opinion on what is a test and what is not a test.

If the gdb.sum should contain only the output of the goals of the testfile
then there should be only IIUC for example for gdb.trace/trace-mt.exp:

PASS: gdb.trace/trace-mt.exp: trace on: tfind frame 0
PASS: gdb.trace/trace-mt.exp: trace on: tfind frame 1
PASS: gdb.trace/trace-mt.exp: trace on: tfind
PASS: gdb.trace/trace-mt.exp: trace off: tfind frame 0
PASS: gdb.trace/trace-mt.exp: trace off: tfind frame 1
PASS: gdb.trace/trace-mt.exp: trace off: tfind
PASS: gdb.trace/trace-mt.exp: step over trace: tstop
PASS: gdb.trace/trace-mt.exp: ftrace on: tfind frame 0
PASS: gdb.trace/trace-mt.exp: ftrace on: tfind frame 1
PASS: gdb.trace/trace-mt.exp: ftrace on: tfind
PASS: gdb.trace/trace-mt.exp: ftrace off: tfind frame 0
PASS: gdb.trace/trace-mt.exp: ftrace off: tfind frame 1
PASS: gdb.trace/trace-mt.exp: ftrace off: tfind
PASS: gdb.trace/trace-mt.exp: step over ftrace: tstop

as all the other testcases (43-14==29; see below the full testfile run) are
only supporting there the goals above and they are no real tests on their own.
Like the testing of system features are only supporting the goal testcases of
the testfile.

I see the testcases as a separation of the unstructured GDB output.  As the
matching is done only by regexes the responses are commonly out-of-sync
leading to various racy bugs (like PR testsuite/12649).  Some common markers
(testcases) make the matching easier separating out racy results from real
regressions.

Dropping results of all the supportive testcases I find both a lot of work on
the testsuite, IMO it is not the style of the current testsuite, and
separating out the daily racy results out of it will be even more difficult
than it is.

TBH I do not mind at all, any decision will be always wrong as the primary
problem is that the testsuite does not use structured GDB output with common
parser of the structured output (with current GDB it means GDB does not use MI
for very every command and there is no MI protocol parser in the testsuite).


Thanks,
Jan


PASS: gdb.trace/trace-mt.exp: successfully compiled posix threads test case
PASS: gdb.trace/trace-mt.exp: trace on: set breakpoint always-inserted on
PASS: gdb.trace/trace-mt.exp: trace on: break end
PASS: gdb.trace/trace-mt.exp: trace on: break set_point1
PASS: gdb.trace/trace-mt.exp: trace on: trace set_point1
PASS: gdb.trace/trace-mt.exp: trace on: tstart
PASS: gdb.trace/trace-mt.exp: trace on: continue to set_point1 1
PASS: gdb.trace/trace-mt.exp: trace on: continue to set_point1 2
PASS: gdb.trace/trace-mt.exp: trace on: continue to end
PASS: gdb.trace/trace-mt.exp: trace on: tstop
PASS: gdb.trace/trace-mt.exp: trace on: tfind frame 0
PASS: gdb.trace/trace-mt.exp: trace on: tfind frame 1
PASS: gdb.trace/trace-mt.exp: trace on: tfind
PASS: gdb.trace/trace-mt.exp: trace off: set breakpoint always-inserted off
PASS: gdb.trace/trace-mt.exp: trace off: break end
PASS: gdb.trace/trace-mt.exp: trace off: break set_point1
PASS: gdb.trace/trace-mt.exp: trace off: trace set_point1
PASS: gdb.trace/trace-mt.exp: trace off: tstart
PASS: gdb.trace/trace-mt.exp: trace off: continue to set_point1 1
PASS: gdb.trace/trace-mt.exp: trace off: continue to set_point1 2
PASS: gdb.trace/trace-mt.exp: trace off: continue to end
PASS: gdb.trace/trace-mt.exp: trace off: tstop
PASS: gdb.trace/trace-mt.exp: trace off: tfind frame 0
PASS: gdb.trace/trace-mt.exp: trace off: tfind frame 1
PASS: gdb.trace/trace-mt.exp: trace off: tfind
PASS: gdb.trace/trace-mt.exp: step over trace: set non-stop 0
PASS: gdb.trace/trace-mt.exp: step over trace: break set_point1
PASS: gdb.trace/trace-mt.exp: step over trace: continue to set_point1
PASS: gdb.trace/trace-mt.exp: step over trace: trace *$pc
PASS: gdb.trace/trace-mt.exp: step over trace: tstart
PASS: gdb.trace/trace-mt.exp: step over trace: stepi
PASS: gdb.trace/trace-mt.exp: step over trace: tstop
PASS: gdb.trace/trace-mt.exp: successfully compiled posix threads test case
PASS: gdb.trace/trace-mt.exp: IPA loaded
PASS: gdb.trace/trace-mt.exp: ftrace on: set breakpoint always-inserted on
PASS: gdb.trace/trace-mt.exp: ftrace on: break end
PASS: gdb.trace/trace-mt.exp: ftrace on: break set_point1
PASS: gdb.trace/trace-mt.exp: ftrace on: ftrace set_point1
PASS: gdb.trace/trace-mt.exp: ftrace on: tstart
PASS: gdb.trace/trace-mt.exp: ftrace on: continue to set_point1 1
PASS: gdb.trace/trace-mt.exp: ftrace on: continue to set_point1 2
PASS: gdb.trace/trace-mt.exp: ftrace on: continue to end
PASS: gdb.trace/trace-mt.exp: ftrace on: tstop