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

[Aleksey Shipilev]

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

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

Thanks,
-Aleksey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170731/e90de6cf/signature.asc>