RFR: 8341184: Clean up the interaction between the launcher native code and the LauncherHelper [v16]

[Chen Liang]

On Mon, 24 Feb 2025 09:46:29 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change, which simplifies the interaction between the `java` launcher's native code with the `sun.launcher.LauncherHelper`? 
>> 
>> As noted in https://bugs.openjdk.org/browse/JDK-8341184, this proposed change reduces the back and forth between the launcher's native code and the `LauncherHelper` class. This also removes the additional reflective lookups from the native code after the main class and main method have been determined by the `LauncherHelper`. 
>> 
>> Although this is a clean up of the code, the changes in the `LauncherHelper` to return a `MainEntry` have been done in a way to facilitate additional upcoming changes in this area, which propose to get rid of the JAR manifest parsing from the launcher's native code.
>> 
>> No new tests have been added. Existing tests in tier1, tier2 and tier3 continue to pass.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits:
> 
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/e410af00...d1ac5174

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 848:

> 846:                     try {
> 847:                         jarFile = new JarFile(what);
> 848:                         cn = getMainClassFromJar(jarFile);

Why is this switch block moved into the `catch (LinkageError le)` block? Is it for this handling?

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 915:

> 913:     // Determine the presence of a valid main method on the MainEntry.Builder.mainClass.
> 914:     // abort() if validation fails.
> 915:     private static MainEntry requireValidMainMethod(MainEntry.Builder builder) {

I think the `MainEntry.Builder` class is redundant - we should just pass in the local variables here.

src/java.base/share/native/libjli/java.c line 477:

> 475:     CHECK_EXCEPTION_NULL_LEAVE(mainEntry);
> 476:     const jclass mainEntryType = (*env)->GetObjectClass(env, mainEntry); // sun/launcher/LauncherHelper$MainEntry
> 477:     const jfieldID mainClassFid = (*env)->GetFieldID(env, mainEntryType, "mainClass", "Ljava/lang/Class;");

Using record fields directly in JNI feels risky. Can we switch to use the getters instead, even though access control is absent in JNI?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21256#discussion_r2009307726
PR Review Comment: https://git.openjdk.org/jdk/pull/21256#discussion_r2009307443
PR Review Comment: https://git.openjdk.org/jdk/pull/21256#discussion_r2009308299