[10] Review Request: 8187442 Xcheck:jni produces various "WARNING in native method" in launcher
Kumar Srinivasan
kumar.x.srinivasan at oracle.com
Wed Sep 13 17:16:15 UTC 2017
Hi Sergey,
If you so wish, you could make the test a little simpler, by using
the createJar method in TestHelper.java which compiles a file and
creates an executable jar out of it, refer to ArgsEnvVar.java,
lines 43 to 54 in the test can be replaced by:
StringBuilder tsrc = new StringBuilder();
tsrc.append("public static void main(String... args) {\n");
tsrc.append(" System.out.println(\"Hello world\");\n");
tsrc.append("}\n");
createJar(testJar, new File("Foo"), tsrc.toString());
but what you currently have is also fine, thanks for making these changes.
Kumar
> On 13/09/2017 9:42 AM, Sergey Bylokhov wrote:
>> On 9/12/17 16:03, David Holmes wrote:
>>> call. That is fine, but still leaves the problem that you are
>>> skipping the DeleteLocalRef call.
>>
>> The leak was there before: if we will get an exception at
>> "1512 SetByteArrayRegion()"
>> then we never call this DeleteLocalRef().
>>
>> I assumed it was done intentionally since this will be kind of
>> "fatal" error.
>
> On further checking DeleteLocalRef is not actually needed in
> NewPlatformString at all. The only time it is recommended practice to
> manually delete local refs is when looping, as in NewPlatformStringArray.
>
> So all good - Reviewed. And sorry for the noise.
>
> Thanks,
> David
>
>
>
>> The same pattern is used in the other places for example in
>> NewPlatformStringArray:
>>
>> jstring str = NewPlatformString(env, *strv++);
>> NULL_CHECK0(str);
>> (*env)->SetObjectArrayElement(env, ary, i, str);
>> (*env)->DeleteLocalRef(env, str);
>>
>>>
>>> Thanks - and sorry again for my confusion on this.
>>>
>>> David
>>> -----
>>>
>>>>> In addition this does nothing to clear the pending exception so I
>>>>> can not see how it would affect any warnings.
>>>>
>>>> It does not clear an exception but preserve it instead, and does
>>>> not use the result of the method which produced an exception.
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>> This value will be propagated to JavaMain() and I as far as
>>>>>> understand will stop the execution.
>>>>>>
>>>>>>
>>>>>> On 9/12/17 13:56, David Holmes wrote:
>>>>>>> Hi Sergey,
>>>>>>>
>>>>>>> On 13/09/2017 5:18 AM, Sergey Bylokhov wrote:
>>>>>>>> Hello,
>>>>>>>> Please review the fix for jdk10/jdk10.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8187442
>>>>>>>> Webrev can be found at:
>>>>>>>> http://cr.openjdk.java.net/~serb/8187442/webrev.00
>>>>>>>
>>>>>>> This doesn't look right to me:
>>>>>>>
>>>>>>> str = (*env)->CallStaticObjectMethod(env, cls,
>>>>>>> makePlatformStringMID, USE_STDERR, ary);
>>>>>>> + CHECK_EXCEPTION_RETURN_VALUE(0);
>>>>>>> (*env)->DeleteLocalRef(env, ary);
>>>>>>> return str;
>>>>>>>
>>>>>>> If there is an exception pending you fail to delete the local
>>>>>>> ref. And there's no need to clear the exception before calling
>>>>>>> DeleteLocalRef as that is okay to call with a pending exception.
>>>>>>> But there is no point returning zero if an exception occurred
>>>>>>> because in that case str will already be 0/NULL.
>>>>>>>
>>>>>>> The same here:
>>>>>>>
>>>>>>> 1596 appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
>>>>>>> 1597 CHECK_EXCEPTION_RETURN_VALUE(0);
>>>>>>> 1598 return appClass;
>>>>>>>
>>>>>>> If an exception is pending then appClass will be 0/NULL.
>>>>>>>
>>>>>>> In addition CHECK_EXCEPTION_RETURN_VALUE doesn't clear the
>>>>>>> pending exception so I can't see what warnings this would be
>>>>>>> clearing up ???
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> The simple application with empty main method produce some
>>>>>>>> "warnings" when Xcheck:jni is used. Because of that it is hard
>>>>>>>> to cleanup other parts of jdk from such warnings.
>>>>>>>>
>>>>>>>> Two additional checks were added, in both cases the new code
>>>>>>>> will return 0 in the same way as NULL_CHECK0 in the same methods.
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
More information about the core-libs-dev
mailing list