RFR: 8353840: JNativeScan should not throw error for missing system classes [v2]

[Danish Nawab]

On Tue, 8 Apr 2025 13:43:54 GMT, Danish Nawab <duke at openjdk.org> wrote:

>> ## 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....
>
> Danish Nawab has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8353840: improve error logging

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

> 203:     }
> 204: 
> 205:     interface Diagnostics {

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

test/langtools/tools/jnativescan/TestMissingSystemClass.java line 64:

> 62:                 .stderrAsLines();
> 63: 
> 64:         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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24499#discussion_r2033227648
PR Review Comment: https://git.openjdk.org/jdk/pull/24499#discussion_r2033225623