[10] Review Request: 8187442 Xcheck:jni produces various "WARNING in native method" in launcher
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Thu Sep 14 19:01:01 UTC 2017
Hi, Kumar.
The test is updated as suggested:
http://cr.openjdk.java.net/~serb/8187442/webrev.01
On 9/13/17 10:16, Kumar Srinivasan wrote:
> 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
--
Best regards, Sergey.
More information about the core-libs-dev
mailing list