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 >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
