RFR: 8329581: Java launcher no longer prints a stack trace [v3]

[Mandy Chung]

On Fri, 19 Apr 2024 19:39:09 GMT, Sonia Zaldana Calles <szaldana at 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