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

[Daniel D. Daugherty]

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