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

[Roman Kennke]

Looks good!

Roman (not an official reviewer)

PS: I've filed JDK-8185580: Refactor
Threads::possibly_parallel_oops_do() to use
Threads::parallel_java_threads_do()
<https://bugs.openjdk.java.net/browse/JDK-8185580> to take care of the
rest for when I get back from vacation.


> Latest webrev: http://cr.openjdk.java.net/~dcubed/8185273-webrev/1/
>
> Only src/share/vm/runtime/thread.cpp is changed relative to round 0:
>
> - Revised the comment in Threads::parallel_java_threads_do.
> - Added the assert to Threads::assert_all_threads_claimed().
>
> Comments, questions and feedback are welcome.
>
> Dan
>
>
> On 7/31/17 10:47 AM, Daniel D. Daugherty wrote:
>> 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
>>>
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170731/be737591/attachment.htm>