On Fri, 19 Apr 2024 17:04:30 GMT, Mandy Chung <mch...@openjdk.org> 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.<clinit>(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
