On Tue, 8 Apr 2025 11:41:15 GMT, Jorn Vernee <jver...@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
