RFR 8167372: Add code to check for getting oops while thread is in native

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
Hi,

Please review this JDK-10 enhancement for bug JDK-8167372.  As described
in the bug, this fix helps check for naked oops.  The fix was tested
with JCK lang and VM tests, JTReg hotspot and many JTReg JDK tests, M5
tier1 - tier5 tests, and JPRT.

Open Webrev:
http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html

JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8167372

Thanks, Harold

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

George Triantafillou
Hi Harold,

Looks good.

-George

On 11/16/2017 8:38 AM, harold seigel wrote:

> Hi,
>
> Please review this JDK-10 enhancement for bug JDK-8167372.  As
> described in the bug, this fix helps check for naked oops.  The fix
> was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>
> Open Webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8167372
>
> Thanks, Harold
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

Aleksey Shipilev-4
In reply to this post by harold seigel
On 11/16/2017 02:38 PM, harold seigel wrote:
> Hi,
>
> Please review this JDK-10 enhancement for bug JDK-8167372.  As described in the bug, this fix helps
> check for naked oops.  The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html

This looks good.

It probably makes sense to assert that JNIHandle::resolve path that unpacks the naked oops also has
thread in proper state? This would expose failures like in JDK-8191227 [1] better.

Thanks,
-Aleksey

[1] https://bugs.openjdk.java.net/browse/JDK-8191227

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
In reply to this post by George Triantafillou
Thanks George for the review!

Harold


On 11/16/2017 10:01 AM, George Triantafillou wrote:

> Hi Harold,
>
> Looks good.
>
> -George
>
> On 11/16/2017 8:38 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 enhancement for bug JDK-8167372.  As
>> described in the bug, this fix helps check for naked oops.  The fix
>> was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8167372
>>
>> Thanks, Harold
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
In reply to this post by Aleksey Shipilev-4
Hi Aleksey,

Thanks for the suggestion.  I added an assert, that checks for the
proper thread state, to JNIHandles::resolve_impl() but was then unable
to build because of JDK-8191227
<https://bugs.openjdk.java.net/browse/JDK-8191227>. So I'm withdrawing
this fix until that bug is fixed.

I added the text of this new change, showing the added assert, as a
comment to JDK-8167372.

Thanks, Harold


On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:

> On 11/16/2017 02:38 PM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 enhancement for bug JDK-8167372.  As described in the bug, this fix helps
>> check for naked oops.  The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
> This looks good.
>
> It probably makes sense to assert that JNIHandle::resolve path that unpacks the naked oops also has
> thread in proper state? This would expose failures like in JDK-8191227 [1] better.
>
> Thanks,
> -Aleksey
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
In reply to this post by Aleksey Shipilev-4
Please review this updated webrev that adds the assert suggested below
by Aleksey.

    http://cr.openjdk.java.net/~hseigel/bug_8167372.2/webrev/index.html

This update was re-tested with the same tests as the original webrev.

Note that this change will be pushed post JDK-10.

Thanks, Harold


On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:

> On 11/16/2017 02:38 PM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 enhancement for bug JDK-8167372.  As described in the bug, this fix helps
>> check for naked oops.  The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
> This looks good.
>
> It probably makes sense to assert that JNIHandle::resolve path that unpacks the naked oops also has
> thread in proper state? This would expose failures like in JDK-8191227 [1] better.
>
> Thanks,
> -Aleksey
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

coleen.phillimore
Harold,
This looks good.
thanks!
Coleen

On 11/29/17 8:40 AM, harold seigel wrote:

> Please review this updated webrev that adds the assert suggested below
> by Aleksey.
>
> http://cr.openjdk.java.net/~hseigel/bug_8167372.2/webrev/index.html
>
> This update was re-tested with the same tests as the original webrev.
>
> Note that this change will be pushed post JDK-10.
>
> Thanks, Harold
>
>
> On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:
>> On 11/16/2017 02:38 PM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-10 enhancement for bug JDK-8167372.  As
>>> described in the bug, this fix helps
>>> check for naked oops.  The fix was tested with JCK lang and VM
>>> tests, JTReg hotspot and many JTReg
>>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>
>>> Open Webrev:
>>> http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>> This looks good.
>>
>> It probably makes sense to assert that JNIHandle::resolve path that
>> unpacks the naked oops also has
>> thread in proper state? This would expose failures like in
>> JDK-8191227 [1] better.
>>
>> Thanks,
>> -Aleksey
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
Thanks Coleen!

Harold


On 12/11/2017 1:42 PM, [hidden email] wrote:

> Harold,
> This looks good.
> thanks!
> Coleen
>
> On 11/29/17 8:40 AM, harold seigel wrote:
>> Please review this updated webrev that adds the assert suggested
>> below by Aleksey.
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8167372.2/webrev/index.html
>>
>> This update was re-tested with the same tests as the original webrev.
>>
>> Note that this change will be pushed post JDK-10.
>>
>> Thanks, Harold
>>
>>
>> On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:
>>> On 11/16/2017 02:38 PM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-10 enhancement for bug JDK-8167372. As
>>>> described in the bug, this fix helps
>>>> check for naked oops.  The fix was tested with JCK lang and VM
>>>> tests, JTReg hotspot and many JTReg
>>>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>
>>>> Open Webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>>> This looks good.
>>>
>>> It probably makes sense to assert that JNIHandle::resolve path that
>>> unpacks the naked oops also has
>>> thread in proper state? This would expose failures like in
>>> JDK-8191227 [1] better.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

Jiangli Zhou
In reply to this post by harold seigel
Hi Harold,

Looks good.

Thanks,
Jiangli

> On Nov 29, 2017, at 5:40 AM, harold seigel <[hidden email]> wrote:
>
> Please review this updated webrev that adds the assert suggested below by Aleksey.
>
>   http://cr.openjdk.java.net/~hseigel/bug_8167372.2/webrev/index.html
>
> This update was re-tested with the same tests as the original webrev.
>
> Note that this change will be pushed post JDK-10.
>
> Thanks, Harold
>
>
> On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:
>> On 11/16/2017 02:38 PM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-10 enhancement for bug JDK-8167372.  As described in the bug, this fix helps
>>> check for naked oops.  The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
>>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>
>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>> This looks good.
>>
>> It probably makes sense to assert that JNIHandle::resolve path that unpacks the naked oops also has
>> thread in proper state? This would expose failures like in JDK-8191227 [1] better.
>>
>> Thanks,
>> -Aleksey
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8167372: Add code to check for getting oops while thread is in native

harold seigel
Thanks Jiangli!

Harold

On 12/11/2017 1:49 PM, Jiangli Zhou wrote:

> Hi Harold,
>
> Looks good.
>
> Thanks,
> Jiangli
>
>> On Nov 29, 2017, at 5:40 AM, harold seigel <[hidden email]> wrote:
>>
>> Please review this updated webrev that adds the assert suggested below by Aleksey.
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8167372.2/webrev/index.html
>>
>> This update was re-tested with the same tests as the original webrev.
>>
>> Note that this change will be pushed post JDK-10.
>>
>> Thanks, Harold
>>
>>
>> On 11/16/2017 10:06 AM, Aleksey Shipilev wrote:
>>> On 11/16/2017 02:38 PM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this JDK-10 enhancement for bug JDK-8167372.  As described in the bug, this fix helps
>>>> check for naked oops.  The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg
>>>> JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>
>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8167372/webrev/index.html
>>> This looks good.
>>>
>>> It probably makes sense to assert that JNIHandle::resolve path that unpacks the naked oops also has
>>> thread in proper state? This would expose failures like in JDK-8191227 [1] better.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8191227
>>>