Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Thu, 20 Jun 2024 11:38:26 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/man/jnativescan.1 line 121: >> >>> 119: .TP >>> 120: \f[V]--release\f[R] \f[I]version\f[R] >>> 121: Used to specify the Java SE release that specifies the set of >>> restricted >> >> In principle, the release could also affect which methods are native or not? > > Yes, but we don't warn on `native` method references, only on declarations. > So, it would only affect which methods might be `native` in the user code. > But I think that is already implied by the note on multi-release jars? You are right... declarations inside the JDK are... inside the JDK - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647820666
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:35:07 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - review comments >> - add man page > > src/jdk.jdeps/share/man/jnativescan.1 line 166: > >> 164: .fi >> 165: .PP >> 166: \f[V]app.jar (ALL-UNNAMED)\f[R] is the path to the jar file, with the > > This is a good explanation, I'm not sure whether it's necessary, as the > output seems quite self-explanatory. But I'm ok either way. I think that people that get to this section of the man page are the people who either don't understand the output, or (probably more likely) have an idea but want to be sure of what things mean. In both those cases, I think walking through the output is useful. The description of the command line options is probably enough for more people? - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647458338
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Thu, 20 Jun 2024 11:42:04 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/man/jnativescan.1 line 127: >> >>> 125: This option should be set to the version of the runtime under which the >>> 126: application is eventually intended to be run. >>> 127: If this flag is omitted, the version of \f[V]jnativescan\f[R] is used >>> as >> >> Maybe we should point to `Runtime.version()` instead? > > Not sure. If a client is just using the tool, how would they know what > `Runtime.version()` returns? I mean, it's the version of the JDK, but to find > that out they'd have to use another JDK tool (e.g. jshell) to print it out, > or look at the release file. The `jnativescan` version on the other hand is > readily accessible through the `--version` flag. I'll add a note like: `, which is the same as the version of the JDK that the tool belongs to.` - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647442344
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:30:49 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - review comments >> - add man page > > src/jdk.jdeps/share/man/jnativescan.1 line 121: > >> 119: .TP >> 120: \f[V]--release\f[R] \f[I]version\f[R] >> 121: Used to specify the Java SE release that specifies the set of restricted > > In principle, the release could also affect which methods are native or not? Yes, but we don't warn on `native` method references, only on declarations. So, it would only affect which methods might be `native` in the user code. But I think that is already implied by the note on multi-release jars? > src/jdk.jdeps/share/man/jnativescan.1 line 127: > >> 125: This option should be set to the version of the runtime under which the >> 126: application is eventually intended to be run. >> 127: If this flag is omitted, the version of \f[V]jnativescan\f[R] is used as > > Maybe we should point to `Runtime.version()` instead? Not sure. If a client is just using the tool, how would they know what `Runtime.version()` returns? I mean, it's the version of the JDK, but to find that out they'd have to use another JDK tool (e.g. jshell) to print it out, or look at the release file. The `jnativescan` version on the other hand is readily accessible through the `--version` flag. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647436934 PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647441115
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:22:10 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - review comments >> - add man page > > src/jdk.jdeps/share/man/jnativescan.1 line 68: > >> 66: .TP >> 67: \f[V]--class-path\f[R] \f[I]path\f[R] >> 68: Used to specify a path list of class path jar files to be scanned. > > I can't parse "path list of class path jar files". What is the argument here? > I think it has to be a list of jars? If so, one way could be `a list of > paths pointing to the jar files to be scanned` My suggestion is to separate up the "what is this argument?" (is a list of paths, to jar files), from the "semantics" (e.g. the classes in the jarfiles are assumed to belong to the unnamed module). The you can mirror this structure in the module option. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646700042
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:16:46 GMT, Jorn Vernee wrote: >> This PR adds a new JDK tool, called `jnativescan`, that can be used to find >> code that accesses native functionality. Currently this includes `native` >> method declarations, and methods marked with `@Restricted`. >> >> The tool accepts a list of class path and module path entries through >> `--class-path` and `--module-path`, and a set of root modules through >> `--add-modules`, as well as an optional target release with `--release`. >> >> The default mode is for the tool to report all uses of `@Restricted` >> methods, and `native` method declaration in a tree-like structure: >> >> >> app.jar (ALL-UNNAMED): >> main.Main: >> main.Main::main(String[])void references restricted methods: >> java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment >> main.Main::m()void is a native method declaration >> >> >> The `--print-native-access` option can be used print out all the module >> names of modules doing native access in a comma separated list. For class >> path code, this will print out `ALL-UNNAMED`. >> >> Testing: >> - `langtools_jnativescan` tests. >> - Running the tool over jextract's libclang bindings, which use the FFM API, >> and thus has a lot of references to `@Restricted` methods. >> - tier 1-3 > > Jorn Vernee has updated the pull request incrementally with two additional > commits since the last revision: > > - review comments > - add man page Nice changes! src/jdk.jdeps/share/man/jnativescan.1 line 43: > 41: .PP > 42: jnativescan - static analysis tool that scans one or more jar files for > 43: uses of native functionality, such as restricted methods or I think using `functionalities` would be better. Also "restricted method **calls** works better too (so it's restricted method **calls** and native method **declarations. src/jdk.jdeps/share/man/jnativescan.1 line 55: > 53: The \f[V]jnative\f[R] tool is a static analysis tool provided by the JDK > 54: that scans a JAR file for uses of native functionality, such as > 55: restricted methods or \f[V]native\f[R] method declarations. Same comments as above src/jdk.jdeps/share/man/jnativescan.1 line 60: > 58: configuration, as well as a set of root modules, and a target release. > 59: It scans the jars on the class and module paths, and reports uses of > 60: native functionality either in a tree like structure, which also functionalities src/jdk.jdeps/share/man/jnativescan.1 line 68: > 66: .TP > 67: \f[V]--class-path\f[R] \f[I]path\f[R] > 68: Used to specify a path list of class path jar files to be scanned. I can't parse "path list of class path jar files". What is the argument here? I think it has to be a list of jars? If so, one way could be `a list of paths pointing to the jar files to be scanned` src/jdk.jdeps/share/man/jnativescan.1 line 76: > 74: .TP > 75: \f[V]--module-path\f[R] \f[I]path\f[R] > 76: Used to specify a path list of jar files or directories containing This is also hard to read. Perhaps `a list of paths pointing to the jar files to be scanned, or the directories containing said jar files.` src/jdk.jdeps/share/man/jnativescan.1 line 112: > 110: Used to specifiy a comma-separated list of module names that indicate > 111: the root modules to scan. > 112: All the modules in the root set will be scanned, as well as any modules Suggestion: All the root modules will be scanned, as well as any modules src/jdk.jdeps/share/man/jnativescan.1 line 121: > 119: .TP > 120: \f[V]--release\f[R] \f[I]version\f[R] > 121: Used to specify the Java SE release that specifies the set of restricted In principle, the release could also affect which methods are native or not? src/jdk.jdeps/share/man/jnativescan.1 line 127: > 125: This option should be set to the version of the runtime under which the > 126: application is eventually intended to be run. > 127: If this flag is omitted, the version of \f[V]jnativescan\f[R] is used as Maybe we should point to `Runtime.version()` instead? src/jdk.jdeps/share/man/jnativescan.1 line 146: > 144: For the class path, the tool will scan all jar files, including those > 145: found recursively through the \f[V]Class-Path\f[R] manifest attribute. > 146: For the module path, the tool scans all modules in the root module set Note: sometimes you say root modules, sometimes root module set. I'd suggest to pick one and remain consistent. src/jdk.jdeps/share/man/jnativescan.1 line 166: > 164: .fi > 165: .PP > 166: \f[V]app.jar (ALL-UNNAMED)\f[R] is the path to the jar file, with the This is a good explanation, I'm not sure whether it's necessary, as the output seems quite self-explanatory. But I'm ok either way. - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2129052320 PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646696108 PR Review Comment:
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 18:57:43 GMT, Jorn Vernee wrote: >> I can do that, but I think this will always be a bit awkward since these >> types don't have a common super type that exposes the needed information. > >> these types don't have a common super type that exposes the needed >> information > > No wait, they actually do :) That's just `MemberRefEntry`. I was able to eliminate this method entirely, now the caller just creates a `MethodRef` and passes that to the other `isRestrictedMethod` overload. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646691228
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:13:33 GMT, Jorn Vernee wrote:
>> This PR adds a new JDK tool, called `jnativescan`, that can be used to find
>> code that accesses native functionality. Currently this includes `native`
>> method declarations, and methods marked with `@Restricted`.
>>
>> The tool accepts a list of class path and module path entries through
>> `--class-path` and `--module-path`, and a set of root modules through
>> `--add-modules`, as well as an optional target release with `--release`.
>>
>> The default mode is for the tool to report all uses of `@Restricted`
>> methods, and `native` method declaration in a tree-like structure:
>>
>>
>> app.jar (ALL-UNNAMED):
>> main.Main:
>> main.Main::main(String[])void references restricted methods:
>> java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment
>> main.Main::m()void is a native method declaration
>>
>>
>> The `--print-native-access` option can be used print out all the module
>> names of modules doing native access in a comma separated list. For class
>> path code, this will print out `ALL-UNNAMED`.
>>
>> Testing:
>> - `langtools_jnativescan` tests.
>> - Running the tool over jextract's libclang bindings, which use the FFM API,
>> and thus has a lot of references to `@Restricted` methods.
>> - tier 1-3
>
> Jorn Vernee has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - review comments
> - add man page
src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/NativeMethodFinder.java
line 89:
> 87: throw new JNativeScanFatalError("Error while
> processing method: " +
> 88: MethodRef.ofModel(methodModel), e);
> 89: }
I'm re-wrapping the exception here, thrown from `SystemClassResolver::lookup`,
in order to include the method that called the missing system class in the
error message as well. (See also the code added in `Main` to print out an
exception cause if it is a `JNativeScanFatalError`).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646689741
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 19:59:19 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java >> line 76: >> >>> 74: URI location = m.reference().location().orElseThrow(); >>> 75: Path path = Path.of(location); >>> 76: checkRegularJar(path); >> >> if exploded modules aren't supported (as discussed offline), maybe the error >> message generated here should be more specific? > > What do you suggest? Just a note in the error message that exploded > modules/class paths are not supported? Something like that yes - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646692395
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 18:02:08 GMT, Jorn Vernee wrote:
>> src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/ClassResolver.java
>> line 126:
>>
>>> 124:
>>> 125: private static Map packageToSystemModule() {
>>> 126: List descriptors =
>>> ModuleFinder.ofSystem()
>>
>> Is it a problem that we compute the package -> module map using the runtime
>> info (so latest version) but then all the other info is taken from a
>> release-specific symbol file? E.g. say that package "foo" was moved from
>> module "A" to module "B" in version N, and that user passes N - 1 as release
>> to the scan tool - would that work?
>
> That's a good point, I don't think that scenario will work. We should really
> use the release specific info if we can. I think that's relatively easy to
> do, will take a look.
Ok, I managed to implement this, but I don't think we can actually test this
use case, since (AFAIK) there's never been a case of a package being moved to a
different module under the same name.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1646688656
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
> This PR adds a new JDK tool, called `jnativescan`, that can be used to find > code that accesses native functionality. Currently this includes `native` > method declarations, and methods marked with `@Restricted`. > > The tool accepts a list of class path and module path entries through > `--class-path` and `--module-path`, and a set of root modules through > `--add-modules`, as well as an optional target release with `--release`. > > The default mode is for the tool to report all uses of `@Restricted` methods, > and `native` method declaration in a tree-like structure: > > > app.jar (ALL-UNNAMED): > main.Main: > main.Main::main(String[])void references restricted methods: > java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment > main.Main::m()void is a native method declaration > > > The `--print-native-access` option can be used print out all the module names > of modules doing native access in a comma separated list. For class path > code, this will print out `ALL-UNNAMED`. > > Testing: > - `langtools_jnativescan` tests. > - Running the tool over jextract's libclang bindings, which use the FFM API, > and thus has a lot of references to `@Restricted` methods. > - tier 1-3 Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - review comments - add man page - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/861965b7..b5468440 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19774=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19774=01-02 Stats: 645 lines in 10 files changed: 467 ins; 155 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/19774.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774 PR: https://git.openjdk.org/jdk/pull/19774
