Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Fri, 21 Jun 2024 18:38:07 GMT, Jorn Vernee wrote:
>> test/langtools/tools/jnativescan/TestJNativeScan.java line 174:
>>
>>> 172: "-add-modules",
>>> "org.singlejar,org.myapp",
>>> 173: "--print-native-access"))
>>> 174: .stdoutShouldContain("org.singlejar")
>>
>> It is a small thing, bu was there a consideration for a stronger assert on
>> the output, checking that the output is precisely something like
>> `--enable-native-access org.lib,org.service,org.singlejar`? Would require
>> that the output is stable, which may be tricky, but also not a bad property.
>> Just an idea for consideration.
>
> Good point. The result should be 'clean' and directly forward-able to
> `--enable-native-access`. (So in this case it should be exactly
> `org.singlejar`)
I'm fixing this test, and also making the output sorted. That is required for
the output stability, but I also thought that would be a nice property in case
a user wants to diff the tool's outputs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649418854
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Fri, 21 Jun 2024 18:37:01 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417: >> >>> 415: return false; >>> 416: } >>> 417: JavaFileManager fm = >>> pp.getPlatformTrusted(release).getFileManager(); >> >> Not sure if this change is necessary. I believe `release` is verified to be >> a valid platform name at this point, so even with the new check, it should >> still work. (And `getPlatformTrusted` could possibly be eliminated.) But >> maybe I am missing something? > > I wanted to avoid adding exception handling code here, since > `PlatformNotSupported` is a checked exception. Ah, OK, I didn't realize that. Seems fine then! - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649315863
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Fri, 21 Jun 2024 18:31:00 GMT, Jan Lahoda wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add extra test for missing root modules
>
> src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417:
>
>> 415: return false;
>> 416: }
>> 417: JavaFileManager fm =
>> pp.getPlatformTrusted(release).getFileManager();
>
> Not sure if this change is necessary. I believe `release` is verified to be a
> valid platform name at this point, so even with the new check, it should
> still work. (And `getPlatformTrusted` could possibly be eliminated.) But
> maybe I am missing something?
I wanted to avoid adding exception handling code here, since
`PlatformNotSupported` is a checked exception.
> test/langtools/tools/jnativescan/TestJNativeScan.java line 174:
>
>> 172: "-add-modules",
>> "org.singlejar,org.myapp",
>> 173: "--print-native-access"))
>> 174: .stdoutShouldContain("org.singlejar")
>
> It is a small thing, bu was there a consideration for a stronger assert on
> the output, checking that the output is precisely something like
> `--enable-native-access org.lib,org.service,org.singlejar`? Would require
> that the output is stable, which may be tricky, but also not a bad property.
> Just an idea for consideration.
Good point. The result should be 'clean' and directly forward-able to
`--enable-native-access`. (So in this case it should be exactly `org.singlejar`)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649312391
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649313272
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Thu, 20 Jun 2024 19:45:29 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 one additional
> commit since the last revision:
>
> add extra test for missing root modules
Overall looks great to me. Some minor comments inline.
src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java
line 87:
> 85: @Override
> 86: public PlatformDescription getPlatform(String platformName, String
> options) throws PlatformNotSupported {
> 87: if (Source.lookup(platformName) == null) {
`Source.lookup` is probably not the right way to check whether the platform is
supported - the `Source` enum may (and does) contain versions for which we
don't have the historical API record. I think
`SUPPORTED_JAVA_PLATFORM_VERSIONS.contains(platformName)` would work better,
since the content is read directly from `ct.sym`.
src/jdk.jdeps/share/classes/com/sun/tools/jdeprscan/Main.java line 417:
> 415: return false;
> 416: }
> 417: JavaFileManager fm =
> pp.getPlatformTrusted(release).getFileManager();
Not sure if this change is necessary. I believe `release` is verified to be a
valid platform name at this point, so even with the new check, it should still
work. (And `getPlatformTrusted` could possibly be eliminated.) But maybe I am
missing something?
src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/ClassResolver.java line
125:
> 123: private static Map
> packageToSystemModule(JavaFileManager platformFileManager) {
> 124: try {
> 125: Set locations =
> platformFileManager.listLocationsForModules(
FWIW: +1 on using the modules from the `ct.sym` rather than runtime modules
here!
test/langtools/tools/jnativescan/TestJNativeScan.java line 174:
> 172: "-add-modules",
> "org.singlejar,org.myapp",
> 173: "--print-native-access"))
> 174: .stdoutShouldContain("org.singlejar")
It is a small thing, bu was there a consideration for a stronger assert on the
output, checking that the output is precisely something like
`--enable-native-access org.lib,org.service,org.singlejar`? Would require that
the output is stable, which may be tricky, but also not a bad property. Just an
idea for consideration.
-
PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2133198642
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649303668
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649307310
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649297021
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649294137
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
On Thu, 20 Jun 2024 16:58:55 GMT, Alan Bateman wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/platform/JDKPlatformProvider.java
>> line 93:
>>
>>> 91: }
>>> 92:
>>> 93: public PlatformDescription getPlatformTrusted(String platformName) {
>>
>> I noticed that `getPlatform` was not throwing an exception if the release
>> was not valid, which then later results in an NPE. I've added an explicit
>> check here instead. The caller can then catch the exception.
>
> I see the "options" arg is not used so maybe another approach here is a
> one-arg getPlatform that throws PlatformNotSupported and then migrate the
> other usages at another time.
>
> (This is just a passing comment, not important for the core of this feature).
We probably can eliminate the `options` (which is a historical artifact), but I
don't think it needs to be done here
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1649307870
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
> 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 one additional commit since the last revision: add extra test for missing root modules - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/75c9a6af..a4723494 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19774&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19774&range=05-06 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 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
