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

[David Holmes]

Hi Sergey,

On 13/09/2017 7:22 AM, Sergey Bylokhov wrote:
> Hi, David.
> Thank you for review, here are my thoughts.
> 
> The patch uses the same pattern as NULL_CHECK0.
> 
> If an exception is occurred then NULL_CHECK0 in one situation and 
> CHECK_EXCEPTION_RETURN_VALUE in another situation will return 0 to the 
> upper code which will process this value(there is the same 0/null checks).
> 
> The difference between NULL_CHECK0 and CHECK_EXCEPTION_RETURN_VALUE, is 
> that the first is used when the "NULL" result also means some error, for 
> example:
>      NULL_CHECK0(mid = (*env)->GetStaticMethodID(env, cls,
>                  "getApplicationClass",
>                  "()Ljava/lang/Class;"));
> 
>      appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
>      CHECK_EXCEPTION_RETURN_VALUE(0);
> 
> In the code above the GetStaticMethodID() returns NULL if the operation 
> fails, but it also throw an exceptions like NoSuchMethodError/etc.
> 
> But in case of CallStaticObjectMethod() we cannot check for NULL which 
> can be a valid result, so I added a check for exception and return 0.

My point is that the check is completely redundant. If an exception 
occurred then the return value is already 0/NULL. In addition this does 
nothing to clear the pending exception so I can not see how it would 
affect any warnings.

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.
>>>
>>>
> 
>