On Fri, 19 Apr 2024 19:39:09 GMT, Sonia Zaldana Calles <szald...@openjdk.org> wrote:
>> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Fixing space formatting src/java.base/share/classes/sun/launcher/LauncherHelper.java line 908: > 906: > 907: private static boolean isStatic = false; > 908: private static boolean noArgs = false; Suggestion: private static boolean isStaticMain = false; private static boolean noArgMain = false; src/java.base/share/native/libjli/java.c line 396: > 394: int > 395: invokeStaticMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray > mainArgs, > 396: JavaVM *vm, int ret) { As `CHECK_EXCEPTION_xxx_LEAVE` assumes to be used within JavaMain and it changes the value of the local `ret` variable, it should call `CHECK_EXCEPTION_RETURN_VALUE` and `NULL_CHECK_RETURN_VALUE` instead. JavaMain should call `CHECK_EXCEPTION_LEAVE(1);` after this method returns to print any exception and exit. src/java.base/share/native/libjli/java.c line 399: > 397: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main", > 398: "([Ljava/lang/String;)V"); > 399: CHECK_EXCEPTION_LEAVE(1); Is this needed? `invokeStaticMainWithArgs` should only be called if the main method is validated as a static main with args. A sanity check `NULL_CHECK0(mainID)` may be adequate (to use existing macro and so keeping the return value 0 to indicate error). src/java.base/share/native/libjli/java.c line 409: > 407: */ > 408: int > 409: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray > mainArgs, int invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray mainArgs) { jmethodID mainID = (*env)->GetMethodID(env, mainClass, "main", "([Ljava/lang/String;)V"); NULL_CHECK0(mainID); jmethodID constructor = (*env)->GetMethodID(env, mainClass, "<init>", "()V"); NULL_CHECK0(constructor); jobject mainObject = (*env)->NewObject(env, mainClass, constructor); CHECK_EXCEPTION_RETURN_VALUE(0); NULL_CHECK0(mainObject); (*env)->CallVoidMethod(env, mainObject, mainID, mainArgs); return 1; } src/java.base/share/native/libjli/java.c line 431: > 429: jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main", > 430: "()V"); > 431: CHECK_EXCEPTION_LEAVE(1); Same comment as `invokeStaticMainWithArgs`. Suggestion: NULL_CHECK0(mainID); src/java.base/share/native/libjli/java.c line 441: > 439: */ > 440: int > 441: invokeInstanceMainWithoutArgs(JNIEnv *env, jclass mainClass, See the suggestion for `invokeInstanceMainWithArgs` src/java.base/share/native/libjli/java.c line 610: > 608: > 609: > 610: jclass helperClass = GetLauncherHelperClass(env); Follow this file convention, declare all local variables at the beginning of this function. src/java.base/share/native/libjli/java.c line 614: > 612: (*env)->GetStaticFieldID(env, helperClass, "isStatic", "Z"); > 613: jboolean isStatic = > 614: (*env)->GetStaticBooleanField(env, helperClass, isStaticField); Check exception. Local variable declared at the beginning of the function. Suggestion: isStaticMainField = (*env)->GetStaticFieldID(env, helperClass, "isStaticMain", "Z"); CHECK_EXCEPTION_NULL_LEAVE(isStaticMain); isStaticMain = (*env)->GetStaticBooleanField(env, helperClass, isStaticMainField); src/java.base/share/native/libjli/java.c line 619: > 617: (*env)->GetStaticFieldID(env, helperClass, "noArgs", "Z"); > 618: jboolean noArgs = > 619: (*env)->GetStaticBooleanField(env, helperClass, noArgsField); Suggestion: noArgMainField = (*env)->GetStaticFieldID(env, helperClass, "noArgMain", "Z"); CHECK_EXCEPTION_NULL_LEAVE(noArgMain); noArgMain = (*env)->GetStaticBooleanField(env, helperClass, noArgMainField); src/java.base/share/native/libjli/java.c line 634: > 632: } > 633: } > 634: Add `CHECK_EXCEPTION_LEAVE(1);` to check any exception thrown when the main method is invoked. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575165463 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575203911 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575201446 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575237945 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575234012 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575238650 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575222579 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575225615 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575226535 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1575227620
