jdk
jdk copied to clipboard
openjdk
8317611: Add a tool like jdeprscan to find usage of restricted methods
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_jnativescantests.- Running the tool over jextract's libclang bindings, which use the FFM API, and thus has a lot of references to
@Restrictedmethods. - tier 1-3
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [x] Change requires CSR request JDK-8334569 to be approved
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issues
- JDK-8317611: Add a tool like jdeprscan to find usage of restricted methods (Enhancement - P3)
- JDK-8334569: Add a tool like jdeprscan to find usage of restricted methods (CSR)
Reviewers
- Magnus Ihse Bursie (@magicus - Reviewer) đ Re-review required (review applies to 9e8dad03)
- Maurizio Cimadamore (@mcimadamore - Reviewer) đ Re-review required (review applies to b5468440)
- Jan Lahoda (@lahodaj - Reviewer) đ Re-review required (review applies to 40ca91ed)
- Alan Bateman (@AlanBateman - Reviewer) đ Re-review required (review applies to c597f247)
- Julian Waters (@TheShermanTanker - Committer) đ Re-review required (review applies to c597f247)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774
$ git checkout pull/19774
Update a local copy of the PR:
$ git checkout pull/19774
$ git pull https://git.openjdk.org/jdk.git pull/19774/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19774
View PR using the GUI difftool:
$ git pr show -t 19774
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19774.diff
Webrev
:wave: Welcome back jvernee! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@JornVernee 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:
8317611: Add a tool like jdeprscan to find usage of restricted methods
Reviewed-by: alanb, ihse, mcimadamore, jlahoda, jwaters
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 12 new commits pushed to the master branch:
- 953c35eb5bff49ec5f7dbb25edd8a324b94318eb: 8335824: Test gc/arguments/TestMinInitialErgonomics.java is timing out
- c34a1b7013b27a8a214f63387bd528a90342a416: 8335861: Problem list compiler/vectorization/TestFloat16VectorConvChain.java
- c5a668bb653feb3408a9efa3274ceabf9f01a2c7: 8334231: Optimize MethodData layout
- 540188fdebd089d4145eca18c0f95bf338cbcefc: 8334445: Parallel: Decouple maximum compaction from SoftReference clearing
- 3cce31ad8877ec62429981871bcb0067770f9ccb: 8335643: serviceability/dcmd/vm tests fail for ZGC after JDK-8322475
- 55fd1ed228ea3c42aaf92579e5dcb818fe14351d: 8333890: Fatal error in auto-vectorizer with float16 kernel.
- 02956ab6e161ca8556a73f328f79bcbfba997cbc: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph
- 3f37c5718d676b7001e6a084aed3ba645745a144: 8335806: RISC-V: Corrected typos Bizarrely
- 6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef: 8333884: MemorySegment::reinterpret removes read-only property
- b83766e59063a41ea8801ac9e7c15dce67727c62: 8335632: jdk/jfr/api/consumer/streaming/TestJVMExit.java failed with "Process [...] is no longer alive"
- ... and 2 more: https://git.openjdk.org/jdk/compare/194425d7875ef42fce52516ed59c81ee97720399...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.
âĄī¸ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@JornVernee The following labels will be automatically applied to this pull request:
buildcompilercore-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.
@JornVernee has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@JornVernee please create a CSR request for issue JDK-8317611 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
Webrevs
- 11: Full - Incremental (1d1ff010)
- 10: Full - Incremental (5afb3561)
- 09: Full - Incremental (c597f247)
- 08: Full - Incremental (40ca91ed)
- 07: Full - Incremental (a046f6fe)
- 06: Full - Incremental (a4723494)
- 05: Full - Incremental (75c9a6af)
- 04: Full - Incremental (06f53e31)
- 03: Full - Incremental (4c6abddf)
- 02: Full - Incremental (b5468440)
- 01: Full - Incremental (861965b7)
- 00: Full (9e8dad03)
One more tool. or... Could this be coalesced into a tool that does deprscan and restricted methods, and other "lint" type checks? I might go so far as to suggest it be extensible and accept patterns or regular expressions for matching classes, methods, fields, etc.
@magicus The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).
One more tool. or... Could this be coalesced into a tool that does deprscan and restricted methods, and other "lint" type checks? I might go so far as to suggest it be extensible and accept patterns or regular expressions for matching classes, methods, fields, etc.
Discussed this with several people ahead of this PR. That is something we might want to do in the future, but we want this scanning feature to be ready for the JNI restriction, which is proposed to target Java 24.
Other work might include unifying the code bases of jdeprscan, jdeps, and exposing a general purpose static-analysis API.
I've also added a first version of the man page for the tool. I'm not every familiar with the JDK man pages, as I rarely use them myself, so there's probably things missing. Please let me know.
Just as a guest here. Just theoretically, wouldn't it be better to create one uniform tool to detect all "undesired" APIs. This would be more flexible, and potentially could be adapted to use some kind of "meticulous" mode, that will also indicate usage of public APIs, that, while never will be removed, are recommended to remove in favor of new alternatives (like old Date API or old collections". Just a thought btw, not a proposal for now
wouldn't it be better to create one uniform tool
See my reply here: https://github.com/openjdk/jdk/pull/19774#issuecomment-2179078565
One thing to check is usages like jnativescan foo, should that fail as the tool doesn't take args?
Another thing is that using joptsimple gives up a bit of control, e.g. the help output shows the parameter for --class-path as <String> where the java launcher and other tools will show "path" or "class path". Same thing with --release where it shows <String> where jar, javac, and other tools will say "release". Not important for now, just comments from trying out the tool.
jnativescan --version prints the value of Runtime.version where jdeprscan and some of the other tools print System.getProperty("java.version"). Just something to check as they might look inconsistent.
Another thing is that using joptsimple gives up a bit of control, e.g. the help output shows the parameter for --class-path as
<String>where the java launcher and other tools will show "path" or "class path". Same thing with--releasewhere it shows<String>where jar, javac, and other tools will say "release". Not important for now, just comments from trying out the tool.
It seems that it is possible in joptsimple to attach a description to the argument as well, and get something like:
--module-path <String: module path> The class path as used at runtime
It looks like we could also customize the argument type and get something like:
--module-path <ModulePath>
(we'd just need to add a ModulePath class with a valueOf(String) method)
I've massaged the parsing code to where the help message now looks like this:
...
Option Description
------ -----------
-?, -h, --help help
--add-modules <String> List of root modules to scan
--class-path <Path> The class path as used at runtime
--module-path <Path> The module path as used at runtime
--print-native-access print a comma separated list of modules that may
perform native access operations. ALL-UNNAMED is used
to indicate unnamed modules.
--release <Version> The runtime version that will run the application
--version Print version information and exit
I'm not sure if joptsimple has a way to display the option arguments like <Path>[,<Path>] to indicate multiple values (at least I couldn't find it immediately).
It looks like it's possible to get even more custom by using a custom HelpFormatter as well, if we wanted.
I've massaged the parsing code to where the help message now looks like this:
It is better but it might mean looking at HelpFormatter as you mentioned,. Right now the usage message --add-modules <String> whereas the java --help will print --add-modules <module name>[,<module name>...] and javac --help will print --add-modules <module>(,<module>)*.
Not super important for now, I think the main thing with this PR is getting agreement to add a tool with a short shelf life. It's clear there is a longer term story here to have one tool to do static analysis and replace (or front) jdeps, jdeprscan and this the new tool here.
Fixed a small bug: https://github.com/openjdk/jdk/pull/19774/commits/40ca91ed47be7697cb720c1b1155d5192e262cc3
I think there's potentially more cleanup that can be done. ClassResolver is trying to do too many things atm, I think. It can both iterate over all classes, and do lookups of individual classes, but we only either use one or the other, either to find all classes to scan, or to lookup system classes (the system resolver doesn't even support iterating). I think ClassResolver would be better off if it just supported lookups, and then NativeMethodFinder operated on individual ClassModules instead of iterating over all classes. The iteration can be lifted into JNativeScanTask::run, and this spreads out the nesting a bit as well so code doesn't become too hard to follow. I sketched out that idea, and it looks better to me, but there's quite a lot of code motion, without any functional difference (commit), so I'll leave it for a followup (unless people want it in this PR).
It looks like it's possible to get even more custom by using a custom
HelpFormatteras well, if we wanted.
I think what you have is okay for the first integration, just looks odd to have the help/usage say "String" and "Path". Using a help formatter can be something for a follow-up issue, assuming it feasible.
I'm running one more tier 1-3 job before integrating. Will need a final sign-off from a Reviewer for that as well. (Either way, I'll save the integration until next week)
@JornVernee This pull request has not yet been marked as ready for integration.
@JornVernee The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).
I've lowered the number of reviewers to 1 again, required for the final sign-off. The number of reviewers was set to 2 by Magnus here, because he only reviewed the build changes, but those didn't change since his last review.
Going to push as commit cec222e46065fc15db3f2eb241d3607d605ab580.
Since your change was applied there have been 12 commits pushed to the master branch:
- 953c35eb5bff49ec5f7dbb25edd8a324b94318eb: 8335824: Test gc/arguments/TestMinInitialErgonomics.java is timing out
- c34a1b7013b27a8a214f63387bd528a90342a416: 8335861: Problem list compiler/vectorization/TestFloat16VectorConvChain.java
- c5a668bb653feb3408a9efa3274ceabf9f01a2c7: 8334231: Optimize MethodData layout
- 540188fdebd089d4145eca18c0f95bf338cbcefc: 8334445: Parallel: Decouple maximum compaction from SoftReference clearing
- 3cce31ad8877ec62429981871bcb0067770f9ccb: 8335643: serviceability/dcmd/vm tests fail for ZGC after JDK-8322475
- 55fd1ed228ea3c42aaf92579e5dcb818fe14351d: 8333890: Fatal error in auto-vectorizer with float16 kernel.
- 02956ab6e161ca8556a73f328f79bcbfba997cbc: 8332163: C2 SuperWord: refactor PacksetGraph and SuperWord::output into VTransformGraph
- 3f37c5718d676b7001e6a084aed3ba645745a144: 8335806: RISC-V: Corrected typos Bizarrely
- 6f7f0f1de05fdc0f6a88ccd90b806e8a5c5074ef: 8333884: MemorySegment::reinterpret removes read-only property
- b83766e59063a41ea8801ac9e7c15dce67727c62: 8335632: jdk/jfr/api/consumer/streaming/TestJVMExit.java failed with "Process [...] is no longer alive"
- ... and 2 more: https://git.openjdk.org/jdk/compare/194425d7875ef42fce52516ed59c81ee97720399...master
Your commit was automatically rebased without conflicts.
@JornVernee Pushed as commit cec222e46065fc15db3f2eb241d3607d605ab580.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.
I assume you'll create follow-up issues in JBS for the Class-Path attribute, improvement the usage/help output to replace the "Path"/"String" types.
I've filed: https://bugs.openjdk.org/browse/JDK-8335877 and https://bugs.openjdk.org/browse/JDK-8335878