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