On Mon, 24 Feb 2025 09:46:29 GMT, Jaikiran Pai <j...@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
