Re: [PATCH] Improve analysis of racy testcases

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, March 01 2016, Pedro Alves wrote:

> On 02/28/2016 09:44 PM, Sergio Durigan Junior wrote:
>> On Thursday, February 25 2016, Pedro Alves wrote:
>
>>
>>> I've now run "make check -j8 RACY_ITER=3" and got this:
>>>
>>> $ cat testsuite/racy.sum
>>> gdb.threads/attach-into-signal.exp: nonthreaded: attempt 1: attach (pass 1), pending signal catch
>>> gdb.threads/attach-into-signal.exp: nonthreaded: attempt 1: attach (pass 2), pending signal catch
>>> gdb.threads/attach-into-signal.exp: nonthreaded: attempt 4: attach (pass 1), pending signal catch
>>> gdb.threads/attach-into-signal.exp: nonthreaded: attempt 6: attach (pass 2), pending signal catch
>>> gdb.threads/attach-into-signal.exp: threaded: attempt 1: attach (pass 1), pending signal catch
>>> gdb.threads/attach-into-signal.exp: threaded: attempt 3: attach (pass 1), pending signal catch
>>> gdb.threads/attach-into-signal.exp: threaded: attempt 3: attach (pass 2), pending signal catch
>>> gdb.threads/attach-many-short-lived-threads.exp: iter 5: attach
>>> gdb.threads/attach-many-short-lived-threads.exp: iter 5: attach (EPERM)
>>> gdb.threads/attach-many-short-lived-threads.exp: iter 9: attach
>>> gdb.threads/attach-many-short-lived-threads.exp: iter 9: attach (EPERM)
>>> gdb.threads/fork-plus-threads.exp: detach-on-fork=off: only inferior 1 left
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: inferior 1 exited
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: inferior 1 exited (prompt) (PRMS: gdb/18749)
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=0: no threads left
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: inferior 1 exited
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: inferior 1 exited (memory error) (PRMS: gdb/18749)
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: no threads left
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: inferior 1 exited
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: inferior 1 exited (prompt) (PRMS: gdb/18749)
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=0: no threads left
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited (timeout) (PRMS: gdb/18749)
>>> gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: no threads left
>>> gdb.threads/watchpoint-fork.exp: child: multithreaded: finish
>>> gdb.threads/watchpoint-fork.exp: child: multithreaded: watchpoint A after the second fork
>>> gdb.threads/watchpoint-fork.exp: child: multithreaded: watchpoint B after the second fork
>>>
>
>>> The gdb.threads/process-dies-while-handling-bp.exp ones are actually:
>>>
>>>   -PASS: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited
>>>   +KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=on: cond_bp_target=1: inferior 1 exited (prompt) (PRMS: gdb/18749)
>>>
>>> Test sum diffing should probably strip away tail ()s, and ignore PASS->KFAIL->PASS.
>>
>> I thought about stripping tail ()s away before comparing the names, but
>> the problem is that maybe we'll miss a test that actually writes
>> something meaningful inside the parentheses.  There isn't a strong
>> convention advising testcase writers to not do that.  What do you think?
>
> I think we should have that convention.  We already largely implicitly
> have it, exactly because of "(PRMS: gdb/NNN)", "(timeout)" or "(eof)",
> etc.
>
> IOW, I think this should be interpreted as a regression in the
> "whatever test" test:
>
>  -PASS: gdb.base/foo.exp: whatever test
>  +FAIL: gdb.base/foo.exp: whatever test (timeout)
>
> If that actually strips something meaningful, I'd just file it under
> the same bucket as non-unique test names, and fix it by tweaking the
> test message.

Alright, makes sense to me.  I went ahead and also updated the wiki with
this rule:

  <https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages>

>>> The only thing I do wish we should do, is use the fruits of this
>>> to somehow mark racy tests in the testsuite itself, instead of only
>>> making the buildbot ignore them, so that local development benefits
>>> as well.
>>
>> I totally agree, and also spent some time thinking about this problem,
>> but I don't see an easy solution for that.  Racy testcases vary wildly
>> between targets, GDB vs. gdbserver, CFLAGS, etc.
>> We would have to maintain several lists of racy tests,
>
> I wouldn't want to maintain separate lists at all.  Instead, I'd want
> to mark testcases themselves with something like setup_kfail.
> Testcases already call those depending on target/arch, I see no
> difference.
>
> Perhaps a problem with setup_kfail is that that generates a KPASS when
> the race doesn't trigger.  We could add a new setup_racy_kfail that
> generates a PASS on success and KFAIL on failure, but never a KPASS.
> Perhaps we could have a racy_test_scope in the spirit of
> with_test_prefix, which would automatically mark all tests in the
> scope
> as racy, but I'm not sure we do need or want that.

I liked the idea of a setup_racy_kfail (not sure about the
racy_test_scope thing...).  I will take a look at implementing that
later.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/