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

[Sergey Bylokhov]

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