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

[Jorn Vernee]

On Tue, 8 Apr 2025 11:41:15 GMT, Jorn Vernee <jvernee 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....
>
> src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java line 91:
> 
>> 89:                              err.println("Error while processing method: " +
>> 90:                                      MethodRef.ofModel(methodModel) + ": " + e.getMessage());
>> 91:                          }
> 
> This catch block is per-method. It means that if the rest of the method contained references to restricted methods, we would not see them. The catch block should be moved to be just around the call to `isRestricted`, which can fail. Then we can record the error and keep iterating over the other instructions.

Also, instead of passing `err` down to this code, I think we should define an interface for `NativeMethodFinder` to log diagnostics instead. e.g. something like:


interface Diagnostics {
    void error(MethodRef context, JNativeScanFatalError error);
} 


`JNativeScanTask` can then define the implementation how it wants. The benefit of that is that we have more freedom to print the errors how we want. For instance, I think we should first collect all the errors, and de-duplicate error messages before printing them, so that if there are 10 references to the same system class, we only print the error message once, and we can print a header like: `Errors while processing classes: ...`

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

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