RFR: 8191870: Remove badJNIHandle

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

RFR: 8191870: Remove badJNIHandle

Kim Barrett
Please review this change to eliminate badJNIHandle and the special
debug-only handling of it when resolving JNI handles.

For the intended purpose of catching bad JNI handle usage while
sometimes normalizing some to NULL, we mostly don't need such a
special value. We can instead use NULL directly, and get largely the
same effect.

By filling blocks with NULL, rather than badJNIHandle, we avoid the
difficulties related to the Access protocol (see the CR).

A NULL value is sufficient for local and non-weak global handles.  A
JNI handle is never allocated to refer to a NULL value; instead, a
NULL handle is used.  resolve and friends already treat the case of a
NULL pointee specially (asserting, except in the external guard case,
where it is allowed to pass through).

Only weak global handles lose the protection afforded by badJNIHandle.
(This is because, unlike local and global handles, a weak global
handle can refer to a NULL value, because it may have been cleared by
the GC.)  That protection was already fairly minimal for such handles,
even when using badJNIHandle.  That protection only applied for
entries in a newly allocated block that have never yet been allocated.
Blocks in the global lists are currently never released, so the
setting to a "bad" value for released handles never occurs for weak
global handles.  So the most interesting use-case for badJNIHandle
(identifying uses of stale handles) doesn't even apply.

There is one other use of badJNIHandle: throw_unsatisfied_link_error.
This function isn't currently called, only used as a marker.  The
discussion in bug 4524377, where that function was added, suggested
using badAddress, so we're replacing badJNIHandle with badAddress.

CR:
https://bugs.openjdk.java.net/browse/JDK-8191870

Webrev:
http://cr.openjdk.java.net/~kbarrett/8191870/open.00/

Testing:
Mach5 hs-tier1-3
tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Erik Österlund-2
Hi Kim,

This change is absolutely fantastic.
No more nightmares about zapped JNI handle values. Thank you for doing this.

Looks magnificent.

Thanks,
/Erik

> On 25 Nov 2017, at 06:35, Kim Barrett <[hidden email]> wrote:
>
> Please review this change to eliminate badJNIHandle and the special
> debug-only handling of it when resolving JNI handles.
>
> For the intended purpose of catching bad JNI handle usage while
> sometimes normalizing some to NULL, we mostly don't need such a
> special value. We can instead use NULL directly, and get largely the
> same effect.
>
> By filling blocks with NULL, rather than badJNIHandle, we avoid the
> difficulties related to the Access protocol (see the CR).
>
> A NULL value is sufficient for local and non-weak global handles.  A
> JNI handle is never allocated to refer to a NULL value; instead, a
> NULL handle is used.  resolve and friends already treat the case of a
> NULL pointee specially (asserting, except in the external guard case,
> where it is allowed to pass through).
>
> Only weak global handles lose the protection afforded by badJNIHandle.
> (This is because, unlike local and global handles, a weak global
> handle can refer to a NULL value, because it may have been cleared by
> the GC.)  That protection was already fairly minimal for such handles,
> even when using badJNIHandle.  That protection only applied for
> entries in a newly allocated block that have never yet been allocated.
> Blocks in the global lists are currently never released, so the
> setting to a "bad" value for released handles never occurs for weak
> global handles.  So the most interesting use-case for badJNIHandle
> (identifying uses of stale handles) doesn't even apply.
>
> There is one other use of badJNIHandle: throw_unsatisfied_link_error.
> This function isn't currently called, only used as a marker.  The
> discussion in bug 4524377, where that function was added, suggested
> using badAddress, so we're replacing badJNIHandle with badAddress.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8191870
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/
>
> Testing:
> Mach5 hs-tier1-3
> tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Daniel D. Daugherty
In reply to this post by Kim Barrett
On 11/25/17 12:35 AM, Kim Barrett wrote:

> Please review this change to eliminate badJNIHandle and the special
> debug-only handling of it when resolving JNI handles.
>
> For the intended purpose of catching bad JNI handle usage while
> sometimes normalizing some to NULL, we mostly don't need such a
> special value. We can instead use NULL directly, and get largely the
> same effect.
>
> By filling blocks with NULL, rather than badJNIHandle, we avoid the
> difficulties related to the Access protocol (see the CR).
>
> A NULL value is sufficient for local and non-weak global handles.  A
> JNI handle is never allocated to refer to a NULL value; instead, a
> NULL handle is used.  resolve and friends already treat the case of a
> NULL pointee specially (asserting, except in the external guard case,
> where it is allowed to pass through).
>
> Only weak global handles lose the protection afforded by badJNIHandle.
> (This is because, unlike local and global handles, a weak global
> handle can refer to a NULL value, because it may have been cleared by
> the GC.)  That protection was already fairly minimal for such handles,
> even when using badJNIHandle.  That protection only applied for
> entries in a newly allocated block that have never yet been allocated.
> Blocks in the global lists are currently never released, so the
> setting to a "bad" value for released handles never occurs for weak
> global handles.  So the most interesting use-case for badJNIHandle
> (identifying uses of stale handles) doesn't even apply.
>
> There is one other use of badJNIHandle: throw_unsatisfied_link_error.
> This function isn't currently called, only used as a marker.  The
> discussion in bug 4524377, where that function was added, suggested
> using badAddress, so we're replacing badJNIHandle with badAddress.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8191870
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/

src/hotspot/share/runtime/globals.hpp
     No comments.

src/hotspot/share/runtime/jniHandles.cpp
     No comments.

src/hotspot/share/runtime/jniHandles.hpp
     No comments.

src/hotspot/share/runtime/sharedRuntime.cpp
     No comments.

src/hotspot/share/utilities/globalDefinitions.hpp
     No comments.

Thumbs up!

Thanks for leaving JNIHandles::resolve_external_guard() so
that JVM/TI can still do its (limited) bad parameter checks.

Dan


>
> Testing:
> Mach5 hs-tier1-3
> tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

coleen.phillimore
In reply to this post by Kim Barrett

Hi, This looks good.

http://cr.openjdk.java.net/~kbarrett/8191870/open.00/src/hotspot/share/runtime/globals.hpp.udiff.html

Is there a point to this command line option?  It's a develop option-
can we just remove it?

Thanks,
Coleen

On 11/25/17 12:35 AM, Kim Barrett wrote:

> Please review this change to eliminate badJNIHandle and the special
> debug-only handling of it when resolving JNI handles.
>
> For the intended purpose of catching bad JNI handle usage while
> sometimes normalizing some to NULL, we mostly don't need such a
> special value. We can instead use NULL directly, and get largely the
> same effect.
>
> By filling blocks with NULL, rather than badJNIHandle, we avoid the
> difficulties related to the Access protocol (see the CR).
>
> A NULL value is sufficient for local and non-weak global handles.  A
> JNI handle is never allocated to refer to a NULL value; instead, a
> NULL handle is used.  resolve and friends already treat the case of a
> NULL pointee specially (asserting, except in the external guard case,
> where it is allowed to pass through).
>
> Only weak global handles lose the protection afforded by badJNIHandle.
> (This is because, unlike local and global handles, a weak global
> handle can refer to a NULL value, because it may have been cleared by
> the GC.)  That protection was already fairly minimal for such handles,
> even when using badJNIHandle.  That protection only applied for
> entries in a newly allocated block that have never yet been allocated.
> Blocks in the global lists are currently never released, so the
> setting to a "bad" value for released handles never occurs for weak
> global handles.  So the most interesting use-case for badJNIHandle
> (identifying uses of stale handles) doesn't even apply.
>
> There is one other use of badJNIHandle: throw_unsatisfied_link_error.
> This function isn't currently called, only used as a marker.  The
> discussion in bug 4524377, where that function was added, suggested
> using badAddress, so we're replacing badJNIHandle with badAddress.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8191870
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/
>
> Testing:
> Mach5 hs-tier1-3
> tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Daniel D. Daugherty
Because it is a 'trueInDebug' option, we only do the ZAP'ing in non-product
builds... so we don't take the time to ZAP in product bits. As for the
ZAP'ing,
it helps us diagnose stale JNI handle usage...

Dan


On 11/27/17 9:53 AM, [hidden email] wrote:

>
> Hi, This looks good.
>
> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/src/hotspot/share/runtime/globals.hpp.udiff.html 
>
>
> Is there a point to this command line option?  It's a develop option-
> can we just remove it?
>
> Thanks,
> Coleen
>
> On 11/25/17 12:35 AM, Kim Barrett wrote:
>> Please review this change to eliminate badJNIHandle and the special
>> debug-only handling of it when resolving JNI handles.
>>
>> For the intended purpose of catching bad JNI handle usage while
>> sometimes normalizing some to NULL, we mostly don't need such a
>> special value. We can instead use NULL directly, and get largely the
>> same effect.
>>
>> By filling blocks with NULL, rather than badJNIHandle, we avoid the
>> difficulties related to the Access protocol (see the CR).
>>
>> A NULL value is sufficient for local and non-weak global handles.  A
>> JNI handle is never allocated to refer to a NULL value; instead, a
>> NULL handle is used.  resolve and friends already treat the case of a
>> NULL pointee specially (asserting, except in the external guard case,
>> where it is allowed to pass through).
>>
>> Only weak global handles lose the protection afforded by badJNIHandle.
>> (This is because, unlike local and global handles, a weak global
>> handle can refer to a NULL value, because it may have been cleared by
>> the GC.)  That protection was already fairly minimal for such handles,
>> even when using badJNIHandle.  That protection only applied for
>> entries in a newly allocated block that have never yet been allocated.
>> Blocks in the global lists are currently never released, so the
>> setting to a "bad" value for released handles never occurs for weak
>> global handles.  So the most interesting use-case for badJNIHandle
>> (identifying uses of stale handles) doesn't even apply.
>>
>> There is one other use of badJNIHandle: throw_unsatisfied_link_error.
>> This function isn't currently called, only used as a marker. The
>> discussion in bug 4524377, where that function was added, suggested
>> using badAddress, so we're replacing badJNIHandle with badAddress.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8191870
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/
>>
>> Testing:
>> Mach5 hs-tier1-3
>> tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

coleen.phillimore

My question was why there's a command line option, which seems like
nobody on earth should use it.  Yes, we don't need to zap for product,
only for debug.
Coleen

On 11/27/17 10:02 AM, Daniel D. Daugherty wrote:

> Because it is a 'trueInDebug' option, we only do the ZAP'ing in
> non-product
> builds... so we don't take the time to ZAP in product bits. As for the
> ZAP'ing,
> it helps us diagnose stale JNI handle usage...
>
> Dan
>
>
> On 11/27/17 9:53 AM, [hidden email] wrote:
>>
>> Hi, This looks good.
>>
>> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/src/hotspot/share/runtime/globals.hpp.udiff.html 
>>
>>
>> Is there a point to this command line option?  It's a develop option-
>> can we just remove it?
>>
>> Thanks,
>> Coleen
>>
>> On 11/25/17 12:35 AM, Kim Barrett wrote:
>>> Please review this change to eliminate badJNIHandle and the special
>>> debug-only handling of it when resolving JNI handles.
>>>
>>> For the intended purpose of catching bad JNI handle usage while
>>> sometimes normalizing some to NULL, we mostly don't need such a
>>> special value. We can instead use NULL directly, and get largely the
>>> same effect.
>>>
>>> By filling blocks with NULL, rather than badJNIHandle, we avoid the
>>> difficulties related to the Access protocol (see the CR).
>>>
>>> A NULL value is sufficient for local and non-weak global handles.  A
>>> JNI handle is never allocated to refer to a NULL value; instead, a
>>> NULL handle is used.  resolve and friends already treat the case of a
>>> NULL pointee specially (asserting, except in the external guard case,
>>> where it is allowed to pass through).
>>>
>>> Only weak global handles lose the protection afforded by badJNIHandle.
>>> (This is because, unlike local and global handles, a weak global
>>> handle can refer to a NULL value, because it may have been cleared by
>>> the GC.)  That protection was already fairly minimal for such handles,
>>> even when using badJNIHandle.  That protection only applied for
>>> entries in a newly allocated block that have never yet been allocated.
>>> Blocks in the global lists are currently never released, so the
>>> setting to a "bad" value for released handles never occurs for weak
>>> global handles.  So the most interesting use-case for badJNIHandle
>>> (identifying uses of stale handles) doesn't even apply.
>>>
>>> There is one other use of badJNIHandle: throw_unsatisfied_link_error.
>>> This function isn't currently called, only used as a marker. The
>>> discussion in bug 4524377, where that function was added, suggested
>>> using badAddress, so we're replacing badJNIHandle with badAddress.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8191870
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8191870/open.00/
>>>
>>> Testing:
>>> Mach5 hs-tier1-3
>>> tonga: nsk.jvmti, nsk.stress, vm.runtime (for various JNI tests)
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>
>
> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.

So remove the CLA and instead #ifdef ASSERT the zapping.

I’d be okay with that.  Another CLA bites the dust.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

coleen.phillimore


On 11/27/17 3:21 PM, Kim Barrett wrote:
>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>
>>
>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
> So remove the CLA and instead #ifdef ASSERT the zapping.

Yes.
>
> I’d be okay with that.  Another CLA bites the dust.

What looks like a pointless one at that.

Coleen
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>
>
>
> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>
>>>
>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>> So remove the CLA and instead #ifdef ASSERT the zapping.
>
> Yes.
>>
>> I’d be okay with that.  Another CLA bites the dust.
>
> What looks like a pointless one at that.
>
> Coleen

New webrev with ZapJNIHandleArea removed.

full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/
incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Daniel D. Daugherty
On 11/27/17 6:48 PM, Kim Barrett wrote:

>> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>>
>>
>>
>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>>
>>>>
>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>> Yes.
>>> I’d be okay with that.  Another CLA bites the dust.
>> What looks like a pointless one at that.
>>
>> Coleen
> New webrev with ZapJNIHandleArea removed.
>
> full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/

src/hotspot/share/runtime/globals.hpp
     No comments.

src/hotspot/share/runtime/jniHandles.cpp
     No comments.

src/hotspot/share/runtime/jniHandles.hpp
     No comments.

src/hotspot/share/runtime/sharedRuntime.cpp
     No comments.

src/hotspot/share/utilities/globalDefinitions.hpp
     No comments.

Thumbs up.

Dan

> incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Erik Österlund-2
In reply to this post by Kim Barrett
Hi Kim,

Still looks good.

Thanks,
/Erik

On 2017-11-28 00:48, Kim Barrett wrote:

>> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>>
>>
>>
>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>>
>>>>
>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>> Yes.
>>> I’d be okay with that.  Another CLA bites the dust.
>> What looks like a pointless one at that.
>>
>> Coleen
> New webrev with ZapJNIHandleArea removed.
>
> full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

coleen.phillimore
In reply to this post by Kim Barrett


On 11/27/17 6:48 PM, Kim Barrett wrote:

>> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>>
>>
>>
>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>>
>>>>
>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>> Yes.
>>> I’d be okay with that.  Another CLA bites the dust.
>> What looks like a pointless one at that.
>>
>> Coleen
> New webrev with ZapJNIHandleArea removed.
>
> full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/
>

http://cr.openjdk.java.net/~kbarrett/8191870/open.01/src/hotspot/share/runtime/jniHandles.hpp.udiff.html

I think you need PRODUCT_RETURN for zap() in case the optimized target
still builds.  Otherwise, looks good.

Coleen
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
In reply to this post by Daniel D. Daugherty
> On Nov 28, 2017, at 8:45 AM, Daniel D. Daugherty <[hidden email]> wrote:
>
> On 11/27/17 6:48 PM, Kim Barrett wrote:
>>> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>>>
>>>
>>>
>>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>>>
>>>>>
>>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>>> Yes.
>>>> I’d be okay with that.  Another CLA bites the dust.
>>> What looks like a pointless one at that.
>>>
>>> Coleen
>> New webrev with ZapJNIHandleArea removed.
>>
>> full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/
>
> src/hotspot/share/runtime/globals.hpp
>     No comments.
>
> src/hotspot/share/runtime/jniHandles.cpp
>     No comments.
>
> src/hotspot/share/runtime/jniHandles.hpp
>     No comments.
>
> src/hotspot/share/runtime/sharedRuntime.cpp
>     No comments.
>
> src/hotspot/share/utilities/globalDefinitions.hpp
>     No comments.
>
> Thumbs up.

Thanks, Dan.

>
> Dan
>
>> incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
In reply to this post by Erik Österlund-2
> On Nov 28, 2017, at 9:16 AM, Erik Österlund <[hidden email]> wrote:
>
> Hi Kim,
>
> Still looks good.

Thanks, Erik.

>
> Thanks,
> /Erik
>
> On 2017-11-28 00:48, Kim Barrett wrote:
>>> On Nov 27, 2017, at 3:43 PM, [hidden email] wrote:
>>>
>>>
>>>
>>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>>>
>>>>>
>>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>>> Yes.
>>>> I’d be okay with that.  Another CLA bites the dust.
>>> What looks like a pointless one at that.
>>>
>>> Coleen
>> New webrev with ZapJNIHandleArea removed.
>>
>> full: http://cr.openjdk.java.net/~kbarrett/8191870/open.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8191870/open.01.inc/


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
In reply to this post by coleen.phillimore
> On Nov 28, 2017, at 9:58 AM, [hidden email] wrote:
>
> http://cr.openjdk.java.net/~kbarrett/8191870/open.01/src/hotspot/share/runtime/jniHandles.hpp.udiff.html
>
> I think you need PRODUCT_RETURN for zap() in case the optimized target still builds.  Otherwise, looks good.
>
> Coleen

I think PRODUCT_RETURN would break optimize builds.

The definition of zap in the .cpp file is wrapped in #ifdef ASSERT.
NOT_DEBUG_RETURN's definition is conditional on ASSERT.  So those
match.

Using PRODUCT_RETURN would require the .cpp definition to be wrapped
in #ifndef PRODUCT.  But that combination would make zapping happen for
optimize builds, which I think is wrong.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

coleen.phillimore


On 11/28/17 2:02 PM, Kim Barrett wrote:

>> On Nov 28, 2017, at 9:58 AM, [hidden email] wrote:
>>
>> http://cr.openjdk.java.net/~kbarrett/8191870/open.01/src/hotspot/share/runtime/jniHandles.hpp.udiff.html
>>
>> I think you need PRODUCT_RETURN for zap() in case the optimized target still builds.  Otherwise, looks good.
>>
>> Coleen
> I think PRODUCT_RETURN would break optimize builds.
>
> The definition of zap in the .cpp file is wrapped in #ifdef ASSERT.
> NOT_DEBUG_RETURN's definition is conditional on ASSERT.  So those
> match.
>
> Using PRODUCT_RETURN would require the .cpp definition to be wrapped
> in #ifndef PRODUCT.  But that combination would make zapping happen for
> optimize builds, which I think is wrong.
>
>

Oh, yes, you're right.  I get that confused.
Thanks,
Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
> On Nov 28, 2017, at 2:37 PM, [hidden email] wrote:
>
>
>
> On 11/28/17 2:02 PM, Kim Barrett wrote:
>>> On Nov 28, 2017, at 9:58 AM, [hidden email] wrote:
>>>
>>> http://cr.openjdk.java.net/~kbarrett/8191870/open.01/src/hotspot/share/runtime/jniHandles.hpp.udiff.html
>>>
>>> I think you need PRODUCT_RETURN for zap() in case the optimized target still builds.  Otherwise, looks good.
>>>
>>> Coleen
>> I think PRODUCT_RETURN would break optimize builds.
>>
>> The definition of zap in the .cpp file is wrapped in #ifdef ASSERT.
>> NOT_DEBUG_RETURN's definition is conditional on ASSERT.  So those
>> match.
>>
>> Using PRODUCT_RETURN would require the .cpp definition to be wrapped
>> in #ifndef PRODUCT.  But that combination would make zapping happen for
>> optimize builds, which I think is wrong.
>>
>>
>
> Oh, yes, you're right.  I get that confused.
> Thanks,
> Coleen

Thanks for reviewing.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

David Holmes
In reply to this post by coleen.phillimore
On 28/11/2017 6:43 AM, [hidden email] wrote:
> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>> My question was why there's a command line option, which seems like
>>> nobody on earth should use it.  Yes, we don't need to zap for
>>> product, only for debug.
>> So remove the CLA and instead #ifdef ASSERT the zapping.
>
> Yes.

Okay we _really_ need to establish the ground rules for when you can
just delete a CLA without going through any deprecation process or
filing a CSR!

David
-----

>>
>> I’d be okay with that.  Another CLA bites the dust.
>
> What looks like a pointless one at that.
>
> Coleen
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191870: Remove badJNIHandle

Kim Barrett
> On Nov 29, 2017, at 5:25 AM, David Holmes <[hidden email]> wrote:
>
> On 28/11/2017 6:43 AM, [hidden email] wrote:
>> On 11/27/17 3:21 PM, Kim Barrett wrote:
>>>> On Nov 27, 2017, at 10:19 AM, [hidden email] wrote:
>>>> My question was why there's a command line option, which seems like nobody on earth should use it.  Yes, we don't need to zap for product, only for debug.
>>> So remove the CLA and instead #ifdef ASSERT the zapping.
>> Yes.
>
> Okay we _really_ need to establish the ground rules for when you can just delete a CLA without going through any deprecation process or filing a CSR!

Yes, that would be good to have.

> David
> -----
>
>>>
>>> I’d be okay with that.  Another CLA bites the dust.
>> What looks like a pointless one at that.
>> Coleen