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

[Daniel D. Daugherty]

On 7/31/17 8:42 AM, Roman Kennke wrote:
> Hi Dan,
>
> You could also do_thread() on the VMThread, and let the ThreadClosurer
> filter it. I believe the ThreadClosure in safepoint.cpp (currently only
> consumer) already filters it. This would make it consistent with
> Threads::possibly_parallel_oops_do() (and infact, that latter method
> could just use the new Threads::parallel_java_threads_do() but this is
> beyond the scope). I leave that to you to decide though.

I'm good with just adding the missing part of the "claims" protocol.
I'm not comfortable with applying the closure to the VMThread since
I'm just visiting the GC sandbox as it were... :-)

> I'd also include the fix for assert_all_threads_claimed() because it's
> related (and the cause for me not noticing this slip). But that is up to
> you too. ;-)

Yes, I plan to kick off another JPRT run with the additional fix for
assert_all_threads_claimed()... If that goes well, then I'll include
it...

>
> In other words, thumbs up, unless you want to add the above points.

Thanks for the review!


> And sorry for making such a mess!

No worries. We have it covered.

Dan

P.S.
Reminder: you're supposed to be on vacation! (But I do appreciate
you taking the time to chime in here...)


>
> Roman
>
>> Greetings,
>>
>> I have a fix for the following P1 JDK10-hs integration_blocker bug:
>>
>>      8185273 Test8004741.java crashes with SIGSEGV in JDK10-hs nightly
>>      https://bugs.openjdk.java.net/browse/JDK-8185273
>>
>> The fix is 2 lines and the comment describing the fix is 4 lines:
>>
>> src/share/vm/runtime/thread.cpp:
>>
>> L3388: void Threads::parallel_java_threads_do(ThreadClosure* tc) {
>> <snip>
>> L3395:   // This function is used by ParallelSPCleanupTask in
>> safepoint.cpp
>> L3396:   // for cleaning up JavaThreads, but we have to keep the
>> VMThread's
>> L3397:   // _oops_do_parity field in sync so we don't miss a parallel
>> GC on
>> L3398:   // the VMThread.
>> L3399:   VMThread* vmt = VMThread::vm_thread();
>> L3400:   (void)vmt->claim_oops_do(true, cp);
>>
>> I'm also including some new logging for the VMThread (tag == 'vmthread')
>> that came in useful during this bug hunt. Lastly, I've fixed a few minor
>> typos that I ran across in the areas where I was hunting.
>>
>> Webrev URL: http://cr.openjdk.java.net/~dcubed/8185273-webrev/0/
>>
>> There's lots of discussion in the bug. The evaluation comment that I
>> added
>> on Sunday, July 30 is probably the most complete and hopefully the
>> most clear.
>>
>> For context, here's the webrev for 8180932 and another bug fix:
>>
>> http://cr.openjdk.java.net/~rkennke/8180932/webrev.18/
>> http://cr.openjdk.java.net/~rkennke/8185102/webrev.01/
>>
>>
>> Testing:
>>    - JPRT
>>    - Test8004741.java has been running in a forever loop with 'fastdebug'
>>      bits (17200+ iterations) and 'slowdebug' bits (13400+ iterations)
>>
>> Comments, questions and feedback are welcome.
>>
>> Dan
>>
>> P.S.
>> Roman and I were also thinking about updating
>> Threads::assert_all_threads_claimed() to verify
>> that the VMThread is also claimed... Obviously
>> that's not part of the current patch...
>