[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): JDK-8030708: warnings from b119 for jdk/src/share/back: JNI exception pending
From:       David Holmes <david.holmes () oracle ! com>
Date:       2015-01-29 11:31:45
Message-ID: 54CA1A21.4070600 () oracle ! com
[Download RAW message or body]

Looks good.

Thanks,
David

On 29/01/2015 8:29 PM, Dmitry Samersoff wrote:
> David,
>
> Fixed in place (press shift-reload)
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.04/
>
> -Dmitry
>
>
> On 2015-01-29 10:26, David Holmes wrote:
>> On 29/01/2015 12:00 AM, Dmitry Samersoff wrote:
>>> David,
>>>
>>>> Don't you need to execute the END_WITH_LOCAL_REFS before you can return?
>>>
>>> done.
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.04/
>>
>> Not quite how I envisaged the fix:
>>
>> JNI_FUNC_PTR(env,PopLocalFrame)(env, NULL); // END_WITH_LOCAL_REFS
>>
>> inlining the macro defeats the purpose the macro to some extent - if it
>> were changed then this code would not get the change. I was thinking
>> more along the lines of:
>>
>>   WITH_LOCAL_REFS(env, 1) {
>>
>>       char *utf;
>>
>>       utf = (char *)JNI_FUNC_PTR(env,GetStringUTFChars)(env, string, NULL);
>>       if (!(*env)->ExceptionCheck(env)) {
>>          (void)outStream_writeString(out, utf);
>>          JNI_FUNC_PTR(env,ReleaseStringUTFChars)(env, string, utf);
>>       }
>>
>> } END_WITH_LOCAL_REFS(env);
>>
>> David
>>
>>> -Dmitry
>>>
>>> On 2015-01-28 10:24, David Holmes wrote:
>>>> Hi Dmitry,
>>>>
>>>> Sorry this slipped through the cracks.
>>>>
>>>> On 21/01/2015 11:05 PM, Dmitry Samersoff wrote:
>>>>> David,
>>>>>
>>>>> Please, take a look at updated webrev
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.03/
>>>>
>>>> src/jdk.jdwp.agent/share/native/libjdwp/StringReferenceImpl.c.
>>>>
>>>> Don't you need to execute the END_WITH_LOCAL_REFS before you can return?
>>>>
>>>> Otherwise seems okay.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2014-10-16 16:07, David Holmes wrote:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 16/10/2014 8:08 PM, Dmitry Samersoff wrote:
>>>>>>> David,
>>>>>>>
>>>>>>> Changed. Thank you for review!
>>>>>>>
>>>>>>> please, see:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.02/
>>>>>>
>>>>>>     102     if (weakRef == NULL || (*env)->ExceptionCheck(env)) {
>>>>>>
>>>>>> Isn't the only time it will return NULL when an exception occurs?
>>>>>> Conversely if an exception occurs then it must return NULL - so the
>>>>>> exception check seems redundant.
>>>>>>
>>>>>> But this also suggests you need similar logic at:
>>>>>>
>>>>>>     182         weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env,
>>>>>> node->ref);
>>>>>>
>>>>>>     456                 lref = JNI_FUNC_PTR(env,NewLocalRef)(env,
>>>>>> node->ref);
>>>>>>
>>>>>> Or more generally any JNI call from JVMTI should be wrapped in a way
>>>>>> that checks for exceptions and clears them.
>>>>>>
>>>>>> David
>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>> On 2014-10-16 04:24, David Holmes wrote:
>>>>>>>> On 16/10/2014 12:33 AM, Dmitry Samersoff wrote:
>>>>>>>>> David,
>>>>>>>>>
>>>>>>>>> Sorry, copied wrong function!
>>>>>>>>>
>>>>>>>>> I mean this call
>>>>>>>>>
>>>>>>>>> weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref);
>>>>>>>>>
>>>>>>>>> that can post  OutOfMemoryError
>>>>>>>>
>>>>>>>> Okay, so shouldn't that be where the exception is cleared:
>>>>>>>>
>>>>>>>>        /* Create weak reference to make sure we have a reference */
>>>>>>>>         weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref);
>>>>>>>>         if (weakRef == NULL) {
>>>>>>>> +       // < clear exception here >
>>>>>>>>             jvmtiDeallocate(node);
>>>>>>>>             return NULL;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> commonRef_refToID() ->
>>>>>>>>>
>>>>>>>>> createNode(JNIEnv *env, jobject ref) ->
>>>>>>>>>
>>>>>>>>> weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref);
>>>>>>>>>
>>>>>>>>> -Dmitry
>>>>>>>>>
>>>>>>>>> On 2014-10-15 16:21, David Holmes wrote:
>>>>>>>>>> On 15/10/2014 8:39 PM, Dmitry Samersoff wrote:
>>>>>>>>>>> On 2014-10-15 14:27, David Holmes wrote:
>>>>>>>>>>>> On 15/10/2014 8:08 PM, Dmitry Samersoff wrote:
>>>>>>>>>>>>> Please review the fix:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.01/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Added missed exception checks.
>>>>>>>>>>>>
>>>>>>>>>>>> src/jdk.jdwp.agent/share/native/libjdwp/outStream.c
>>>>>>>>>>>>
>>>>>>>>>>>> What is potentially posting the exception?
>>>>>>>>>>>
>>>>>>>>>>> JvmtiEnv::GetTag(jobject object, jlong* tag_ptr) called from
>>>>>>>>>>> commonRef_refToID()
>>>>>>>>>>
>>>>>>>>>> You mean this call:
>>>>>>>>>>
>>>>>>>>>>         error =
>>>>>>>>>> JVMTI_FUNC_PTR(gdata->jvmti,GetTag)(gdata->jvmti, ref,
>>>>>>>>>> &tag);
>>>>>>>>>
>>>>>>>>> x
>>>>>>>>>>
>>>>>>>>>> in findNodeByRef which is called by commonRef_refToID?  JVM TI
>>>>>>>>>> doesn't
>>>>>>>>>> post exceptions.
>>>>>>>>>>
>>>>>>>>>> "JVM TI functions never throw exceptions; error conditions are
>>>>>>>>>> communicated via the function return value. Any existing exception
>>>>>>>>>> state
>>>>>>>>>> is preserved across a call to a JVM TI function."
>>>>>>>>>>
>>>>>>>>>> http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]