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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
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...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
On 07/31/2017 04:24 PM, Daniel D. Daugherty wrote:

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

Those changes make sense, thanks.

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.

-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Roman Kennke-6
In reply to this post by Daniel D. Daugherty
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'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. ;-)

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

And sorry for making such a mess!

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by Aleksey Shipilev-4
On 7/31/17 8:35 AM, Aleksey Shipilev wrote:

> On 07/31/2017 04:24 PM, Daniel D. Daugherty wrote:
>> 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/
> 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.

I think we'll be in good shape once
Threads::assert_all_threads_claimed() is updated to make
sure that the VMThread is claimed. Once that happens, anyone
that uses StrongRootsScope to manage the "claim" protocol
will have a sanity check in place.

Dan



>
> -Aleksey
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by Roman Kennke-6
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...
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
In reply to this post by Daniel D. Daugherty
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by Daniel D. Daugherty
On 7/31/17 9:14 AM, Daniel D. Daugherty wrote:

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

Here's the addition of the assert:

$ diff -C 6 src/share/vm/runtime/thread.cpp.cr0
src/share/vm/runtime/thread.cpp
*** src/share/vm/runtime/thread.cpp.cr0 Sun Jul 30 18:49:06 2017
--- src/share/vm/runtime/thread.cpp     Mon Jul 31 08:22:47 2017
***************
*** 4360,4371 ****
--- 4360,4375 ----
   void Threads::assert_all_threads_claimed() {
     ALL_JAVA_THREADS(p) {
       const int thread_parity = p->oops_do_parity();
       assert((thread_parity == _thread_claim_parity),
              "Thread " PTR_FORMAT " has incorrect parity %d != %d",
p2i(p), thread_parity, _thread_claim_parity);
     }
+   VMThread* vmt = VMThread::vm_thread();
+   const int thread_parity = vmt->oops_do_parity();
+   assert((thread_parity == _thread_claim_parity),
+          "VMThread " PTR_FORMAT " has incorrect parity %d != %d",
p2i(vmt), thread_parity, _thread_claim_parity);
   }
   #endif // ASSERT

   void Threads::possibly_parallel_oops_do(bool is_par, OopClosure* f,
CodeBlobClosure* cf) {
     int cp = Threads::thread_claim_parity();
     ALL_JAVA_THREADS(p) {


I ran a test JPRT job and there were no problems.

Aleksey and Roman, are you two good with the assert?


Dan


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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Roman Kennke-6
In reply to this post by Daniel D. Daugherty
Am 31.07.2017 um 17:14 schrieb 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... :-)

Ok. I'll do it in a followup then. IMO it would be best if there is
*one* place that does the claiming protocol (i.e.
parallel_java_threads_do() which should probably be renamed to
parallel_threads_do() ), and have possibly_parallel_oops_do() use that
via a private ThreadClosure. Best to do it asap, as long as there's only
1 user it's easy to see that it's correct ;-)


>> 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...
Great!

>
> P.S.
> Reminder: you're supposed to be on vacation! (But I do appreciate
> you taking the time to chime in here...)
Yeah, I should be at the Atlantic already, but my son got sick and we
have to delay travel a little bit...

And in reply to Aleksey: yes there will be more callers of
Threads::parallel_java_threads_do() in the future :-) We've got one in
Shenandoah already...

Thanks for doing this!
Cheers,
Roman

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by Aleksey Shipilev-4
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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by Roman Kennke-6
On 7/31/17 10:46 AM, Roman Kennke wrote:

> Am 31.07.2017 um 17:14 schrieb 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... :-)
> Ok. I'll do it in a followup then. IMO it would be best if there is
> *one* place that does the claiming protocol (i.e.
> parallel_java_threads_do() which should probably be renamed to
> parallel_threads_do() ), and have possibly_parallel_oops_do() use that
> via a private ThreadClosure. Best to do it asap, as long as there's only
> 1 user it's easy to see that it's correct ;-)

Thanks. I can file a follow up bug:

     cleanup parallel_java_threads_do() and possibly_parallel_oops_do()

and assign it to you if you like...


>>> 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...
> Great!

Done. And I sent out the diffs...


>> P.S.
>> Reminder: you're supposed to be on vacation! (But I do appreciate
>> you taking the time to chime in here...)
> Yeah, I should be at the Atlantic already, but my son got sick and we
> have to delay travel a little bit...

Hope your son gets well soon...


> And in reply to Aleksey: yes there will be more callers of
> Threads::parallel_java_threads_do() in the future :-) We've got one in
> Shenandoah already...
>
> Thanks for doing this!
> Cheers,
> Roman
>

Dan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
In reply to this post by 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
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Roman Kennke-6
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
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
Thanks Roman!

Dan


On 7/31/17 11:10 AM, Roman Kennke wrote:

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aleksey Shipilev-4
In reply to this post by Daniel D. Daugherty
On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
> 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.

Looks good!

-Aleksey

P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
Thanks for the re-review! (and for the reworded comment...)

Dan

On 7/31/17 11:24 AM, Aleksey Shipilev wrote:

> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>> 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.
> Looks good!
>
> -Aleksey
>
> P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vladimir Kozlov
Dan

Can you put new code which used for assert check under #ifdef ASSERT to avoid side effects in product code?

Thanks
Vladimir

> On Jul 31, 2017, at 10:26 AM, Daniel D. Daugherty <[hidden email]> wrote:
>
> Thanks for the re-review! (and for the reworded comment...)
>
> Dan
>
>> On 7/31/17 11:24 AM, Aleksey Shipilev wrote:
>>> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>>> 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.
>> Looks good!
>>
>> -Aleksey
>>
>> P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
On 7/31/17 12:39 PM, Vladimir Kozlov wrote:
> Dan
>
> Can you put new code which used for assert check under #ifdef ASSERT to avoid side effects in product code?

That entire function is in a #ifdef ASSERT:

4360 #ifdef ASSERT
4361 void Threads::assert_all_threads_claimed() {
4362   ALL_JAVA_THREADS(p) {
4363     const int thread_parity = p->oops_do_parity();
4364     assert((thread_parity == _thread_claim_parity),
4365            "Thread " PTR_FORMAT " has incorrect parity %d != %d",
p2i(p), thread_parity, _thread_claim_parity);
4366   }
4367   VMThread* vmt = VMThread::vm_thread();
4368   const int thread_parity = vmt->oops_do_parity();
4369   assert((thread_parity == _thread_claim_parity),
4370          "VMThread " PTR_FORMAT " has incorrect parity %d != %d",
p2i(vmt), thread_parity, _thread_claim_parity);
4371 }
4372 #endif // ASSERT

Thanks for the review!

Dan


>
> Thanks
> Vladimir
>
>> On Jul 31, 2017, at 10:26 AM, Daniel D. Daugherty <[hidden email]> wrote:
>>
>> Thanks for the re-review! (and for the reworded comment...)
>>
>> Dan
>>
>>> On 7/31/17 11:24 AM, Aleksey Shipilev wrote:
>>>> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>>>> 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.
>>> Looks good!
>>>
>>> -Aleksey
>>>
>>> P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Vladimir Kozlov
This is what happens when you do review on phone ;)
Sorry for noise. Looks good.

Vladimir

Sent from my iPhone

> On Jul 31, 2017, at 11:49 AM, Daniel D. Daugherty <[hidden email]> wrote:
>
>> On 7/31/17 12:39 PM, Vladimir Kozlov wrote:
>> Dan
>>
>> Can you put new code which used for assert check under #ifdef ASSERT to avoid side effects in product code?
>
> That entire function is in a #ifdef ASSERT:
>
> 4360 #ifdef ASSERT
> 4361 void Threads::assert_all_threads_claimed() {
> 4362   ALL_JAVA_THREADS(p) {
> 4363     const int thread_parity = p->oops_do_parity();
> 4364     assert((thread_parity == _thread_claim_parity),
> 4365            "Thread " PTR_FORMAT " has incorrect parity %d != %d", p2i(p), thread_parity, _thread_claim_parity);
> 4366   }
> 4367   VMThread* vmt = VMThread::vm_thread();
> 4368   const int thread_parity = vmt->oops_do_parity();
> 4369   assert((thread_parity == _thread_claim_parity),
> 4370          "VMThread " PTR_FORMAT " has incorrect parity %d != %d", p2i(vmt), thread_parity, _thread_claim_parity);
> 4371 }
> 4372 #endif // ASSERT
>
> Thanks for the review!
>
> Dan
>
>
>>
>> Thanks
>> Vladimir
>>
>>> On Jul 31, 2017, at 10:26 AM, Daniel D. Daugherty <[hidden email]> wrote:
>>>
>>> Thanks for the re-review! (and for the reworded comment...)
>>>
>>> Dan
>>>
>>>>> On 7/31/17 11:24 AM, Aleksey Shipilev wrote:
>>>>> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>>>>> 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.
>>>> Looks good!
>>>>
>>>> -Aleksey
>>>>
>>>> P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.
>>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
Thanks!

Dan


On 7/31/17 2:14 PM, Vladimir Kozlov wrote:

> This is what happens when you do review on phone ;)
> Sorry for noise. Looks good.
>
> Vladimir
>
> Sent from my iPhone
>
>> On Jul 31, 2017, at 11:49 AM, Daniel D. Daugherty <[hidden email]> wrote:
>>
>>> On 7/31/17 12:39 PM, Vladimir Kozlov wrote:
>>> Dan
>>>
>>> Can you put new code which used for assert check under #ifdef ASSERT to avoid side effects in product code?
>> That entire function is in a #ifdef ASSERT:
>>
>> 4360 #ifdef ASSERT
>> 4361 void Threads::assert_all_threads_claimed() {
>> 4362   ALL_JAVA_THREADS(p) {
>> 4363     const int thread_parity = p->oops_do_parity();
>> 4364     assert((thread_parity == _thread_claim_parity),
>> 4365            "Thread " PTR_FORMAT " has incorrect parity %d != %d", p2i(p), thread_parity, _thread_claim_parity);
>> 4366   }
>> 4367   VMThread* vmt = VMThread::vm_thread();
>> 4368   const int thread_parity = vmt->oops_do_parity();
>> 4369   assert((thread_parity == _thread_claim_parity),
>> 4370          "VMThread " PTR_FORMAT " has incorrect parity %d != %d", p2i(vmt), thread_parity, _thread_claim_parity);
>> 4371 }
>> 4372 #endif // ASSERT
>>
>> Thanks for the review!
>>
>> Dan
>>
>>
>>> Thanks
>>> Vladimir
>>>
>>>> On Jul 31, 2017, at 10:26 AM, Daniel D. Daugherty <[hidden email]> wrote:
>>>>
>>>> Thanks for the re-review! (and for the reworded comment...)
>>>>
>>>> Dan
>>>>
>>>>>> On 7/31/17 11:24 AM, Aleksey Shipilev wrote:
>>>>>> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>>>>>> 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.
>>>>> Looks good!
>>>>>
>>>>> -Aleksey
>>>>>
>>>>> P.S. Roman: I'm going to cherry-pick that to Shenandoah after this lands to jdk10/hs.
>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel D. Daugherty
Final local testing numbers for this fix:

20062 runs on slowdebug bits; of those, 802 were in the right
     sequence of VM-ops for the crash
28523 runs on fastdebug bits; of those, 1915 were in the right
     sequence of VM-ops for the crash

Dan


On 7/31/17 2:56 PM, Daniel D. Daugherty wrote:

> Thanks!
>
> Dan
>
>
> On 7/31/17 2:14 PM, Vladimir Kozlov wrote:
>> This is what happens when you do review on phone ;)
>> Sorry for noise. Looks good.
>>
>> Vladimir
>>
>> Sent from my iPhone
>>
>>> On Jul 31, 2017, at 11:49 AM, Daniel D. Daugherty
>>> <[hidden email]> wrote:
>>>
>>>> On 7/31/17 12:39 PM, Vladimir Kozlov wrote:
>>>> Dan
>>>>
>>>> Can you put new code which used for assert check under #ifdef
>>>> ASSERT to avoid side effects in product code?
>>> That entire function is in a #ifdef ASSERT:
>>>
>>> 4360 #ifdef ASSERT
>>> 4361 void Threads::assert_all_threads_claimed() {
>>> 4362   ALL_JAVA_THREADS(p) {
>>> 4363     const int thread_parity = p->oops_do_parity();
>>> 4364     assert((thread_parity == _thread_claim_parity),
>>> 4365            "Thread " PTR_FORMAT " has incorrect parity %d !=
>>> %d", p2i(p), thread_parity, _thread_claim_parity);
>>> 4366   }
>>> 4367   VMThread* vmt = VMThread::vm_thread();
>>> 4368   const int thread_parity = vmt->oops_do_parity();
>>> 4369   assert((thread_parity == _thread_claim_parity),
>>> 4370          "VMThread " PTR_FORMAT " has incorrect parity %d !=
>>> %d", p2i(vmt), thread_parity, _thread_claim_parity);
>>> 4371 }
>>> 4372 #endif // ASSERT
>>>
>>> Thanks for the review!
>>>
>>> Dan
>>>
>>>
>>>> Thanks
>>>> Vladimir
>>>>
>>>>> On Jul 31, 2017, at 10:26 AM, Daniel D. Daugherty
>>>>> <[hidden email]> wrote:
>>>>>
>>>>> Thanks for the re-review! (and for the reworded comment...)
>>>>>
>>>>> Dan
>>>>>
>>>>>>> On 7/31/17 11:24 AM, Aleksey Shipilev wrote:
>>>>>>> On 07/31/2017 07:07 PM, Daniel D. Daugherty wrote:
>>>>>>> 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.
>>>>>> Looks good!
>>>>>>
>>>>>> -Aleksey
>>>>>>
>>>>>> P.S. Roman: I'm going to cherry-pick that to Shenandoah after
>>>>>> this lands to jdk10/hs.
>>>>>>
>
>

Loading...