Re: [PATCH] Improve analysis of racy testcases

[Pedro Alves <palves@redhat.com>]

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.

>> 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.

> and even this wouldn't be 100%
> trustworthy because the racy tests also vary depending on the machine
> load...

Varying the load does not make the _set_ of racy tests vary.  It only
varies the chances of a racy test in a set of racy tests actual trigger
the race.  A test in that set is still racy regardless.

Thanks,
Pedro Alves