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

[Pedro Alves <palves@redhat.com>]

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