[10] Review Request: 8187442 Xcheck:jni produces various "WARNING in native method" in launcher

[Kumar Srinivasan]

Hi,

Looks good to me.

Kumar

On 9/14/2017 12:01 PM, Sergey Bylokhov wrote:
> 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>
>