8353840: JNativeScan should not abort for missing classes #24499

danishnawab · 2025-04-08T08:18:36Z

Description

https://bugs.openjdk.org/browse/JDK-8353840

Existing behavior

Log the error message and terminate in case of missing system class

$ jnativescan --class-path reactor-core-3.7.4.jar; echo "Exit code: $?"

ERROR: Error while processing method: reactor.core.publisher.CallSiteSupplierFactory$SharedSecretsCallSiteSupplierFactory$TracingException::get()String
CAUSED BY: System class can not be found: sun.misc.JavaLangAccess
Exit code: 1

New behavior

Still log the error message about the missing system class, but continue the analysis

$ build/macosx-aarch64-server-release/jdk/bin/jnativescan --class-path reactor-core-3.7.4.jar; echo "Exit code: $?" 

  <no restricted methods>
Error while processing method: reactor.core.publisher.CallSiteSupplierFactory$SharedSecretsCallSiteSupplierFactory$TracingException::get()String: System class can not be found: sun.misc.JavaLangAccess
Error while processing method: reactor.core.publisher.CallSiteSupplierFactory$SharedSecretsCallSiteSupplierFactory$TracingException::<clinit>()void: System class can not be found: sun.misc.SharedSecrets
Error while processing method: reactor.core.publisher.CallSiteSupplierFactory$SharedSecretsCallSiteSupplierFactory::<clinit>()void: System class can not be found: sun.misc.SharedSecrets
Exit code: 0

Design choices

Propagate err all the way to NativeMethodFinder which can log the error to it, but continue with the analysis

Alternatives considered

Instead of propagating err downstream, adapt the outer try-catch in com.sun.tools.jnativescan.Main#run.
- Con: This would require complicated error recovery synchronization between Main and JNativeScanTask to resume the scanning after the exception
Instead of adapting the catch block in com.sun.tools.jnativescan.NativeMethodFinder#findAll, don't even surface the exception from com.sun.tools.jnativescan.ClassResolver.SystemModuleClassResolver#lookup rather log it right in ClassResolver
- Con: We don't have access to MethodRef/MethodModel in ClassResolver#lookup which will make the error message less detailed/useful

stdout vs stderr

One could argue that since this is a non-terminal error, perhaps it should be logged to stdout. However, my thinking was that while it may be non-terminal, it is still an "error", and thus should be logged to stderr. I am happy to revisit this decision, if needed.

Testing

The existing test TestMissingSystemClass#testSingleJarClassPath has been adapted to test for successful execution, as well as verify the stdout output.
I considered briefly to update the test setup in a way that we see some restricted methods on the stdout instead of just <no restricted methods>, but it was unclear if that would really add any additional value since the main purpose of this test is just to ascertain the missing system class (with successful execution from now onward).

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8353840: JNativeScan should not abort for missing classes (Enhancement - P4)

Reviewers

Jorn Vernee (@JornVernee - Reviewer)
Chen Liang (@liach - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24499/head:pull/24499
$ git checkout pull/24499

Update a local copy of the PR:
$ git checkout pull/24499
$ git pull https://git.openjdk.org/jdk.git pull/24499/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24499

View PR using the GUI difftool:
$ git pr show -t 24499

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24499.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-08T08:19:35Z

👋 Welcome back danishnawab! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-08T08:19:51Z

@danishnawab This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8353840: JNativeScan should not abort for missing classes

Reviewed-by: jvernee, liach

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 49 new commits pushed to the master branch:

567c688: 8353597: Refactor handling VM options for AOT cache input and output
4dc9e58: 8310310: Migrate CreateSymbols tool in make/langtools to Classfile API
7a7b9ed: 8353727: HeapDumpPath doesn't expand %p
... and 46 more: https://git.openjdk.org/jdk/compare/9844c1c52b9b7b6959d4e5cc626495c83f4aa9d4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JornVernee, @liach) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-04-08T08:20:39Z

@danishnawab The following labels will be automatically applied to this pull request:

compiler
core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-08T08:26:23Z

Webrevs

JornVernee

I've updated the issue title to start with a capital letter.

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java

JornVernee · 2025-04-08T11:49:29Z

Also, note that there was some other cleanup I wanted to do in this area first: #24493 which will likely result in merge conflicts.

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java

danishnawab · 2025-04-08T13:42:56Z

test/langtools/tools/jnativescan/TestMissingSystemClass.java

+                .stderrShouldContain("java.lang.Compiler")
+                .stderrAsLines();
+
+        assertEquals(2, stderr.size(), "Unexpected number of lines in stderr");


To ensure that the two calls to java.lang.Compiler.enable have been de-duped into one.

danishnawab · 2025-04-08T13:43:53Z

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java

@@ -192,4 +201,8 @@ public static String qualName(ClassDesc desc) {
        String packagePrefix = desc.packageName().isEmpty() ? "" : desc.packageName() + ".";
        return packagePrefix + desc.displayName();
    }
+
+    interface Diagnostics {


Added as an inner type since there was already precedence for that with com.sun.tools.jnativescan.JNativeScanTask.Action

JornVernee

Changes look good, thanks!

Please note that we will have to wait 24 hours before integrating, in order to give other reviews time to look as well.

danishnawab · 2025-04-08T14:25:11Z

Changes look good, thanks!

Please note that we will have to wait 24 hours before integrating, in order to give other reviews time to look as well.

Great, thanks for your review!
I will wait until tomorrow to issue the /integrate (as instructed by openjdk[bot]). Since I am not an OpenJDK committer, I will request you to please sponsor it.

danishnawab · 2025-04-09T14:26:00Z

/integrate

openjdk · 2025-04-09T14:27:31Z

@danishnawab
Your change (at version 1de3fec) is now ready to be sponsored by a Committer.

liach · 2025-04-09T15:11:11Z

Hello @danishnawab, I have updated the issue title to "JNativeScan should not abort for missing classes" to be more accurate for the changes done. Can you update the PR title here too?

danishnawab · 2025-04-09T15:14:44Z

Hello @danishnawab, I have updated the issue title to "JNativeScan should not abort for missing classes" to be more accurate for the changes done. Can you update the PR title here too?

@liach it's done.

liach · 2025-04-09T15:24:07Z

Thanks!

/sponsor

openjdk · 2025-04-09T15:25:28Z

Going to push as commit 5f2a604.
Since your change was applied there have been 50 commits pushed to the master branch:

1f21da7: 8351462: Improve robustness of String concatenation
567c688: 8353597: Refactor handling VM options for AOT cache input and output
4dc9e58: 8310310: Migrate CreateSymbols tool in make/langtools to Classfile API
... and 47 more: https://git.openjdk.org/jdk/compare/9844c1c52b9b7b6959d4e5cc626495c83f4aa9d4...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-09T15:25:46Z

@liach @danishnawab Pushed as commit 5f2a604.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8353840: jnativescan should not throw error for missing system classes

e658c46

openjdk bot added the rfr Pull request is ready for review label Apr 8, 2025

openjdk bot added core-libs [email protected] compiler [email protected] labels Apr 8, 2025

danishnawab changed the title ~~8353840: jnativescan should not throw error for missing system classes~~ 8353840: Jnativescan should not throw error for missing system classes Apr 8, 2025

openjdk bot changed the title ~~8353840: Jnativescan should not throw error for missing system classes~~ 8353840: jnativescan should not throw error for missing system classes Apr 8, 2025

JornVernee suggested changes Apr 8, 2025

View reviewed changes

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java Outdated Show resolved Hide resolved

openjdk bot changed the title ~~8353840: jnativescan should not throw error for missing system classes~~ 8353840: JNativeScan should not throw error for missing system classes Apr 8, 2025

JornVernee reviewed Apr 8, 2025

View reviewed changes

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java Outdated Show resolved Hide resolved

src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java Outdated Show resolved Hide resolved

8353840: improve error logging

3aaef9d

danishnawab requested a review from JornVernee April 8, 2025 13:42

danishnawab commented Apr 8, 2025

View reviewed changes

8353840: remove unnecessary final

1de3fec

JornVernee approved these changes Apr 8, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 8, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 9, 2025

liach approved these changes Apr 9, 2025

View reviewed changes

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Apr 9, 2025

danishnawab changed the title ~~8353840: JNativeScan should not throw error for missing system classes~~ 8353840: JNativeScan should not abort for missing classes Apr 9, 2025

openjdk bot added sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Apr 9, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2025

openjdk bot closed this Apr 9, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8353840: JNativeScan should not abort for missing classes #24499

8353840: JNativeScan should not abort for missing classes #24499

danishnawab commented Apr 8, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025 •

edited

Loading

openjdk bot commented Apr 8, 2025

mlbridge bot commented Apr 8, 2025 •

edited

Loading

JornVernee left a comment •

edited

Loading

JornVernee commented Apr 8, 2025

danishnawab Apr 8, 2025

danishnawab Apr 8, 2025

JornVernee left a comment

danishnawab commented Apr 8, 2025

danishnawab commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

liach commented Apr 9, 2025

danishnawab commented Apr 9, 2025

liach commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

8353840: JNativeScan should not abort for missing classes #24499

8353840: JNativeScan should not abort for missing classes #24499

Conversation

danishnawab commented Apr 8, 2025 • edited by openjdk bot Loading

Description

Existing behavior

New behavior

Design choices

Alternatives considered

stdout vs stderr

Testing

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 8, 2025

openjdk bot commented Apr 8, 2025 • edited Loading

openjdk bot commented Apr 8, 2025

mlbridge bot commented Apr 8, 2025 • edited Loading

Webrevs

JornVernee left a comment • edited Loading

Choose a reason for hiding this comment

JornVernee commented Apr 8, 2025

danishnawab Apr 8, 2025

Choose a reason for hiding this comment

danishnawab Apr 8, 2025

Choose a reason for hiding this comment

JornVernee left a comment

Choose a reason for hiding this comment

danishnawab commented Apr 8, 2025

danishnawab commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

liach commented Apr 9, 2025

danishnawab commented Apr 9, 2025

liach commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

danishnawab commented Apr 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 8, 2025 •

edited

Loading

mlbridge bot commented Apr 8, 2025 •

edited

Loading

JornVernee left a comment •

edited

Loading