Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > 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 > @SoniaZaldana CI testing of this PR along with latest master commits passed > without any failures. So you can now integrate this. Thanks @jaikiran! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2149833060
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > 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 @SoniaZaldana CI testing of this PR along with latest master commits passed without any failures. So you can now integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2148719775
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > 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 > 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. Curious, why tier 1 to 3 specifically? Is there anything specific in tier 3 you want to have tested? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147514185
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v11]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8329581 - Decreasing diff size addressing unnecessary changes - Fixing indentation Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Fixing broken uncaught exception mechanism - Make deleting file OS agnostic - Adding test case - Removing long lines - Using new macro to avoid reporting JNI error - Changes based on feedback - Fixing space formatting - ... and 4 more: https://git.openjdk.org/jdk/compare/d654de0c...12fe416c - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/1a0c76f2..12fe416c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=09-10 Stats: 582850 lines in 7246 files changed: 146903 ins; 86361 del; 349586 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 4 Jun 2024 13:17:11 GMT, Thomas Stuefe wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147552797
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 4 Jun 2024 12:59:54 GMT, Jaikiran Pai wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147492466
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v11]
On Tue, 4 Jun 2024 13:34:30 GMT, Sonia Zaldana Calles 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 with a new target base due > to a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into JDK-8329581 > - Decreasing diff size addressing unnecessary changes > - Fixing indentation > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > - Fixing broken uncaught exception mechanism > - Make deleting file OS agnostic > - Adding test case > - Removing long lines > - Using new macro to avoid reporting JNI error > - Changes based on feedback > - Fixing space formatting > - ... and 4 more: https://git.openjdk.org/jdk/compare/d654de0c...12fe416c Latest version is still good. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2096358502
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > 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 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147476871
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147452633
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles 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: > > Decreasing diff size addressing unnecessary changes 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 - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2142383991
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles 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: > > Decreasing diff size addressing unnecessary changes Hello Sonia, I happened to be working on something else and ended up hitting an issue where the java launch was no longer throwing 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2141428133
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 14 May 2024 07:14:09 GMT, Thomas Stuefe wrote: > but it does not state explicitly that an exception is thrown on every error, > or whether there are cases where the API can return NULL but not throw an > exception, or vice versa. > > So, I'd check for both. Or, if we think that both should not happen or happen > together, assert that. (we can use the standard C assert in the JDK > libraries, no?) 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. It's not wrong to first check for a first exception, just a bit strange and not immediately clear what issues they may be masking. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2109495552
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Mon, 13 May 2024 18:01:25 GMT, Sonia Zaldana Calles wrote: > > This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI > > functions GetStaticMethodID, GetMethodID and NewObject return NULL with a > > pending exception when they fail. So I would expect > > CHECK_EXCEPTION_NULL_FAIL to just check if obj is NULL rather check for an > > exception first. It's not wrong to check for an exception, just curious > > when looking at this macro. > > Hi @AlanBateman, thanks for taking a look. That's a good point - would it be > worthwhile to delete the exception check in this case? Well, its not wrong, and arguably more defensive. Doc for GetStaticMethodID states: RETURNS: Returns a method ID, or NULL if the operation fails. THROWS: NoSuchMethodError: if the specified static method cannot be found. ExceptionInInitializerError: if the class initializer fails due to an exception. OutOfMemoryError: if the system runs out of memory. but it does not state explicitly that an exception is thrown on every error, or whether there are cases where the API can return NULL but not throw an exception, or vice versa. So, I'd check for both. Or, if we think that both should not happen or happen together, assert that. (we can use the standard C assert in the JDK libraries, no?) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2109450146
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Sun, 12 May 2024 18:52:30 GMT, Alan Bateman wrote: > This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI > functions GetStaticMethodID, GetMethodID and NewObject return NULL with a > pending exception when they fail. So I would expect CHECK_EXCEPTION_NULL_FAIL > to just check if obj is NULL rather check for an exception first. It's not > wrong to check for an exception, just curious when looking at this macro. Hi @AlanBateman, thanks for taking a look. That's a good point - would it be worthwhile to delete the exception check in this case? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2108464274
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles 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: > > Decreasing diff size addressing unnecessary changes This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI functions GetStaticMethodID, GetMethodID and NewObject return NULL with a pending exception when they fail. So I would expect CHECK_EXCEPTION_NULL_FAIL to just check if obj is NULL rather check for an exception first. It's not wrong to check for an exception, just curious when looking at this macro. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2106343194
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Thu, 9 May 2024 19:48:53 GMT, Sonia Zaldana Calles wrote: >> 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. > >> 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. > > @tstuefe Agreed, I can look into opening another issue to track this after we > fix the regression. @SoniaZaldana I plan to review this shortly. It's an important issue that slipped through due to insufficient tests. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2104216259
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles 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: > > Decreasing diff size addressing unnecessary changes My remarks have all been addressed. Thank you, @SoniaZaldana. From my side this is good to go. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2049599835
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Thu, 9 May 2024 19:48:53 GMT, Sonia Zaldana Calles wrote: > > 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. > > @tstuefe Agreed, I can look into opening another issue to track this after we > fix the regression. Thank you. Please do. You can just copy-paste my rant into the bug description. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2104186742
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Wed, 8 May 2024 09:37:58 GMT, Thomas Stuefe wrote: > 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. @tstuefe Agreed, I can look into opening another issue to track this after we fix the regression. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2103312450
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
> 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: Decreasing diff size addressing unnecessary changes - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/9b67fd86..1a0c76f2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=08-09 Stats: 9 lines in 1 file changed: 4 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Mon, 6 May 2024 19:06:10 GMT, Sonia Zaldana Calles
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-b...@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
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
On Mon, 6 May 2024 19:06:10 GMT, Sonia Zaldana Calles 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-b...@users.noreply.github.com> Just noting, I don't think the GHA failures are related to my patch. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2099126506
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v9]
> 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-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/d918d995..9b67fd86 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=07-08 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v8]
On Mon, 6 May 2024 16:30:11 GMT, Sonia Zaldana Calles
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 broken uncaught exception mechanism
src/java.base/share/native/libjli/java.c line 639:
> 637:} else {
> 638: ret = invokeInstanceMainWithArgs(env, mainClass, mainArgs);
> 639:}
**Nit:** Wrong indentation[^1]:
Suggestion:
if (noArgMain) {
ret = invokeInstanceMainWithoutArgs(env, mainClass);
} else {
ret = invokeInstanceMainWithArgs(env, mainClass, mainArgs);
}
[^1]: (this is one of the reasons I hate when spaces are used to indent things)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1591407476
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v8]
On Mon, 6 May 2024 16:30:11 GMT, Sonia Zaldana Calles 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 broken uncaught exception mechanism FYI, I've also re-run all added test cases since making the latest changes and they pass. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2096458546
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v8]
> 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 broken uncaught exception mechanism - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/ca03ead2..d918d995 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=06-07 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 6 May 2024 10:21:12 GMT, Jaikiran Pai wrote: >> Hi @mlchung, thanks for the feedback! I’ve pushed the updates. >> >> Just a question about ```NULL_CHECK0```. >> >> ```NULL_CHECK0``` reports the error message and then the exception is >> described in ```CHECK_EXCEPTION_LEAVE```. This results in a stack trace that >> looks like this: >> >> >> Error: A JNI error has occurred, please check your installation and try again >> Exception in thread "main" java.lang.NoClassDefFoundError: >> DemoSonia$SomeDependency >> at DemoSonia.(DemoSonia.java:3) >> Caused by: java.lang.ClassNotFoundException: DemoSonia$SomeDependency >> at >> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) >> at >> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) >> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:528) >> ... 1 more >> >> >> >> 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! > > Hello @SoniaZaldana, I don't remember seeing the > `java/lang/Thread/UncaughtExceptionsTest` fail before. It's failing in the > current PR's GitHub actions job on several platforms. Could you take a look > if it's related to the current changes? Hi @jaikiran, the failures were related to my change. I forgot to check the invocation failed and only trigger ```CHECK_EXCEPTION_LEAVE``` then. I have pushed an update that should fix the issue. Let's see how GHA turns out this time. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2096439688
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 23 Apr 2024 18:21:30 GMT, Sonia Zaldana Calles wrote: >>> 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? >> >> I'm okay with adding static boolean fields and read by the native code and >> the name can be explicit like `isStaticMain` and `mainWithNoArg`. > > Hi @mlchung, thanks for the feedback! I’ve pushed the updates. > > Just a question about ```NULL_CHECK0```. > > ```NULL_CHECK0``` reports the error message and then the exception is > described in ```CHECK_EXCEPTION_LEAVE```. This results in a stack trace that > looks like this: > > > Error: A JNI error has occurred, please check your installation and try again > Exception in thread "main" java.lang.NoClassDefFoundError: > DemoSonia$SomeDependency > at DemoSonia.(DemoSonia.java:3) > Caused by: java.lang.ClassNotFoundException: DemoSonia$SomeDependency > at > java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) > at > java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) > at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:528) > ... 1 more > > > > 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! Hello @SoniaZaldana, I don't remember seeing the `java/lang/Thread/UncaughtExceptionsTest` fail before. It's failing in the current PR's GitHub actions job on several platforms. Could you take a look if it's related to the current changes? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2095648483
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v7]
> 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: Make deleting file OS agnostic - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/e2ef2a51..ca03ead2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=05-06 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v6]
> 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 two additional commits since the last revision: - Adding test case - Removing long lines - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/3ea56c5c..e2ef2a51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=04-05 Stats: 112 lines in 2 files changed: 106 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v5]
On Wed, 24 Apr 2024 14:49:55 GMT, Sonia Zaldana Calles 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: > > Using new macro to avoid reporting JNI error Looks good. Can you add a new test for this? You can reference MainClassCantBeLoadedTest.java which does something similar to what you need. src/java.base/share/native/libjli/java.c line 621: > 619: helperClass = GetLauncherHelperClass(env); > 620: isStaticMainField = > 621: (*env)->GetStaticFieldID(env, helperClass, "isStaticMain", "Z"); Nit: this line isn't long. Can do in 1 line. Same for line 623-624, 626-627. - PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2020321074 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1578158252
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v5]
> 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: Using new macro to avoid reporting JNI error - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/66942238..3ea56c5c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=03-04 Stats: 19 lines in 1 file changed: 9 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v4]
On Tue, 23 Apr 2024 18:21:42 GMT, Sonia Zaldana Calles
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:
>
> Changes based on feedback
I missed that `NULL_CHECK0` prints that JNI error, sorry. We should avoid
using it then. What do you think having a check macro to return 0 if
exception occurred or the given object is null? No exception cleared as it
will be described in JavaMain.
#define CHECK_EXCEPTION_NULL_FAIL(obj) \
do { \
if ((*env)->ExceptionOccurred(env)) { \
return 0; \
} else if (obj == NULL) { \
return 0; \
} \
} while (JNI_FALSE)
-
PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2073184282
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 23 Apr 2024 18:21:30 GMT, Sonia Zaldana Calles wrote: > 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. 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) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2073171918
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Fri, 19 Apr 2024 17:04:30 GMT, Mandy Chung wrote: >> @lahodaj >> >>> I would suggest to take the test from 18753 though - doing a change like >>> this without a test may lead to hard-to-find regressions in the future. >>> (Note the current test should guard against both JDK-8329420 and >>> JDK-8329581.) >> >> Agreed. I’ll add the test case if this PR proceeds (see my comment above). >> >>> as Mandy points out, `LaucherHelper` already reads/has variables for >>> "is-static" and "no-arguments" in `validateMainMethod`, so it should be >>> possible to just use that values; >> >> 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? >> >>> also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` >>> and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The >>> `..._FAIL` variants where needed so that the launcher could continue with >>> the next variant, but since we now only call the correct variant, we can >>> just stop if something goes wrong?) >> >> Agreed, I’ve updated that on my side of things. > >> 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? > > I'm okay with adding static boolean fields and read by the native code and > the name can be explicit like `isStaticMain` and `mainWithNoArg`. Hi @mlchung, thanks for the feedback! I’ve pushed the updates. Just a question about ```NULL_CHECK0```. ```NULL_CHECK0``` reports the error message and then the exception is described in ```CHECK_EXCEPTION_LEAVE```. This results in a stack trace that looks like this: Error: A JNI error has occurred, please check your installation and try again Exception in thread "main" java.lang.NoClassDefFoundError: DemoSonia$SomeDependency at DemoSonia.(DemoSonia.java:3) Caused by: java.lang.ClassNotFoundException: DemoSonia$SomeDependency at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:528) ... 1 more 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! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2073092870
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v4]
> 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: Changes based on feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/79c70343..66942238 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=02-03 Stats: 65 lines in 2 files changed: 8 ins; 7 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v3]
On Fri, 19 Apr 2024 19:39:09 GMT, Sonia Zaldana Calles
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, "",
"()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)->GetSt
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/97a5f53f..79c70343 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 I made the updates based on feedback. Just a few things to note: * I included the test case from https://github.com/openjdk/jdk/pull/18753 and verified it passes with this PR. * Replacing ```CHECK_EXCEPTION_FAIL``` with ```CHECK_EXCEPTION_LEAVE``` looks a bit similar to my original proposed change. Calling ```CHECK_EXCEPTION_LEAVE``` directly in the JavaMain function causes the function to return early, whereas calling it inside the helpers (invokeStatic…) doesn’t quite have the same effect so I need to catch the result and then return. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2067171396
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v2]
> 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 three additional commits since the last revision: - Adding test case - Removing the need to call mainType method and adding static fields to java launcher - Removing CHECK_EXCEPTION_FAIL in favour of CHECK_EXCEPTION_LEAVE - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/2ed5a93b..97a5f53f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=00-01 Stats: 289 lines in 3 files changed: 188 ins; 41 del; 60 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Thu, 18 Apr 2024 20:41:05 GMT, Sonia Zaldana Calles wrote: > 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? I'm okay with adding static boolean fields and read by the native code and the name can be explicit like `isStaticMain` and `mainWithNoArg`. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2066958511
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Thu, 18 Apr 2024 07:34:24 GMT, Jan Lahoda 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 > > My personal comments here: > - I am fine with a solution like this. In 18753, I wanted to avoid a change > of dynamics between the Java helper and the native part. But if we can change > that, it looks better. I would suggest to take the test from 18753 though - > doing a change like this without a test may lead to hard-to-find regressions > in the future. (Note the current test should guard against both JDK-8329420 > and JDK-8329581.) Or write a different test. > - as Mandy points out, `LaucherHelper` already reads/has variables for > "is-static" and "no-arguments" in `validateMainMethod`, so it should be > possible to just use that values; also as Mandy points out, we can probably > get rid of `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL`, and use > the `..._LEAVE` variants, no? (The `..._FAIL` variants where needed so that > the launcher could continue with the next variant, but since we now only call > the correct variant, we can just stop if something goes wrong?) @lahodaj > I would suggest to take the test from 18753 though - doing a change like this > without a test may lead to hard-to-find regressions in the future. (Note the > current test should guard against both JDK-8329420 and JDK-8329581.) Agreed. I’ll add the test case if this PR proceeds (see my comment above). > as Mandy points out, `LaucherHelper` already reads/has variables for > "is-static" and "no-arguments" in `validateMainMethod`, so it should be > possible to just use that values; 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? > also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` > and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The > `..._FAIL` variants where needed so that the launcher could continue with the > next variant, but since we now only call the correct variant, we can just > stop if something goes wrong?) Agreed, I’ve updated that on my side of things. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2065283183
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 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 https://github.com/openjdk/jdk/pull/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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2065279462
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 16 Apr 2024 10:30:14 GMT, Alan Bateman wrote: >> Thinking about this some more, would it not be possible to just use the >> mainMethod directly down in C? > > 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. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1570997544
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 My personal comments here: - I am fine with a solution like this. In 18753, I wanted to avoid a change of dynamics between the Java helper and the native part. But if we can change that, it looks better. I would suggest to take the test from 18753 though - doing a change like this without a test may lead to hard-to-find regressions in the future. (Note the current test should guard against both JDK-8329420 and JDK-8329581.) Or write a different test. - as Mandy points out, `LaucherHelper` already reads/has variables for "is-static" and "no-arguments" in `validateMainMethod`, so it should be possible to just use that values; also as Mandy points out, we can probably get rid of `CHECK_EXCEPTION_FAIL` and `CHECK_EXCEPTION_NULL_FAIL`, and use the `..._LEAVE` variants, no? (The `..._FAIL` variants where needed so that the launcher could continue with the next variant, but since we now only call the correct variant, we can just stop if something goes wrong?) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2063217552
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 I tested now pull/18753 and pull/18786: both solve both issues JDK-8329420 and JDK-8329581 - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2061302794
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 Just tried to avoid unnecessary duplication of work. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2061191285
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 16 Apr 2024 06:30:55 GMT, Alan Bateman 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 > > Yes, it would be good to try the changes in pull/18753 first. @AlanBateman @wolfseifert Thanks for the comments! Just wanted to note that I tried out https://github.com/openjdk/jdk/pull/18753 and the issue is not resolved. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2059229527
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 16 Apr 2024 08:25:01 GMT, Thomas Stuefe wrote: >> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 912: >> >>> 910: private static final int MAIN_WITHOUT_ARGS = 1; >>> 911: private static final int MAIN_NONSTATIC = 2; >>> 912: private static int mainType = 0; >> >> 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. > > Thinking about this some more, would it not be possible to just use the > mainMethod directly down in C? The changes JEP 463 went through many iterations, it was a fine balance of avoiding too many transitions and upscales, 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1567124485
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Tue, 16 Apr 2024 07:55:26 GMT, Thomas Stuefe 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 > > src/java.base/share/classes/sun/launcher/LauncherHelper.java line 912: > >> 910: private static final int MAIN_WITHOUT_ARGS = 1; >> 911: private static final int MAIN_NONSTATIC = 2; >> 912: private static int mainType = 0; > > 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. Thinking about this some more, would it not be possible to just use the mainMethod directly down in C? - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566948449
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 > Isn't this already fixed by #18753? > Yes, it would be good to try the changes in pull/18753 first. 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. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2058521084
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 Good find. src/java.base/share/classes/sun/launcher/LauncherHelper.java line 912: > 910: private static final int MAIN_WITHOUT_ARGS = 1; > 911: private static final int MAIN_NONSTATIC = 2; > 912: private static int mainType = 0; 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. - PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2002891641 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566896963
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 Yes, it would be good to try the changes in pull/18753 first. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2058328338
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 Isn't this already fixed by https://github.com/openjdk/jdk/pull/18753? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2058185032
Re: RFR: 8329581: Java launcher no longer prints a stack trace
On Mon, 15 Apr 2024 18:25:02 GMT, Sonia Zaldana Calles 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 src/java.base/share/classes/sun/launcher/LauncherHelper.java line 954: > 952: > 953: int mods = mainMethod.getModifiers(); > 954: boolean isStatic = Modifier.isStatic(mods); Can get the value for `isStatic` and `noArgs` from the main type. src/java.base/share/native/libjli/java.c line 645: > 643: res = invokeInstanceMainWithoutArgs(env, mainClass); > 644: break; > 645: } 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566502390 PR Review Comment: https://git.openjdk.org/jdk/pull/18786#discussion_r1566545886
RFR: 8329581: Java launcher no longer prints a stack trace
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 - Commit messages: - 8329581: Java launcher no longer prints a stack trace Changes: https://git.openjdk.org/jdk/pull/18786/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329581 Stats: 53 lines in 2 files changed: 45 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
