On 9/12/17 14:28, David Holmes wrote:
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.

According to the spec of jni there is no information that CallStaticObjectMethod() return null/0 when exception is occurred.
So checkjni is not smart enough or takes this into account.

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.

Reply via email to