On Thu, 18 Apr 2024 07:34:24 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > 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
