-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8329581: Java launcher no longer prints a stack trace #18786
Conversation
👋 Welcome back szaldana! A progress list of the required criteria for merging this PR into |
@SoniaZaldana This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 22 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tstuefe, @mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@SoniaZaldana The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
@@ -926,6 +948,8 @@ private static void validateMainMethod(Class<?> mainClass) { | |||
} | |||
} | |||
|
|||
setMainType(mainMethod); | |||
|
|||
int mods = mainMethod.getModifiers(); | |||
boolean isStatic = Modifier.isStatic(mods); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can get the value for isStatic
and noArgs
from the main type.
switch (mainType) { | ||
case MAIN_WITH_ARGS: | ||
res = invokeStaticMainWithArgs(env, mainClass, mainArgs); | ||
break; | ||
case MAIN_WITHOUT_ARGS: | ||
res = invokeStaticMainWithoutArgs(env, mainClass); | ||
break; | ||
case MAIN_NONSTATIC: | ||
res = invokeInstanceMainWithArgs(env, mainClass, mainArgs); | ||
break; | ||
case MAIN_NONSTATIC | MAIN_WITHOUT_ARGS: | ||
res = invokeInstanceMainWithoutArgs(env, mainClass); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one of invokeXXXMainYYYArgs is called. Looks like CHECK_EXCEPTION_FAIL
and CHECK_EXCEPTION_NULL_FAIL
are not needed and can use CHECK_EXCEPTION_LEAVE
and CHECK_EXCEPTION_NULL_LEAVE
instead?
Isn't this already fixed by #18753? |
Yes, it would be good to try the changes in pull/18753 first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find.
*/ | ||
private static final int MAIN_WITHOUT_ARGS = 1; | ||
private static final int MAIN_NONSTATIC = 2; | ||
private static int mainType = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: transferring information from java to C in this way is usually done, AFAICS, by accessing static fields directly (GetStaticFieldID+GetStaticXXXField). There is also a helper function that wraps this, see JNU_GetStaticFieldByName. Not sure if you have that available in libjli though. There are many examples for both patterns.
I also would consider just declaring two booleans here, isStatic and hasArgs, which that would be a bit clearer to read instead of combining both into a single flag variable, and no need to keep flag values synced across java/C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this some more, would it not be possible to just use the mainMethod directly down in C?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes JEP 463 went through many iterations, it was a fine balance of avoiding too many transitions and upcalls, and at the same time, have something that can be maintained. There are several JBS issues on this issue now, probably because there just wasn't enough tests introduced with the JEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, just ran into this as well. I was trying to do System::loadLibrary
and had no clue why I could load non-existent libraries. Turns out I couldn't :-) but the error was never reported back to me. Confusing.
I still like @SoniaZaldana 's variant better. AFAIU, in the original variant, we choose the main method via MethodFinder.findMainMethod, but then down in JavaMain in C we just try to invoke all variants in succession (static,args->instance,args->static,noargs->instance,noargs). Does that no mean we ignore the decision of findMainMethod? Apart from that, it means fewer exceptions are thrown when starting the JVM if we avoid calling methods we know don't work. |
@AlanBateman @wolfseifert Thanks for the comments! Just wanted to note that I tried out #18753 and the issue is not resolved. |
Just tried to avoid unnecessary duplication of work. |
I tested now pull/18753 and pull/18786: both solve both issues JDK-8329420 and JDK-8329581 |
My personal comments here:
|
hi all, thanks for the comments! Happy to make the updates based on the feedback. However, could someone please advice if we are proceeding with this PR or #18753? As @lahodaj noted, theirs avoids changing the dynamics between the Java helper and native code, so I just want to make sure we are on the same page. |
Agreed. I’ll add the test case if this PR proceeds (see my comment above).
Just to clarify, this would still mean converting “isStatic” and “noArgs” from local variables to fields so I am able to read them on the C side of things. Did I understand this correctly?
Agreed, I’ve updated that on my side of things. |
I'm okay with adding static boolean fields and read by the native code and the name can be explicit like |
I made the updates based on feedback. Just a few things to note:
Thanks! |
private static boolean isStatic = false; | ||
private static boolean noArgs = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static boolean isStatic = false; | |
private static boolean noArgs = false; | |
private static boolean isStaticMain = false; | |
private static boolean noArgMain = false; |
jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main", | ||
"([Ljava/lang/String;)V"); | ||
CHECK_EXCEPTION_FAIL(); | ||
CHECK_EXCEPTION_LEAVE(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
*/ | ||
int | ||
invokeStaticMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray mainArgs) { | ||
invokeStaticMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray mainArgs, | ||
JavaVM *vm, int ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
LEAVE(); | ||
|
||
|
||
jclass helperClass = GetLauncherHelperClass(env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow this file convention, declare all local variables at the beginning of this function.
jfieldID isStaticField = | ||
(*env)->GetStaticFieldID(env, helperClass, "isStatic", "Z"); | ||
jboolean isStatic = | ||
(*env)->GetStaticBooleanField(env, helperClass, isStaticField); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check exception. Local variable declared at the beginning of the function.
jfieldID isStaticField = | |
(*env)->GetStaticFieldID(env, helperClass, "isStatic", "Z"); | |
jboolean isStatic = | |
(*env)->GetStaticBooleanField(env, helperClass, isStaticField); | |
isStaticMainField = (*env)->GetStaticFieldID(env, helperClass, "isStaticMain", "Z"); | |
CHECK_EXCEPTION_NULL_LEAVE(isStaticMain); | |
isStaticMain = (*env)->GetStaticBooleanField(env, helperClass, isStaticMainField); |
jfieldID noArgsField = | ||
(*env)->GetStaticFieldID(env, helperClass, "noArgs", "Z"); | ||
jboolean noArgs = | ||
(*env)->GetStaticBooleanField(env, helperClass, noArgsField); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jfieldID noArgsField = | |
(*env)->GetStaticFieldID(env, helperClass, "noArgs", "Z"); | |
jboolean noArgs = | |
(*env)->GetStaticBooleanField(env, helperClass, noArgsField); | |
noArgMainField = (*env)->GetStaticFieldID(env, helperClass, "noArgMain", "Z"); | |
CHECK_EXCEPTION_NULL_LEAVE(noArgMain); | |
noArgMain = (*env)->GetStaticBooleanField(env, helperClass, noArgMainField); |
ret = invokeInstanceMainWithArgs(env, mainClass, mainArgs, vm, ret); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add CHECK_EXCEPTION_LEAVE(1);
to check any exception thrown when the main method is invoked.
jmethodID mainID = (*env)->GetStaticMethodID(env, mainClass, "main", | ||
"()V"); | ||
CHECK_EXCEPTION_FAIL(); | ||
CHECK_EXCEPTION_LEAVE(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as invokeStaticMainWithArgs
.
CHECK_EXCEPTION_LEAVE(1); | |
NULL_CHECK0(mainID); |
*/ | ||
int | ||
invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray mainArgs) { | ||
invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray mainArgs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
*/ | ||
int | ||
invokeInstanceMainWithoutArgs(JNIEnv *env, jclass mainClass) { | ||
invokeInstanceMainWithoutArgs(JNIEnv *env, jclass mainClass, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the suggestion for invokeInstanceMainWithArgs
Hi @mlchung, thanks for the feedback! I’ve pushed the updates. Just a question about
The JNI error message didn’t previously get reported before the regression was introduced, so I just wanted to make sure we were okay with this. Looking forward to your comments! |
I think such errors have a very high potential to confuse the hell out of the poor ops people looking for binary corruptions and stuff when it’s “only” a regular Java level error condition. So I think it should not talk about JNI in this case (even when the rest of the exception makes it obvious) |
I missed that
|
This same issue comes up periodically and maybe the JNI spec needs improvement to make it clearer that returning NULL means there is a pending exception/error. JNI would be very broken if it were to return a jclass or ID and also raise an exception, or return NULL rather than a jclass or ID when it succeeds. As you say, it's not wrong to check both, just a bit strange as they can be just checks for NULL. At some point I think we need to do a round of cleanup here as these macros can mask other issues. |
Hello Sonia, I happened to be working on something else and ended up hitting an issue where the java launch was failing with exit code 1 but was no longer printing the exception from the main application and realized that this PR hasn't been integrated. I see Alan has suggested a change but I'm unsure if you plan to implement that in this PR or if that's something that's suggested for a subsequent cleanup. I think it would be good to have the review suggestions resolved soon and have this integrated. |
/issue add JDK-8329420 |
@jaikiran Only the author (@SoniaZaldana) is allowed to issue the |
/issue add JDK-8329420 |
@SoniaZaldana |
/issue add JDK-8330864 |
@SoniaZaldana |
Hi all, I think there's some consensus that we need some follow up cleanup issues for the JNI spec, renaming constants, fixing return codes, etc. Seeing how that grows the scope of the issue quite a bit, I'd like to push this patch and track the other issues brought up separately. If there are no objections about the current state, I'd like to integrate some time next week. Let me know your thoughts. cc: @jaikiran, @AlanBateman |
Okay. The follow-on issue can remove CHECK_EXCEPTION_NULL_FAIL introduced by this PR or replace it with something simple that just checks if the ID is null. |
Hello Sonia @SoniaZaldana, so based on Alan's latest inputs, the next thing to do on this PR is to refresh the PR to merge it with the latest master branch changes and then run tier1, tier2 and tier3 tests to make sure they continue to pass and have this integrated soon. I'll file follow up issue(s) and also trigger CI testing of this PR. |
Thanks, the regressions fixed here are important to fix. It's unfortunate there the original changes weren't changes weren't caught by tests. There is a good test coverage added here but I think we'll need to see if there are other testing gaps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest version is still good.
Curious, why tier 1 to 3 specifically? Is there anything specific in tier 3 you want to have tested? |
I think just prudent to run more tests when touching the launcher as it has options that aren't tested much in tier1. Shouldn't be an issue here but thinking of the splash screen tests (jdk_desktop test group), and tests for Java agents. Also jlink and jpackage can be useful when testing changes to the launcher. |
@SoniaZaldana CI testing of this PR along with latest master commits passed without any failures. So you can now integrate this. |
Thanks @jaikiran! |
/integrate |
@SoniaZaldana |
/sponsor |
Going to push as commit cbb6747.
Your commit was automatically rebased without conflicts. |
@jaikiran @SoniaZaldana Pushed as commit cbb6747. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi folks,
This PR aims to fix JDK-8329581.
I think the regression got introduced in JDK-8315458.
In the issue linked above, LauncherHelper#getMainType got removed to simplify launcher code.
Previously, we used
getMainType
to do the appropriate main method invocation inJavaMain
. However, we currently attempt to do all types of main method invocations at the same time here.Note how all of these invocations clear the exception reported with CHECK_EXCEPTION_FAIL.
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
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786
$ git checkout pull/18786
Update a local copy of the PR:
$ git checkout pull/18786
$ git pull https://git.openjdk.org/jdk.git pull/18786/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18786
View PR using the GUI difftool:
$ git pr show -t 18786
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18786.diff
Webrev
Link to Webrev Comment