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.
