RFR: 8329581: Java launcher no longer prints a stack trace [v9]
Thomas Stuefe
stuefe at openjdk.org
Wed May 8 09:41:00 UTC 2024
On Mon, 6 May 2024 19:06:10 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 indentation
>
> Co-authored-by: ExE Boss <3889017+ExE-Boss at users.noreply.github.com>
Pre-existing: Man, I cannot grok the complex return code handling, tbh.
We have the local `ret` variable holding a return code. We also hand codes to CHECK_EXCEPTION_LEAVE as macro argument. But we don't hand codes to CHECK_EXCEPTION_NULL_LEAVE. LEAVE uses the locally defined `ret` instead of getting the return code as argument. CHECK_EXCEPTION_LEAVE modifies the local `ret`, but CHECK_EXCEPTION_NULL_LEAVE does not.
CHECK_EXCEPTION_NULL_LEAVE does not set `ret`. So, in case of an error, it would cause the launcher to return OK, but this does not happen because the local `ret` gets initialized to 1 before the first call to CHECK_EXCEPTION_NULL_LEAVE (line 566 resp. 560). Not sure if this was intentional, but it surely is very brittle. We rely on the content of `ret`, and that changes several times throughout JavaMain.
CHECK_EXCEPTION_NULL_LEAVE argument is named CENL_exception, which I don't understand.
To confuse matters more, the logic for internal error codes and the launcher return code is reversed: internally, 0 means error, and externally, 0 means success. And we only use numerical literals (`1`, `0`) instead of clearly named constants.
This may be food for another RFE, to keep this patch minimal. But a good solution, to me, would be like this:
- have the same logic for return codes (1 = error, 0 = success) to ease understanding
- have clearly named constants (e.g. "LAUNCHER_OK" 0, "LAUNCHER_ERR" = 1)
- have the LEAVE macro take the launcher return code as argument
- have all xxx_LEAVE macros pass in LAUNCHER_ERR to LEAVE
- call the final LEAVE with LAUNCHER_OK
- optionally, define something like "LEAVE_ERR" and "LEAVE_OK" that call LEAVE with either LAUNCHER_ERR or LAUNCHER_OK, for more concise coding.
For this patch, I think the return code logic is okay, but I would feel better if others double-checked.
src/java.base/share/native/libjli/java.c line 394:
> 392: if ((*env)->ExceptionOccurred(env)) { \
> 393: return 0; \
> 394: } else if (obj == NULL) { \
Side note, I first wondered if this comparison is strictly correct, since we now pass in `jmethodID` as well as `jobject`, which are opaque types and not necessarily of the same size.
But seems that jmethodID==NULL is defined to mean "invalid" [1] by the spec. Requiring NULL instead of providing an opaque invalid constant feels like an odd choice in the original JNI spec, since it requires implementors to use a pointer type to implement jmethodID? Which we do, in OpenJDK [2].
[1] https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#getstaticmethodid
[2] https://github.com/openjdk/jdk/blob/2baacfc16916220846743c6e49a99a6c41cac510/src/java.base/share/native/include/jni.h#L135-L136
src/java.base/share/native/libjli/java.c line 420:
> 418: jmethodID mainID =
> 419: (*env)->GetMethodID(env, mainClass, "main", "([Ljava/lang/String;)V");
> 420: CHECK_EXCEPTION_NULL_FAIL(mainID);
Is there a particular reason why you moved this section up here, from line 432 before? If not, I'd restore it to its original position to keep the diff small.
src/java.base/share/native/libjli/java.c line 452:
> 450: jobject mainObject = (*env)->NewObject(env, mainClass, constructor);
> 451: CHECK_EXCEPTION_NULL_FAIL(mainObject);
> 452: jmethodID mainID = (*env)->GetMethodID(env, mainClass, "main", "()V");
Unnecessary change. Please restore original linebreak to have a smaller diff.
src/java.base/share/native/libjli/java.c line 618:
> 616: */
> 617:
> 618:
nit, please remove one line
-------------
PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2045014321
PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1593646409
PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1593649988
PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1593653226
PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1593653669
More information about the core-libs-dev
mailing list