URGENT RFR (S): fix for Test8004741.java crashes with SIGSEGV in JDK10-hs nightly (8185273)

[Daniel D. Daugherty]

On 7/31/17 9:43 AM, Aleksey Shipilev wrote:
> On 07/31/2017 05:09 PM, Daniel D. Daugherty wrote:
>> On 7/31/17 8:35 AM, Aleksey Shipilev wrote:
>>>> Webrev URL: http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/
>>> Those changes make sense, thanks.
>> Thanks for the fast review!
>>
>>
>>> It is probably worth mentioning that Threads::parallel_java_threads_do should be in sync with
>>> Threads::possibly_parallel_oops_do? It gets easier to point out the symmetry: possibly_parallel_...
>>> claims all Java threads and the VMThread, so this should also claim the VMThread.
>> We would have to be careful about how we phrase that.
>> Threads::possibly_parallel_oops_do() claims and applies
>> the closure to all the threads it claims.
>>
>> Threads::parallel_java_threads_do() is missing the claim
>> for the VMThread (this bug), but does not apply the
>> closure to the VMThread.
> Yeah. It's just I had to work upwards from the gory details explained in the comment to the actual
> setup for the bug to appear. I think details about ParallelSPCleanupTask, safepoint.cpp, parity,
> etc. are too low-level here, and capture only the current state of affairs. E.g. what if there are
> more callers to parallel_java_threads_do in future? What if Parallel SP cleanup ceases to call it?
> Would the comment get outdated? Does Threads::parallel_java_threads_do make sense without Parallel
> SP cleanup? Yes, it does. Would it make sense to cherry-pick it somewhere else with that comment as
> stated? Not really.
>
> AFAIU, the high-level bug is because we have to claim the same subset of threads on all paths. From
> that, it becomes obvious that if possibly_parallel_java_threads_do claims VMThread, all other paths
> should claim it too.
>
> Something like this:
>
> Threads::parallel_java_threads_do(ThreadClosure* tc) {
>     ...
>
>     // Thread claiming protocol requires us to claim the same interesting threads
>     // on all paths. Notably, Threads::possibly_parallel_threads_do claims all
>     // Java threads *and* the VMThread. To avoid breaking the claiming protocol,
>     // we have to appear to claim VMThread on this path too, even if we would not
>     // process the VMThread oops.
>     VMThread* vmt = VMThread::vm_thread();
>     (void)vmt->claim_oops_do(true, cp);

I like your comment better than mine, with a slight tweak:

    // Thread claiming protocol requires us to claim the same 
interesting threads
    // on all paths. Notably, Threads::possibly_parallel_threads_do 
claims all
    // Java threads *and* the VMThread. To avoid breaking the claiming 
protocol,
    // we have to claim VMThread on this path too, even if we do not 
apply the
    // closure to the VMThread.

>
> ...and then the assert fix would seal the deal.

The assert diffs are applied and tested via JPRT. Please see my
other e-mail on this thread...

Dan


>
> Thanks,
> -Aleksey
>