Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v12]
On Fri, 5 Jul 2024 13:44: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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 29 additional > commits since the last revision: > > - Merge branch 'master' into jnativescan > - ofInvokeInstruction > - use instance resolveAndBind + use junit in tests > - de-dupe on path, not module name > - Add support for module directories + class path directories > - sort output for easier diffs > - Jan comments > - add extra test for missing root modules > - review comments Alan > - update man page header to be consisten with the others > - ... and 19 more: https://git.openjdk.org/jdk/compare/a6e3a992...1d1ff010 Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2162567660
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v10]
On Fri, 28 Jun 2024 19:43:36 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: > > use instance resolveAndBind + use junit in tests Thanks for all the work on this, it will be useful to have this tool in the JDK. I don't have any other comments. 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. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2149370644
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v9]
On Mon, 24 Jun 2024 12:57:39 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: > > de-dupe on path, not module name src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java line 69: > 67: } > 68: Configuration config = Configuration.resolveAndBind(moduleFinder, > List.of(systemConfiguration()), > 69: ModuleFinder.of(), rootModules); I think the module path should work like other tools so the "moduleFinder" should be "after" finder, not the "before" finder. In other words, the modules that are observable on the application module path don't override the system modules. If you want you can use an instance method here, like this: `systemConfiguration().resovleAndBind(ModuleFinder.of(), moduleFinder, rootModules)` - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1659028434
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v9]
On Mon, 24 Jun 2024 12:57:39 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:
>
> de-dupe on path, not module name
src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java line
105:
> 103: for (String classPathEntry : classPathAttribute) {
> 104: Path otherJar = parentDir != null
> 105: ? parentDir.resolve(classPathEntry)
We'lll need to create a follow on issue to re-examine this as the value of a
Class-Path attribute aren't sequence of relative URIs (with an optional "file"
URI scheme) rather than file paths. Treating it as a file path may work in some
cases but won't work once you encounter cases that use escaping.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1659018308
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v9]
On Mon, 24 Jun 2024 12:57:39 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: > > de-dupe on path, not module name test/langtools/tools/jnativescan/TestJNativeScan.java line 33: > 31: * cases.classpath.app.App > 32: * cases.classpath.unnamed_package.UnnamedPackage > 33: * @run testng TestJNativeScan We've using JUnit rather than in TestNG for new tests, this one looks strange forward to move if we want. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1659011377
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 19:37:23 GMT, Jorn Vernee wrote: > It looks like it's possible to get even more custom by using a custom > `HelpFormatter` as 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. - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2197088172
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 19:37:23 GMT, Jorn Vernee wrote: > 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 ` whereas the java --help will print `--add-modules [,...]` and javac --help will print `--add-modules (,)*`. 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. - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2182708610
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 17:40:25 GMT, Alan Bateman wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update man page header to be consisten with the others > > src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java > line 113: > >> 111: // Class-Path attribute specifies that jars that >> 112: // are not found are simply ignored. Do the same >> here >> 113: classPathJars.offer(otherJar); > > The expansion of the class path looks right but the Class-Path attribute is > awkward to deal with as its relative URI rather than a file path. More on > this this later. Not important for now but the expansion will probably need to a "visited" set to detect cycles (yes, it is possible). - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647934012
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 12:11:25 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: > > update man page header to be consisten with the others src/jdk.jdeps/share/classes/com/sun/tools/jnativescan/JNativeScanTask.java line 113: > 111: // Class-Path attribute specifies that jars that > 112: // are not found are simply ignored. Do the same here > 113: classPathJars.offer(otherJar); The expansion of the class path looks right but the Class-Path attribute is awkward to deal with as its relative URI rather than a file path. More on this this later. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647929185
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Wed, 19 Jun 2024 21:13:33 GMT, Maurizio Cimadamore wrote: >> What do you suggest? Just a note in the error message that exploded >> modules/class paths are not supported? > > Something like that yes An altermative is to use ResolvedModule::reference to get a ModuleReference, then use its open method to open the contents of the module to get a ModuleReader. That will give you a stream over the names of all resources in the module. It will work for all exploded and packaged modules. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647911987
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Tue, 18 Jun 2024 16:35:10 GMT, Jorn Vernee wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update man page header to be consisten with the others
>
> 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).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647877795
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 12:11:25 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:
>
> update man page header to be consisten with the others
`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.
-
PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2181075350
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 12:11:25 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: > > update man page header to be consisten with the others 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 `` where the java launcher and other tools will show "path" or "class path". Same thing with `--release` where it shows `` where jar, javac, and other tools will say "release". Not important for now, just comments from trying out the tool. - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2181070909
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows The changes to the launcher look okay. The move from `ifdef STATIC_BUILD` to `JLI_IsStaticallyLinked` is quite nice. Having libjdwp link to libjvm was a surprise but I think okay. I think it would be useful to provide a brief summary on the when/where the static builds are tested to ensure that the changes don't bit rot. I realise we already have static builds but it isn't obvious where this is tested. src/hotspot/share/runtime/linkType.cpp line 36: > 34: return JNI_TRUE; > 35: #else > 36: return JNI_FALSE; bool != jboolean, I assume you don't want that. The naming is a bit unusual, a function that returns a boolean is usually name is_XXX, but maybe there is reason for this? - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2180635747 PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1647480992
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Wed, 12 Jun 2024 16:54:54 GMT, Severin Gehwolf wrote: > Could you please help review it? Thanks! Yes, on my list. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2167341692
Re: RFR: 8334036: Update JCov for class file version 68
On Tue, 11 Jun 2024 19:02:29 GMT, Alexandre Iline wrote: > Update JCov for class file version 68 Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19665#pullrequestreview-2111285609
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 10:42:20 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix default description of keep-packaged-modules > > I've read through all src changes. I think Sundar is looking at the code > changes too. > > The overall design seems solid. I know it took a long time to get there but > this is nature of a feature like this. At this point I regret that there > isn't a JEP. A JEP would have captured the motivation, outlined the design, > the reasoning for the restrictions, and document the design > choices/directions that have been prototyped. If there isn't a JEP then I > suppose it can come later if the feature is progressed and there is > discussion about making it the default rather than opt-in at build configure > time. > > As cleanup, I think we will need to bring some consistency to the terminology > and phrasing in documentation, code and comments. Right now there is > "run-time linking", "linkable run-time", "run-time linkable JDK image", > "linkable jimage". > > Also as cleanup, I think the code needs more comments. There is no JEP right > now with an authoritive description of the feature so anyone maintaining this > code will have to figure out a lot of details. It feels like there should be > somehting that documents the effect of --enable-runtime-link-image, how the > diffs are generated and saved, and how they are used by jlink. There is also > a lot of new code in ImageFileCreator and JlinkTask that is asking for some > method descriptions so that anyone touching this code can quickly understand > what these methods are doing. I don't want to get into code style issues now > but I think it would be helpful for future maintainers to avoid more of the > overfly long lines if possible (some of them are 150, 160, 170+ and really > hard to look at code side-by-side). > @AlanBateman Sure, I appreciate the feedback. Do I understand it correctly > that this won't get into JDK 23 then? If so, perhaps the better way would be > to draft a JEP for JDK 24 and get that proposed early. What I'd like to avoid > is for this change to don't see much movement for a long time between now and > RDP 1 of JDK 24 and have a similar crunch when JDK 24 is close to forking. > You mention clean-up a lot. Is that suggesting it _can_ go into JDK 23 and > clean-up in ramp-down? I'm confused... > > Some clarity on how to best approach getting this integrated that would be > acceptable for all involved would be appreciated. Thanks! The fork for JDK 23 is today so if I was running with this feature then I wouldn't integrate it today. If you are willing to put the time into writing a JEP then I think it's the right thing to do. We should probably have brought this up long before now. I'm happy to help review iterations. There is a lot to write down and this will be very valuable for future phases of this work. I assume future phases. We agreed some restrictions to reduce complexity for this first phase. Future phases may remove these and maybe there is confidence in the future to make it default. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2152576950
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v32]
On Thu, 6 Jun 2024 09:47:30 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Fix default description of keep-packaged-modules I've read through all src changes. I think Sundar is looking at the code changes too. The overall design seems solid. I know it took a long time to get there but this is nature of a feature like this. At this point I regret that there isn't a JEP. A JEP would have captured the motivation, outlined the design, the reasoning for the restrictions, and document the design choices/directions that have been prototyped. If there isn't a JEP then I suppose it can come later if the feature is progressed and there is discussion about making it the default rather than opt-in at build configure time. As cleanup, I think we will need to bring some consistency to the terminology and phrasing in documentation, code and comments. Right now there is "run-time linking", "linkable run-time", "run-time linkable JDK image", "linkable jimage". Also as cleanup, I think the code needs more comments. There is no JEP right now with an authoritive description of the feature so anyone maintaining this code will have to figure out a lot of details. It feels like there should be somehting that documents the effect of --enable-runtime-link-image, how the diffs are generated and saved, and how they are used by jlink. There is also a lot of new code in ImageFileCreator and JlinkTask that is asking for some method descriptions so that anyone touching this code can quickly understand what these methods are doing. I don't want to get into code style issues now but I think it would be helpful for future maintainers to avoid more of the overfly long lines if possible (some of them are 150, 160, 170+ and really hard to look at code side-by-side). - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151964298
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote:
>> Please review this patch which adds a jlink mode to the JDK which doesn't
>> need the packaged modules being present. A.k.a run-time image based jlink.
>> Fundamentally this patch adds an option to use `jlink` even though your JDK
>> install might not come with the packaged modules (directory `jmods`). This
>> is particularly useful to further reduce the size of a jlinked runtime.
>> After the removal of the concept of a JRE, a common distribution mechanism
>> is still the full JDK with all modules and packaged modules. However,
>> packaged modules can incur an additional size tax. For example in a
>> container scenario it could be useful to have a base JDK container including
>> all modules, but without also delivering the packaged modules. This comes at
>> a size advantage of `~25%`. Such a base JDK container could then be used to
>> `jlink` application specific runtimes, further reducing the size of the
>> application runtime image (App + JDK runtime; as a single image *or*
>> separate bundles, depending on the app
being modularized).
>>
>> The basic design of this approach is to add a jlink plugin for tracking
>> non-class and non-resource files of a JDK install. I.e. files which aren't
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive`
>> class which has all the info of what constitutes the final jlinked runtime.
>>
>> Basic usage example:
>>
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod
>> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod
>> jdk.dynalink.jmod jdk.internal.vm.ci.jmod
>> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod
>> jdk.editpad.jmod jdk.internal.vm.compiler.jmod
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod java.xml.jmod
>> jdk.hotspot.agent.jmod jdk.i...
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
line 39:
> 37:
> 38: /**
> 39: * Class representing a difference between a jimage resource. For all
> intents
A difference between a jimage resource and ??? Someone might wonder about that.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourceDiff.java
line 215:
> 213: }
> 214: } catch (IOException e) {
> 215: e.printStackTrace();
Is this IOException swallowed by jlink when not running with the debugging
option? I'm wondering about the printStackTrace here.
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
line 33:
> 31: import jdk.tools.jlink.plugin.ResourcePool;
> 32:
> 33: @SuppressWarnings("try")
Is this needed?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/ResourcePoolReader.java
line 49:
> 47: @Override
> 48: public List getEntries() {
> 49: return pool.entries().map(a ->
> a.path()).collect(Collectors.toList());
I think you can use toList() here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627830148
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627833211
PR Review Comment:
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 119: > 117: > 118: err.runtime.link.not.linkable.runtime=The current run-time image does > not support run-time linking. > 119: err.runtime.link.jdk.jlink.prohibited=Including jdk.jlink module for > run-time image based links is not allowed. The phrase "run-time image based links" in this error message (and in the value of err.runtime.link.packaged.mods) is a bit unusual. As developers will see this message then maybe it could say that this jlink in the current run-time image doesn't contain packaged modules and cannot be used to create another run-time image that include the jdk.jlink module. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627820124
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote:
>> Please review this patch which adds a jlink mode to the JDK which doesn't
>> need the packaged modules being present. A.k.a run-time image based jlink.
>> Fundamentally this patch adds an option to use `jlink` even though your JDK
>> install might not come with the packaged modules (directory `jmods`). This
>> is particularly useful to further reduce the size of a jlinked runtime.
>> After the removal of the concept of a JRE, a common distribution mechanism
>> is still the full JDK with all modules and packaged modules. However,
>> packaged modules can incur an additional size tax. For example in a
>> container scenario it could be useful to have a base JDK container including
>> all modules, but without also delivering the packaged modules. This comes at
>> a size advantage of `~25%`. Such a base JDK container could then be used to
>> `jlink` application specific runtimes, further reducing the size of the
>> application runtime image (App + JDK runtime; as a single image *or*
>> separate bundles, depending on the app
being modularized).
>>
>> The basic design of this approach is to add a jlink plugin for tracking
>> non-class and non-resource files of a JDK install. I.e. files which aren't
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive`
>> class which has all the info of what constitutes the final jlinked runtime.
>>
>> Basic usage example:
>>
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod
>> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod
>> jdk.dynalink.jmod jdk.internal.vm.ci.jmod
>> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod
>> jdk.editpad.jmod jdk.internal.vm.compiler.jmod
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod java.xml.jmod
>> jdk.hotspot.agent.jmod jdk.i...
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JRTArchive.java line 131:
> 129: public boolean equals(Object obj) {
> 130: if (obj instanceof JRTArchive) {
> 131: JRTArchive other = (JRTArchive)obj;
Cleanup for later, you can use pattern matching here and avoid the cast.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627798949
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d make/Images.gmk line 101: > 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) > 100: JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime > 101: endif In passing, I suppose this could be JLINK_PRODUCE_LINKABLE_RUNTIME to be consistent with the configure option. No big deal of course, just noticed a few places where the words are transposed. make/autoconf/jdk-options.m4 line 594: > 592: DEFAULT_PACKAGED_MODULES=false > 593: else > 594: DEFAULT_PACKAGED_MODULES=true Good! - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627791600 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627791805
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote:
>> Please review this patch which adds a jlink mode to the JDK which doesn't
>> need the packaged modules being present. A.k.a run-time image based jlink.
>> Fundamentally this patch adds an option to use `jlink` even though your JDK
>> install might not come with the packaged modules (directory `jmods`). This
>> is particularly useful to further reduce the size of a jlinked runtime.
>> After the removal of the concept of a JRE, a common distribution mechanism
>> is still the full JDK with all modules and packaged modules. However,
>> packaged modules can incur an additional size tax. For example in a
>> container scenario it could be useful to have a base JDK container including
>> all modules, but without also delivering the packaged modules. This comes at
>> a size advantage of `~25%`. Such a base JDK container could then be used to
>> `jlink` application specific runtimes, further reducing the size of the
>> application runtime image (App + JDK runtime; as a single image *or*
>> separate bundles, depending on the app
being modularized).
>>
>> The basic design of this approach is to add a jlink plugin for tracking
>> non-class and non-resource files of a JDK install. I.e. files which aren't
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive`
>> class which has all the info of what constitutes the final jlinked runtime.
>>
>> Basic usage example:
>>
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink)
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod
>> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod
>> jdk.dynalink.jmod jdk.internal.vm.ci.jmod
>> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod
>> jdk.editpad.jmod jdk.internal.vm.compiler.jmod
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod java.xml.jmod
>> jdk.hotspot.agent.jmod jdk.i...
>
> Severin Gehwolf has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 113 commits:
>
> - Mark some tests with requiring packaged modules
> - Correctly set packaged modules default
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Fix new line in jlink.properties
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - Simplify JLINK_JDK_EXTRA_OPTS var handling
> - Only add runtime track files for linkable runtimes
> - Generate the runtime image link diff at jlink time
>
>But only do that once the plugin-pipeline has run. The generation is
>enabled with the hidden option '--generate-linkable-runtime' and
>the resource pools available at jlink time are being used to generate
>the diff.
> - Merge branch 'master' into jdk-8311302-jmodless-link
> - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d
test/jdk/tools/lib/tests/JImageHelper.java line 38:
> 36: * JDK Modular image iterator
> 37: */
> 38: public class JImageHelper {
This is only usable in tests that use `@modules` to open jdk.internal.jimage.*.
It might be better co-locate this with the jlink tests for now. It may be that
we do have test infra structure for jimage in the future but starting out with
the supporting test lib in the jlink test tree should be okay.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1627780757
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Tue, 4 Jun 2024 14:39:23 GMT, Severin Gehwolf wrote: > I've added a couple of `@requires jlink.packagedModules` (new with this > patch) so that jlink tests don't start to fail with it. Good, the `@requires jlink.packagedModules` make sense. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-214941
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v30]
On Tue, 4 Jun 2024 16:23:19 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 113 commits: > > - Mark some tests with requiring packaged modules > - Correctly set packaged modules default > - Merge branch 'master' into jdk-8311302-jmodless-link > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - ... and 103 more: https://git.openjdk.org/jdk/compare/64bbae75...0eb1e48d > [5fef297](https://github.com/openjdk/jdk/commit/5fef297ba63d60452f098193d2cba33a941b) > implements this. I think it was surprising that --enable-runtime-link-image still included the packaged modules so thanks for taking the feedback on this point. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2149894976
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Mon, 3 Jun 2024 15:40:10 GMT, Severin Gehwolf wrote: > Does that proposal sound good? That table is useful, I think it's right. No change to default behavior. If someone configures with --enable-runtime-image then they get a JDK run-time image that supports jlink (with some limitations) but is significantly small as it doesn't include the packaged modules. I've read through most of the changes now. Overall I think it's looking good, just a few terminology and minor points that I'll add as comments. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2145932080
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Sun, 2 Jun 2024 17:41:55 GMT, Alan Bateman wrote: >> Severin Gehwolf has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 110 commits: >> >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Fix new line in jlink.properties >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Simplify JLINK_JDK_EXTRA_OPTS var handling >> - Only add runtime track files for linkable runtimes >> - Generate the runtime image link diff at jlink time >> >>But only do that once the plugin-pipeline has run. The generation is >>enabled with the hidden option '--generate-linkable-runtime' and >>the resource pools available at jlink time are being used to generate >>the diff. >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Use shorter name for the build tool >> - Review feedback from Erik J. >> - Remove dependency on CDS which isn't needed anymore >> - ... and 100 more: https://git.openjdk.org/jdk/compare/4f1a10f8...e1e3f02f > > I've been looking through the changes. One thing that I'm wondering about is > whether --generate-runtime-link-image should disable the keeping of packaged > modules (set JLINK_KEEP_PACKAGED_MODULES to false). It seems surprising to > use this configure option and have the jlink command in the build also copy > the packaged modules into the image. > > (Doing that would of course mean that several existing jlink tests would need > some changes, either to `@requires` or to remove the checks for the `jmods` > directory) > @AlanBateman IMO those are orthogonal concepts. The purpose of --enable-runtime-link-image is to produce a run-time image that is capable of running jlink without the packaged modules. The benefit is is a reducing size on the file system but you don't get that benefit if the packages modules are copied in the image. So I think we may have the wrong default. Yes, they are separate configure and jlink options but I'm wondering if it would be more sensible to opt-in(not opt-out) to keep the packaged modules when configured with --enable-runtime-link-image. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2145136017
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Wed, 22 May 2024 13:23:25 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 110 commits: > > - Merge branch 'master' into jdk-8311302-jmodless-link > - Fix new line in jlink.properties > - Merge branch 'master' into jdk-8311302-jmodless-link > - Simplify JLINK_JDK_EXTRA_OPTS var handling > - Only add runtime track files for linkable runtimes > - Generate the runtime image link diff at jlink time > >But only do that once the plugin-pipeline has run. The generation is >enabled with the hidden option '--generate-linkable-runtime' and >the resource pools available at jlink time are being used to generate >the diff. > - Merge branch 'master' into jdk-8311302-jmodless-link > - Use shorter name for the build tool > - Review feedback from Erik J. > - Remove dependency on CDS which isn't needed anymore > - ... and 100 more: https://git.openjdk.org/jdk/compare/4f1a10f8...e1e3f02f I've been looking through the changes. One thing that I'm wondering about is whether --generate-runtime-link-image should disable the keeping of packaged modules (set JLINK_KEEP_PACKAGED_MODULES to false). It seems surprising to use this configure option and have the jlink command in the build also copy the packaged modules into the image. (Doing that would of course mean that several existing jlink tests would need some changes, either to `@requires` or to remove the checks for the `jmods` directory) - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2143963280
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v13]
On Fri, 24 May 2024 05:26:40 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add a template instead of a property file; remove implNote; update test and > make script accordingly. Marked as reviewed by alanb (Reviewer). Magnus's suggestion for the suffix to be .properties.template makes sense, consistent with the one .template that the JDK currently includes in the run-time image. Overall this looks okay, I'm happy that the implNote update is removed from the proposal as it read too much like the introducing a new supported interface and would have been confusing to have two configurations in the conf directory. - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2076667358 PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2129320078
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Thu, 23 May 2024 16:42:39 GMT, Severin Gehwolf wrote: > Do you think you'll be able to review this next week? Yes, I want to help you get this one over the line. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2127828050
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Wed, 22 May 2024 21:42:14 GMT, Kevin Rushforth wrote: > Further, I confirm that if I pass that option to jlink or jpackage when > creating a custom runtime, there is no warning. Great! What about jpackage without a custom runtime, wondering if --java-options can be tested. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2126320311
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Mon, 20 May 2024 18:47:35 GMT, Phil Race wrote: > Have you looked into / thought about how this will work for jpackaged apps ? > I suspect that both the existing FFM usage and this will be options the > application packager will need to supply when building the jpackaged app - > the end user cannot pass in command line VM options. Seems there should be > some testing of this as some kind of native access could be a common case for > jpackaged apps. I don't see any tests in test/jdk/tools/jpackage that creates an application that uses JNI code. Seems like a good idea to add this via another PR and it specify --java-options so that the application launcher enables native access. It could test jpackage using jlink too. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2121927727
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Mon, 20 May 2024 18:39:31 GMT, Phil Race wrote: >> make/conf/module-loader-map.conf line 105: >> >>> 103: java.smartcardio \ >>> 104: jdk.accessibility \ >>> 105: jdk.attach \ >> >> The list of allowed modules has been rewritten from scratch, by looking at >> the set of modules containing at least one `native` method declaration. > > Should I understand this list to be the set of modules exempt from needing to > specific that native access is allowed ? > ie they always have native access without any warnings, and further that any > attempt to enable warnings, or to disable native access for these modules is > ignored ? Yes, this was added via JDK-8327218. The changes in this PR are just trimming down the list to only the modules that have native code. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607147983
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v10]
On Mon, 20 May 2024 12:48:01 GMT, Sean Mullan wrote: >> Joe Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> withdraw changes to jaxp.properties. The configuration process has not >> changed, changing the default configuration would result in many failures >> that test the process. > > src/java.xml/share/classes/module-info.java line 444: > >> 442: * >> 443: * Deploying with this configuration prevents processors from >> unknowingly making >> 444: * outbound network connections to fetch DTDs, or process XML that >> makes use of > > s/process/processing/ In XML parlance, a "processor" is an aggregation of parsers, serializers, and other things that contribute to the processing. So I think it could be either here, but you have a point and if it stays as "processor" then it should link #FacPro where the term is defined. - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1606745477
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v10]
On Sun, 19 May 2024 05:01:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > withdraw changes to jaxp.properties. The configuration process has not > changed, changing the default configuration would result in many failures > that test the process. Looks good, just one comment on on the jaxp-strict.properties file text. src/java.xml/share/conf/jaxp-strict.properties line 17: > 15: # java -Djava.xml.config.file=$JAVA_HOME/conf/jaxp-strict.properties > 16: # > 17: # The pathname to the configuration file must be valid. If it is not > absolute, I think it would be better to drop this paragraph or else just replace it with a sentence to say that the java.xml module description specifies the system property. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2065520089 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1606350445
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
This looks good. Just a few minor comments where future maintainers might
appreciate comments that describe parameters.
src/java.base/share/classes/java/lang/Module.java line 332:
> 330: String caller = currentClass != null ?
> currentClass.getName() : "code";
> 331: if (jni) {
> 332: System.err.printf("""
System.err may change in a running VM. It may be that we will need to change
this at some point to use its initial setting. Not suggesting we changing it
now but we might have to re-visit this.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2062832385
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604653749
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/ClassLoader.java line 2448:
> 2446: * Invoked in the VM class linking code.
> 2447: */
> 2448: static long findNative(ClassLoader loader, Class clazz, String
> entryName, String javaName) {
I think this is another place where `@param` descriptions would help as it's
not immediately clear that "javaName" is a method name.
src/java.base/share/classes/java/lang/Runtime.java line 39:
> 37:
> 38: import jdk.internal.access.SharedSecrets;
> 39: import jdk.internal.javac.Restricted;
Runtime has been touched for a while so you'll need to bump the copyright year.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604648529
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604650293
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add note on --illegal-native-access default value in the launcher help src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 288: > 286: * throw exception depending on the configuration. > 287: */ > 288: void ensureNativeAccess(Module m, Class owner, String methodName, > Class currentClass, boolean jni); It might be helpful to future maintainers if we put `@param` descriptions for these parameters. I had to re-read Module.enableNativeAccess to remember the difference between the owner class and the current class. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604644767
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/foreign/package-info.java line 170:
> 168: * the special value {@code ALL-UNNAMED} can be used). Access to
> restricted methods
> 169: * from modules not listed by that option is deemed illegal.
> Clients can
> 170: * control how illegal access to restricted method is handled, using the
> command line
I assume this should be "to a restricted method is handled" or "to restricted
methods are handled", either would work here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604637950
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]
On Thu, 16 May 2024 22:20:39 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > remove jaxp-compat.properties from the list src/java.xml/share/classes/module-info.java line 443: > 441: * > 442: * > 443: * This file allows deployments to test the more secure/strict behavior, I think it might be better to reduce this paragraph down to just say something like "Deploying with this configuation prevents processors from unknowingly making outbound network connections to fetch DTDs, or process XML that makes use of extension functions." We could say that a future JDK release may use a strict configuration by default but that opens the door to questions as to whether the system property is needed, whether jaxp.propeteries is going away, so maybe better to leave that out for now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604418621
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]
On Thu, 16 May 2024 22:20:39 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > remove jaxp-compat.properties from the list src/java.xml/share/conf/jaxp-strict.properties line 9: > 7: # test the more secure/strict behavior, identify issues such as a processor > 8: # unknowingly makes outbound network connections to fetch DTD, or > processes XML > 9: # that relies on extension functions. There isn't a JEP to propose that XML processing be secure by default on the technical roadmap right now so I think this paragraph will need to be tweaked to avoid making any assumptions. I think just say that the file provides the settings for more secure XML processing and drop the text about testing (and "and create your own configuration file for the experiment" from the paragraph below). - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604405287
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]
On Thu, 16 May 2024 22:20:39 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > remove jaxp-compat.properties from the list make/modules/java.xml/Copy.gmk line 37: > 35: JAXPPROPFILE_TARGET_FILES := $(subst > $(JAXPPROPFILE_SRC_DIR),$(CONF_DST_DIR),$(JAXPPROPFILE_SRCS)) > 36: > 37: $(CONF_DST_DIR)/%: $(JAXPPROPFILE_SRC_DIR)/% The make file changes to copy the properties files look okay but I'm curious about why the naming changes from "XML" to "JAXPPROFILE". - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604383246
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 12:23:44 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add note on --illegal-native-access default value in the launcher help
src/java.base/share/classes/java/lang/System.java line 2023:
> 2021: * @throws NullPointerException if {@code filename} is {@code
> null}
> 2022: * @throws IllegalCallerException If the caller is in a module
> that
> 2023: * does not have native access enabled.
The exception description is fine, just noticed the other exception
descriptions start with a lowercase "if", this one is different.
src/java.base/share/man/java.1 line 587:
> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
> 587: command-line option.
"This mode disable all illegal native access except for those modules enabled
the --enable-native-access command-line option".
This can be read to mean that modules granted native access with the command
line option is also illegal native access An alternative is to make the second
part of the sentence a new sentence, something like "Only modules enabled by
the --enable-native-access command line option may perform native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603878829
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603875920
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman wrote: >>> > @mlchung @AlanBateman Any thoughts on this latest version? Is this going >>> > into the direction you had envisioned? Any more blockers? The CSR should >>> > be up-to-date and is open for review as well. If no more blockers I'll go >>> > over this patch once more and clean it up to prepare for integration. >>> > Thanks! >>> >>> Thanks for all the efforts on this. >> >> Thanks for looking at this, Alan! >> >>> I've looked through the latest version. I'm a little bit comfortable with >>> LinkDeltaProducer. One reason it's build tool that is tied tied to code in >>> the jdk.jlink module, and secondly is that it copies >>> runtime-image-link.delta into the run-time image. Our long standing >>> position is that the run-time image is created by jlink rather than a >>> combination of jlink and extra stuff copied in by the build. >>> >>> The part that I like with the current proposal is >>> lib/runtime-image-link.delta as it's less complicated that previous >>> iterations that added a resource into the jimage container. >>> >>> What would you (and @mlchung) think of having jlink generate >>> lib/runtime-image-link.delta as a step between the pipeline and image >>> generation? >> >> If I understand you correctly, this would be no longer a build-time only >> approach to produce the "linkable runtime"? It would be some-kind of >> jlink-option driven approach (as it would run some code that should only run >> when producing a linkable runtime is requested)? Other than that, it should >> be fine as the previous iteration basically did that but at a different >> phase. Also note that the previous iteration used a build-only jlink plugin >> so as to satisfy the build-time only approach, yet it ran as part of the >> plugin-pipeline which wasn't desired either. But it was something similar to >> what you seem to be describing now. Did I get something wrong? > >> If I understand you correctly, this would be no longer a build-time only >> approach to produce the "linkable runtime"? It would be some-kind of >> jlink-option driven approach (as it would run some code that should only run >> when producing a linkable runtime is requested)? Other than that, it should >> be fine as the previous iteration basically did that but at a different >> phase. Also note that the previous iteration used a build-only jlink plugin >> so as to satisfy the build-time only approach, yet it ran as part of the >> plugin-pipeline which wasn't desired either. But it was something similar to >> what you seem to be describing now. Did I get something wrong? > > I think it continues to build time, it's just using some hidden jlink option. > So yes, it similar to a previous iteration except that it doesn't run as a > plugin the pipeline and the delta goes to the lib directory. > > Let's see what @mlchung says. You've put a lot of work into this so let's see > if we can converge to avoid too many more rounds. > @AlanBateman @mlchung The latest update now uses the `jlink` build time > option `--generate-linkable-runtime` to add needed resources to the `jimage` > when a runtime linkable JDK image is being asked for using the configure > option. This now runs outside the plugin-pipeline. I think this is what you > meant. Sorry it took longer to get back to this... I think you've got this to a good place and I think the overall solution is good. It may be that JDK should move to this by default in the future, and at the same time re-visit the restriction on generating an image containing jdk.jlink, but let's see if any issues come up. I've added myself as Reviewer to the CSR and I'm working through the code changes. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2115298661
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v5]
On Wed, 15 May 2024 10:40:34 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Refine warning text for JNI method binding
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 871:
> 869: return IllegalNativeAccess.WARN;
> 870: } else {
> 871: fail("Value specified to --illegal-access not recognized:"
Typo in the message, should be --illegal-native-access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601898238
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 10:34:01 GMT, Maurizio Cimadamore wrote: > I don't fully agree that this option is not module related (which is why I > gave it that name). The very definition of illegal native access is related > to native access occurring from a module that is outside a specific set. So I > think it's 50/50 as to whether this option is module-related or not. Of > course I can fix the code if there's something clearly better. It maps here to a jdk.module.* property so I suppose it's okay. The functions were introduced to create jdk.module.* properties where the values were a set module names or a module path. Maybe these functions should be renamed at some point (not here) as they are more widely useful now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601421535
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 00:54:43 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/arguments.cpp line 2271:
>>
>>> 2269: } else if (match_option(option, "--illegal-native-access=",
>>> )) {
>>> 2270: if (!create_module_property("jdk.module.illegal.native.access",
>>> tail, InternalProperty)) {
>>> 2271: return JNI_ENOMEM;
>>
>> I think it would be helpful to get guidance on if this is the right way to
>> add this system property, only because this one not a "module property". The
>> configuration (WriteableProperty + InternalProperty) look right.
>
> So my recollection/understanding is that we use this mechanism to convert
> module-related `--` flags passed to the VM into system properties that the
> Java code can then read, but we set them up such that you are not allowed to
> specify them directly via `-D`. Is the question here whether this new
> property should be in the `jdk.module` namespace?
That's my recollection too. The usage here isn' related to modules which makes
me wonder if this function should be renamed (not by this PR of course) of if
we should be using PropertyList_unique_add (with AddProperty,
WriteableProperty, InternalProperty) instead. There will be further GNU style
options coming that will likely need to map to an internal system property in
the same way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601002132
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Fix another typo
> - Fix typo
> - Add more comments
src/hotspot/share/runtime/arguments.cpp line 2271:
> 2269: } else if (match_option(option, "--illegal-native-access=", ))
> {
> 2270: if (!create_module_property("jdk.module.illegal.native.access",
> tail, InternalProperty)) {
> 2271: return JNI_ENOMEM;
I think it would be helpful to get guidance on if this is the right way to add
this system property, only because this one not a "module property". The
configuration (WriteableProperty + InternalProperty) look right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598673962
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 16 Apr 2024 11:59:12 GMT, Severin Gehwolf wrote: > If I understand you correctly, this would be no longer a build-time only > approach to produce the "linkable runtime"? It would be some-kind of > jlink-option driven approach (as it would run some code that should only run > when producing a linkable runtime is requested)? Other than that, it should > be fine as the previous iteration basically did that but at a different > phase. Also note that the previous iteration used a build-only jlink plugin > so as to satisfy the build-time only approach, yet it ran as part of the > plugin-pipeline which wasn't desired either. But it was something similar to > what you seem to be describing now. Did I get something wrong? I think it continues to build time, it's just using some hidden jlink option. So yes, it similar to a previous iteration except that it doesn't run as a plugin the pipeline and the delta goes to the lib directory. Let's see what @mlchung says. You've put a lot of work into this so let's see if we can converge to avoid too many more rounds. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2059157584
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Wed, 3 Apr 2024 22:31:39 GMT, Mandy Chung wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move CreateLinkableRuntimePlugin to build folder >> >> Keep runtime link supporting classes in package >> jdk.tools.jlink.internal.runtimelink > > Thanks for the investigation w.r.t. extending jimage. It does not seem worth > the effort in pursuing the support of adding resources to an existing jimage > file. To me, putting the diff data under `lib` directory as a private file > is a simpler and acceptable solution. It allows the second jlink invocation > to avoid the plugin pipeline if the per-module metadata is generated in > normal jlink invocation. > > (An observation: The current implementation requires the diffs to be > generated as a plugin. For this approach, it may be possible to implement > with a separate main class that calls JLink API and generates the diffs.) > @mlchung @AlanBateman Any thoughts on this latest version? Is this going into > the direction you had envisioned? Any more blockers? The CSR should be > up-to-date and is open for review as well. If no more blockers I'll go over > this patch once more and clean it up to prepare for integration. Thanks! Thanks for all the efforts on this. I've looked through the latest version. I'm a little bit comfortable with LinkDeltaProducer. One reason it's build tool that is tied tied to code in the jdk.jlink module, and secondly is that it copies runtime-image-link.delta into the run-time image. Our long standing position is that the run-time image is created by jlink rather than a combination of jlink and extra stuff copied in by the build. The part that I like with the current proposal is lib/runtime-image-link.delta as it's less complicated that previous iterations that added a resource into the jimage container. What would you (and @mlchung) think of having jlink generate lib/runtime-image-link.delta as a step between the pipeline and image generation? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2058876568
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic
On Tue, 2 Apr 2024 15:42:05 GMT, Volodymyr Paprotski wrote: > Performance. Before: > > Benchmark(algorithm) (dataSize) (keyLength) > (provider) Mode Cnt ScoreError Units > SignatureBench.ECDSA.signSHA256withECDSA1024 256 > thrpt3 6443.934 ± 6.491 ops/s > SignatureBench.ECDSA.signSHA256withECDSA 16384 256 > thrpt3 6152.979 ± 4.954 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA1024 256 > thrpt3 1895.410 ± 36.979 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 > thrpt3 1878.955 ± 45.487 ops/s > Benchmark(algorithm) (keyLength) > (kpgAlgorithm) (provider) Mode Cnt ScoreError Units > o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1357.810 ± 26.584 ops/s > o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1352.119 ± 23.547 ops/s > Benchmark (isMontBench) Mode Cnt Score > Error Units > PolynomialP256Bench.benchMultiply false thrpt3 1746.126 ± > 10.970 ops/s > > Performance, no intrinsic: > > Benchmark(algorithm) (dataSize) (keyLength) > (provider) Mode Cnt Score Error Units > SignatureBench.ECDSA.signSHA256withECDSA1024 256 > thrpt3 6529.839 ± 42.420 ops/s > SignatureBench.ECDSA.signSHA256withECDSA 16384 256 > thrpt3 6199.747 ± 133.566 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA1024 256 > thrpt3 1973.676 ± 54.071 ops/s > SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 > thrpt3 1932.127 ± 35.920 ops/s > Benchmark(algorithm) (keyLength) > (kpgAlgorithm) (provider) Mode Cnt ScoreError Units > o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1355.788 ± 29.858 ops/s > o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH 256 > EC thrpt3 1346.523 ± 28.722 ops/s > Benchmark (isMontBench) Mode Cnt Score > Error Units > PolynomialP256Bench.benchMultiply true thrpt3 1919.574 ± > 10.591 ops/s > > Performance, **with intrinsics*... src/java.base/share/classes/module-info.java line 265: > 263: jdk.jfr, > 264: jdk.unsupported, > 265: jdk.crypto.ec; jdk.crypto.ec has been hollowed out since JDK 22, the sun.security.ec are in java.base. So I don't think you need this qualified export. - PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1548199507
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Wed, 6 Mar 2024 13:43:00 GMT, Magnus Ihse Bursie wrote: >> Currently, our symbol visibility handling for tests are sloppy; we only >> handle it properly on Windows. We need to bring it up to the same levels as >> product code. This is a prerequisite for >> [JDK-8327045](https://bugs.openjdk.org/browse/JDK-8327045), which in turn is >> a building block for Hermetic Java. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Update line number for dereference_null in TestDwarf Good cleanup. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18135#pullrequestreview-1927041103
Re: RFR: 8327460: Compile tests with the same visibility rules as product code
On Wed, 6 Mar 2024 12:14:10 GMT, Magnus Ihse Bursie wrote: > * `test/jdk/java/lang/Thread/jni/AttachCurrentThread/libImplicitAttach.c` was > missing an export. This had not been discovered before since that file was > not compiled on Windows. It's a Linux/macOS specific test so it wasn't needed. - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1987743188
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v12]
On Fri, 8 Mar 2024 16:47:33 GMT, Magnus Ihse Bursie wrote: > What is the rationale for introducing a special configure flag for this > functionality? I've tried to look though all comments in this PR, and read > the JBS issue and CSR, but I have not found any motivation for this. I'm > sorry if it's there and I missed it, but this is a huge PR with a lot of > discussion. > > In general, it is better not to introduce variants of the build like this. > The testing matrix just explodes a bit more. And my understanding from the > discussion is that this functionality is likely to be turned on anyway, > otherwise you'll build a crippled jlink without full functionality. The JDK image (images/jdk) includes the packaged modules (as .jmod files) that the jlink tool can use to create other run-time images. The proposal here isn't meant to change this. "make images" should create the JDK image as before and that image will include the packaged modules. The inclusion of the packaged modules has been problematic in some environments, esp. when there are debug symbols. libjvm.so can be huge, which begs the question as to why there is a copy in java.base.jmod. There are of course options to build the JDK image without --keep-packaged-modules and host the packaged modules as a separate download. Back in the JDK 9 we decided not to do this for Oracle downloads. Severin has implemented an approach that allows "observable modules" be found in the current run-time image when using jlink. This requires some heretics and computation of diffs between the bits in the original packaged modules and the transformed bits in the run-time image. This exploration has gone through many iterations. Recently, Severin, Mandy and I have met to try to simplify things and tame the goals in order to get to something that is maintainable, and to allow time to get experience with this direction. So at a high-level, the intention is that the build be capable of producing an alternative JDK image that doesn't have a "jmods" directory. The jlink tool in that image has a limitation, a compromise to keep the complexity at a manageable level. I have no opinion on whether the opt-in is at configure time or its just make target (like "make legacy-jre-image"). In the discussions it wasn't meant to be built by default. If distributions choose to distribute this image then this will likely influence what they test. Once experience builds up with using these run-time images then it may be that there is a proposal (and JEP) to make it the default, meaning images/jdk would not include .jmod files and multi-hop restriction is removed from jlink. That may be something for much further down the road. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1986796401
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v4]
On Thu, 7 Mar 2024 21:53:07 GMT, Jan Lahoda wrote: >> Currently, JDK modules load by the bootstrap and platform ClassLoaders are >> automatically granted the native access. I am working on an upgrade of JLine >> inside the `jdk.internal.le` module, and I would like to replace the current >> native bindings with FFM-based bindings (which are now somewhat provided by >> JLine). But, for that, native access is needed for the `jdk.internal.le` >> module. We could possibly move the module to the platform ClassLoader, but >> it seems it might be better to have more control over which modules have the >> native access. >> >> This patch introduces an explicit list of modules that will automatically be >> granted the native access. Note this patch is not yet intended to change the >> end behavior - the list of modules granted native access is supposed to be >> the same as modules in the boot and platform ClassLoaders. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Adjusting javadoc as suggested. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1924302699
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v3]
On Wed, 6 Mar 2024 21:16:25 GMT, Jan Lahoda wrote:
>> Currently, JDK modules load by the bootstrap and platform ClassLoaders are
>> automatically granted the native access. I am working on an upgrade of JLine
>> inside the `jdk.internal.le` module, and I would like to replace the current
>> native bindings with FFM-based bindings (which are now somewhat provided by
>> JLine). But, for that, native access is needed for the `jdk.internal.le`
>> module. We could possibly move the module to the platform ClassLoader, but
>> it seems it might be better to have more control over which modules have the
>> native access.
>>
>> This patch introduces an explicit list of modules that will automatically be
>> granted the native access. Note this patch is not yet intended to change the
>> end behavior - the list of modules granted native access is supposed to be
>> the same as modules in the boot and platform ClassLoaders.
>
> Jan Lahoda has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/native-access-modules1' into
> native-access-modules1
> - Reflecting review feedback.
> - Updating copyright headers.
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/lang/ModuleLayer.java line 891:
> 889: * {@code false} otherwise
> 890: */
> 891: boolean addEnableNativeAccess(String name) {
Do you mind changing the method description to "Updates the module with the
given name in this layer to allow access to restricted methods"? This will be
keep it more consistent with the exiting methods.
Also "was present" in the return description hints that it may not now be
present. A module layer is immutable so it can just say that it returns true if
the module is in this layer.
-
PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1921919290
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515834537
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 21:22:40 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line
>> 812:
>>
>>> 810: }
>>> 811:
>>> 812: private static void addEnableNativeAccess(ModuleLayer layer,
>>> Set moduleNames, boolean shouldWarn) {
>>
>> The private methods in this class have a short comment to summarise what
>> they do.
>
> I've tried to add a comment here:
> https://github.com/openjdk/jdk/pull/18106/commits/e17cd3722724cbc6aa298f7b789c6574554af6ea
Thanks. I view these changes to ModuleBootstrap to be temporary. We'll need to
create a few follow on issues in JBS, one of which is to update jlink so that
we have the exact set of modules in the run-time image that have been granted
native access.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1515838474
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Wed, 6 Mar 2024 18:00:25 GMT, Mandy Chung wrote: > Native access is needed for modules which calls restricted methods [1]. In time, we'll need it for modules using JNI too. We can do this trimming in another PR to avoid Jan getting pulled into deeply. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1514999868
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 22:44:20 GMT, Mandy Chung wrote: > Many of these modules do not need native access in the current implementation. In addition this will eventually need jlink support. I view the change to ModuleBootstrap initialiser to use ModuleLoaderMap.nativeAccessModules() as very temporary. It may include many standard/JDK modules that aren't in the image. In addition we'll need some way to grant native access at link-time. The workaround for the latter right now is to configure default options. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1514113280
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 GMT, Jan Lahoda wrote:
>> Currently, JDK modules load by the bootstrap and platform ClassLoaders are
>> automatically granted the native access. I am working on an upgrade of JLine
>> inside the `jdk.internal.le` module, and I would like to replace the current
>> native bindings with FFM-based bindings (which are now somewhat provided by
>> JLine). But, for that, native access is needed for the `jdk.internal.le`
>> module. We could possibly move the module to the platform ClassLoader, but
>> it seems it might be better to have more control over which modules have the
>> native access.
>>
>> This patch introduces an explicit list of modules that will automatically be
>> granted the native access. Note this patch is not yet intended to change the
>> end behavior - the list of modules granted native access is supposed to be
>> the same as modules in the boot and platform ClassLoaders.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
I assume you'll bump the copyright header on all files before integrating.
make/conf/module-loader-map.conf line 139:
> 137: jdk.unsupported \
> 138: jdk.xml.dom \
> 139: jdk.zipfs \
We should create a JBS issue to prune this.
src/java.base/share/classes/java/lang/ModuleLayer.java line 896:
> 894: return nameToModule.get(name);
> 895: }
> 896:
What would you think about replacing this with addEnableNativeAccess(String
name) so it can be called by JLA. addEnableNativeAccess. The reason is that the
JLA methods are usually just calls to some non-public method but the changes
mean mean there is "core" in the JLA method that is not easy to find.
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 812:
> 810: }
> 811:
> 812: private static void addEnableNativeAccess(ModuleLayer layer,
> Set moduleNames, boolean shouldWarn) {
The private methods in this class have a short comment to summarise what they
do.
-
PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1917862955
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513331594
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513345563
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513347180
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 13:42:32 GMT, Jaikiran Pai wrote: > to make the intention clear as well as reduce the chances of missing some > boot or platform module in this NATIVE_ACCESS_MODULES? The list to be be granted native access is a subset, it shouldn't be granted all modules mapped to the boot or platform class loaders. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512873855
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK
On Fri, 23 Feb 2024 21:24:10 GMT, Naoto Sato wrote: > This PR intends to remove the legacy `COMPAT` locale data from the JDK. The > `COMPAT` locale data was introduced for applications' migratory purposes > transitioning to `CLDR`. It is becoming a technical debt and now is the time > to remove it (we've been emitting a warning at JVM startup since JDK21, if > the app is using `COMPAT`). A corresponding CSR has also been drafted. >From a stewardship perspective I think we've done the right steps. To >summarize: - JDK 8 added the option to use CLDR locale data (JEP 127). - JDK 9 switched to using CLDR locale data by default (JEP 252) with the option to run with -Djava.locale.providers=COMPAT and use the legacy/unmaintained locale data. - JDK 21 added a warning when you run with -Djava.locale.providers=COMPAT announcing that this provider will be removed in a future release. - With the proposal here, running with -Djava.locale.providers=COMPAT will print a warning to say that the configuration is ignored. The reduction of 10Mb will be welcomed. There are likely projects that run their tests with the COMPAT provider. There may be some application deployments too. I've seen a few projects do changes in response to the run-time warning introduced in JDK 21 but there are likely projects/applications that will be "surprised" when they upgrade to JDK 23+ and tests fail. So I think one will need a bit of socialization and a loud release note. - PR Comment: https://git.openjdk.org/jdk/pull/17991#issuecomment-1962299087
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 07:44:18 GMT, Magnus Ihse Bursie wrote:
>> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we
>> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK
>> native libraries.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Once more, remove AIX dirent64 et al defines
I can't comment on AIX but the changes look okay overall. I assume you'll bump
the copyright header date on all the updated files before integrating.
src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 257:
> 255: static int fstatat_wrapper(int dfd, const char *path,
> 256: struct stat *statbuf, int flag)
> 257: {
Minor nit - you can probably fix the align after the edit or collapse it into
one line.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17538#pullrequestreview-1872182776
PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1484203284
Re: RFR: JDK-8325189: Enable this-escape javac warning in java.base
On Fri, 2 Feb 2024 23:36:41 GMT, Joe Darcy wrote: > After the "this-escape" lint warning was added to javac (JDK-8015831), the > base module was not updated to be able to compile with this warning enabled. > This PR makes the necessary changes to allow the base module to build with > the warning enabled. I skimmed through the use sites and don't see any issues. There is one bucket of escaping "this" that will go away once the support for running with the SM goes away. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17692#pullrequestreview-1861282672
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v4]
On Tue, 30 Jan 2024 14:15:57 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge branch 'master' into jdk-FOB64 > - Move #include out of debug_util.h > - Restore AIX dirent64 et al defines > - Rollback AIX changes since they are now tracked in JDK-8324834 > - Remove superfluous setting of FOB64 > - Replace all foo64() with foo() for large-file functions in the JDK > - 8324539: Do not use LFS64 symbols in JDK libs I skimmed through the changes and they look okay. Can you confirm that you've run tier1-4 at least? Some of the library code that is changed here is not tested in the lower tiers. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1921189429
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon wrote: > You're right. I had forgotten the intricacies of class loader delegation. The > only hard constraint on loading a class in multiple loaders is that `java.*` > classes [must (only) be loaded by the boot > loader](https://github.com/openjdk/jdk/blob/bccd823c8e40863bed70ff5b24772843203871a5/src/java.base/share/classes/java/lang/ClassLoader.java#L904). Just to add that this restriction was relaxed in Java 9 to allow java.* classes be defined by the platform class loader. The code that is linked to here throws if the class loader is not the platform class loader. There isn't a user accessible ClassLoader object for the boot loader and testing `this == null` doesn't make sense. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908375220
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote:
>> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform
>> class loader instead of the boot class loader. This allows Native Image to
>> load a version of JVMCI different than the version on top of which Native
>> Image is running. This capability is demonstrated and tested by
>> `LoadAlternativeJVMCI.java`.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use null to denote boot class loader as delegation parent
test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 50:
> 48: e = e + File.separator;
> 49: }
> 50: cp[i] = new URI("file:" + e).toURL();
This should be `cp[I] = file.toURI().toURL()` as a file path needs encoding to
be URI path component.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464719091
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Sat, 20 Jan 2024 07:34:34 GMT, Alan Bateman wrote: >> Sam James has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains seven additional commits >> since the last revision: >> >> - Merge master >> - crank copyright >> - sendfile64 -> sendfile >> >>Signed-off-by: Sam James >> - buf64->buf >> >>Signed-off-by: Sam James >> - Add message for assert >> >>Not all C++ stds implement it w/o. >> - Add off_t static_asserts >> >>Signed-off-by: Sam James >> - Do not use LFS64 symbols on Linux >> >>The LFS64 symbols provided by glibc are not part of any standard and >>were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in >>1.2.5). This commit replaces the usage of LFS64 symbols with their >>regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that >> functions >>will always act as their -64 variants on glibc. >> >>Signed-off-by: Sam James > > I assume a separate issue will be needed for the JDK native libraries as > there are quite a few LFS64 usages. > @AlanBateman @thesamesam I opened > [JDK-8324539](https://bugs.openjdk.org/browse/JDK-8324539) for the JDK libs. > The implementation is trivial (#17538) but I am not sure how to understand > the impact. My gut feeling is that if anything was wrong with this it would > not even compile, but I need to understand this properly. Doesn't it mean going over the native code and replacing the LFS64 symbols with their regular counterparts? - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1906382096
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Fri, 19 Jan 2024 23:50:46 GMT, Sam James wrote: >> The LFS64 symbols provided by glibc are not part of any standard and were >> gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in 1.2.5). >> This commit replaces the usage of LFS64 symbols with their regular >> counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that functions >> will always act as their -64 variants on glibc. > > Sam James has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains seven additional commits since > the last revision: > > - Merge master > - crank copyright > - sendfile64 -> sendfile > >Signed-off-by: Sam James > - buf64->buf > >Signed-off-by: Sam James > - Add message for assert > >Not all C++ stds implement it w/o. > - Add off_t static_asserts > >Signed-off-by: Sam James > - Do not use LFS64 symbols on Linux > >The LFS64 symbols provided by glibc are not part of any standard and >were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in >1.2.5). This commit replaces the usage of LFS64 symbols with their >regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that > functions >will always act as their -64 variants on glibc. > >Signed-off-by: Sam James I assume a separate issue will be needed for the JDK native libraries as there are quite a few LFS64 usages. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1901863054
Re: [jdk22] RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable
On Wed, 20 Dec 2023 21:28:04 GMT, Serguei Spitsyn wrote: > Hi all, > > This pull request contains a backport of commit > [0f8e4e0a](https://github.com/openjdk/jdk/commit/0f8e4e0a81257c678e948c341a241dc0b810494f) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Serguei Spitsyn on 19 Dec 2023 > and was reviewed by Leonid Mesnik and Alan Bateman. > > Thanks! Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk22/pull/23#pullrequestreview-1812613022
Re: RFR: JDK-8321621: Update JCov version to 3.0.16
On Fri, 8 Dec 2023 22:42:42 GMT, Alexandre Iline wrote: > JCov support for byte code version 67. Happy to see this being changed soon after the fork. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17041#pullrequestreview-1773616082
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 GMT, Serguei Spitsyn wrote: >> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 >> time frame. >> It is fixing a deadlock issue between `VirtualThread` class critical >> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, >> `getAndClearInterrupt()`, `threadState()`, `toString()`), >> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. >> The deadlocking scenario is well described by Patricio in a bug report >> comment. >> In simple words, a virtual thread should not be suspended during >> 'interruptLock' critical sections. >> >> The fix is to record that a virtual thread is in a critical section >> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about >> begin/end of critical section. >> This bit is used in `HandshakeState::get_op_for_self()` to filter out any >> `HandshakeOperation` if a target `JavaThread` is in a critical section. >> >> Some of new notifications with `notifyJvmtiSync()` method is on a >> performance critical path. It is why this method has been intrincified. >> >> New test was developed by Patricio: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> The test is very nice as it reliably in 100% reproduces the deadlock without >> the fix. >> The test is never failing with this fix. >> >> Testing: >> - tested with newly added test: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: (1) rename notifyJvmti method; (2) add try-final statements to > VirtualThread methods I chatted briefly with @sspitsyn about this. A couple of points: - It shouldn't be necessary to touch mount/unmount as the thread identity is the carrier, not the virtual thread, when executing the "critical code". - toggle_is_in_critical_section needs to detect reentrancy, it is otherwise too early to refactor the Java code, e.g. call threadState while holding the interrupt lock. - All the use-sides will need to use try-finally to more reliably revert the critical section flag when rewinding. - The naming is very problematic, we'll need to replace with methods that are clearly named enter and exit critical section. Ongoing work in this area to support monitors has to introduce some temporary pinning so there will be enter/exitCriticalSection methods, that's a better place for the JVMTI hooks. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1847063362
Re: RFR: 8211238: @Deprecated JFR event [v8]
On Sun, 3 Dec 2023 16:31:13 GMT, Markus Grönlund wrote: >> Greetings, >> >> please help review this enhancement to add a JFR event to report runtime >> invocations of methods that have been declared deprecated in the JDK. >> >> Testing: jdk_jfr, CI 1-6, stress testing >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > minor adjustment A question about "level". Is the intention that the value can be anything, e.g. some new event next month might use the values "1", "2, "3"? Just asking because ordinarily deprecated vs. terminally deprecated is very specific to the manner in which a program element is deprecated and I assume you don't want this event grabbing the general name for a very specific event setting. - PR Comment: https://git.openjdk.org/jdk/pull/16931#issuecomment-1837532534
Re: RFR: 8211238: @Deprecated JFR event [v2]
On Sat, 2 Dec 2023 17:20:58 GMT, Markus Grönlund wrote:
>> Greetings,
>>
>> please help review this enhancement to add a JFR event to report runtime
>> invocations of methods that have been declared deprecated in the JDK.
>>
>> Testing: jdk_jfr, CI 1-6, stress testing
>>
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Merge branch '8211238' of github.com:mgronlun/jdk into 8211238
> - reflection support
src/jdk.jfr/share/classes/jdk/jfr/internal/test/DeprecatedThing.java line 90:
> 88: public static void reflectionForRemoval() {
> 89: staticCounter++;
> 90: }
You might want to extend the set of tests to include cases that have the
"since" element. There is a 2x2 matrix of cases to fully exercise the parsing
of the RuntimeVisibleAnnotations content.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16931#discussion_r1413041171
Re: RFR: JDK-8319413: Start of release updates for JDK 23 [v4]
On Thu, 30 Nov 2023 23:49:24 GMT, Joe Darcy wrote: >> Time to start making preparations for JDK 23. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update symbol files to JDK 22 b26. Good good. I assume you'll bump the copyright header on the files that need it before integration, e.g. JavacTestingAbstractProcessor. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16505#pullrequestreview-1759306298
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete [v2]
On Fri, 24 Nov 2023 12:04:01 GMT, Daniel Jeliński wrote: > Yes, with `/mA` the command exits as soon as the dump is completed. I > couldn't find a way to make `cdb` not enter interactive mode on unrecognized > parameter, so I left that part as is. Okay, maybe it can be figured out another time. - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404343895
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete [v2]
On Fri, 24 Nov 2023 09:58:17 GMT, Daniel Jeliński wrote: >> The recent cdb versions do not support `.dump /f`: >> >> * >> * .dump /f is not supported on a user mode process. * >> * * >> * .dump /ma creates a complete memory dump of a user mode process. * >> * >> >> and after printing that message, cdb ignores the rest of the command line >> and never quits. >> >> This PR updates the dump command to use the recommended `/ma` parameter. >> This allows the command to produce a dump and complete in a timely manner. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16806#pullrequestreview-1747936722
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete [v2]
On Fri, 24 Nov 2023 11:33:57 GMT, Daniel Jeliński wrote: > It's ignoring the rest of the command line after `/f`, which means it ignores > the `;qd` (quit and detach) part and enters the interactive mode. Wonderful :-)Does switching to `/mA` change that? - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404258410
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete [v2]
On Fri, 24 Nov 2023 09:40:16 GMT, Daniel Jeliński wrote: >>> Notice the absence of "params" part in that key. I wonder if that is >>> playing a role here and whether we should fix this key. >> >> Actually ignore that part. I had a look at the internal logs that you >> referenced. It appears that this form of specifying the timeout (through the >> use of `params` key) seems to work too. The reason why it took 2 hours is >> because it ran that command against 2 separate processes and each one timed >> out after one hour: >> >> >> [2023-11-23 21:45:40] [cdb.exe, -c, ".dump, /f, core.12345;qd", -p, 12345] >> timeout=360 >> ... >> 0:001> WARNING: tool timed out: killed process after 360 ms >> >> [2023-11-23 22:45:40] exit code: -2 time: 366 ms >> >> >> >> >> [2023-11-23 22:47:36] [cdb.exe, -c, ".dump, /f, core.6789;qd", -p, 6789] >> timeout=360 >> >> >> ... >> 0:063> WARNING: tool timed out: killed process after 360 ms >> >> [2023-11-23 23:47:36] exit code: -2 time: 356 ms >> >> >> >> Edit: ... and you did mention about this in the description of the JBS >> issue. I just overlooked it :) > > good point, 10 minutes should be more than enough. I'll update. Out of curiosity, do we know why it takes more than an hour? Is it attempting to connect to the Microsoft symbol server? - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404194681
Re: RFR: 8320691: Timeout handler on Windows takes 2 hours to complete
On Fri, 24 Nov 2023 07:58:18 GMT, Daniel Jeliński wrote: > The recent cdb versions do not support `.dump /f`: > > * > * .dump /f is not supported on a user mode process. * > * * > * .dump /ma creates a complete memory dump of a user mode process. * > * > > and after printing that message, cdb ignores the rest of the command line and > never quits. > > This PR updates the dump command to use the recommended `/ma` parameter. This > allows the command to produce a dump and complete in a timely manner. test/failure_handler/src/share/conf/windows.properties line 60: > 58: > 59: native.core.app=cdb > 60: native.core.args=-c ".dump /ma core.%p;qd" -p %p Just to double check that this is the right option. `/ma` terminates if there is "inaccessable memory" where `/mA` seems to be continue. The `/f` option says all "accessible" memory which I assume it means skips/ignored inaccessible, is that right? - PR Review Comment: https://git.openjdk.org/jdk/pull/16806#discussion_r1404110118
Re: RFR: 8306055: Add a built-in Catalog to JDK XML module [v3]
On Mon, 20 Nov 2023 17:46:53 GMT, Joe Wang wrote: >> Implement the built-in Catalog. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add a note; fix alignment I'm happy with the addition of the JDK built-in catalog, the inclusion of the DTD defined by Java SE, and the docs updates. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16719#pullrequestreview-1743867203
Re: RFR: 8306055: Add a built-in Catalog to JDK XML module
On Fri, 17 Nov 2023 20:22:40 GMT, Joe Wang wrote: > Implement the built-in Catalog. src/java.xml/share/classes/jdk/xml/internal/jdkcatalog/JDKCatalog.xml line 34: > 32: > 33: http://java.sun.com/dtd/preferences.dtd; > uri="J2SE/preferences.dtd"/> > 34: http://java.sun.com/dtd/properties.dtd; > uri="J2SE/properties.dtd"/> Would it be possible to provide a summary on how the relative uri in the "uri" attribute is handled? Asking as it's not clear to me how the decoding is handling, meaning this is a relative URI, not a relative file path. src/java.xml/share/classes/module-info.java line 910: > 908: * Method 1 > 909: * 22 > 910: * I went through a number of iterations with Joe on this implNote so happy with this version. - PR Review Comment: https://git.openjdk.org/jdk/pull/16719#discussion_r1398355355 PR Review Comment: https://git.openjdk.org/jdk/pull/16719#discussion_r1398355680
Re: RFR: 8306055: Add a built-in Catalog to JDK XML module
On Sun, 19 Nov 2023 08:44:01 GMT, Alan Bateman wrote: >> Implement the built-in Catalog. > > src/java.xml/share/classes/jdk/xml/internal/jdkcatalog/JDKCatalog.xml line 34: > >> 32: >> 33: http://java.sun.com/dtd/preferences.dtd; >> uri="J2SE/preferences.dtd"/> >> 34: http://java.sun.com/dtd/properties.dtd; >> uri="J2SE/properties.dtd"/> > > Would it be possible to provide a summary on how the relative uri in the > "uri" attribute is handled? Asking as it's not clear to me how the decoding > is handling, meaning this is a relative URI, not a relative file path. Can we move DTD in the JDK's catalog to a "Java" or "JavaSE" directory, only because "J2SE" feels a bit yesterday. - PR Review Comment: https://git.openjdk.org/jdk/pull/16719#discussion_r1398355546
Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols
On Fri, 10 Nov 2023 16:30:35 GMT, suchismith1993 wrote: > There is not generic way of generating this. It needs a manual intervention > and symbols are to be added on a need basis in future. Looks like a list to > be maintained. I had tried adding comments to explain the list, but that > causes build failures. Would it be possible to paste the summary of the "instructions" to generate this? My initial reaction when seeing this PR is to wonder why it can't be generated at build time but from the discussion, it seems like it's a subset of the symbols but it is really just the functions used by libjvm or is it all the native libraries? - PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1807608795
Re: RFR: 8284890: Support for Do not fragment IP socket options [v8]
On Wed, 20 Apr 2022 14:30:21 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following PR review please? It adds a new JDK specific >> extended socket option >> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 >> and IPv6 >> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF >> (Dont Fragment) bit >> in the IP header. There is no equivalent in the IPv6 packet header as >> fragmentation is implemented >> exclusively by the sending and receiving nodes. >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test update src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java line 221: > 219: * which case, the local network MTU must be observed. > 220: */ > 221: public static final SocketOption IP_DONTFRAGMENT = Just noticed that we are missing `@since 19`. - PR Review Comment: https://git.openjdk.org/jdk/pull/8245#discussion_r1388119739
Re: RFR: JDK-8296240: Augment discussion of test tiers in doc/testing.md [v6]
On Tue, 31 Oct 2023 16:11:47 GMT, Joe Darcy wrote: >> Clarify the intention of tier 1 tests. I'll reflow the paragraph and >> regenerate the HTML file once the wording is agreed upon. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update wording. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16384#pullrequestreview-1711155231
Re: RFR: 8317620: Build JDK tools with ModuleMainClass attribute [v2]
On Thu, 2 Nov 2023 16:19:35 GMT, Mandy Chung wrote: >> Tool modules can be created via `jmod --main-class` option such that >> `ModuleMainClass` attribute will be added in `module-info.class` and the >> module's main class can be launched via `java -m ` without >> specifying the name of the main class. >> >> In addition, for modules with `ModuleMainClass` attribute, jlink will >> pre-resolve the module graph such that when such module is launched at >> runtime (without `--add-modules` or `--limit-modules` option), the runtime >> can skip the module resolution and speed up the startup time. >> >> This PR extends the build system to allow a module to specify the main class >> under `make/modules/$MODULE/Jmod.gmk` file.Also JDK tools with a single >> entry point (or a primary entry point) are candidates to add >> `ModuleMainClass` attribute in `module-info.class` to benefit from the jlink >> optimization. For example, `java -m jdk.jpackage` will be launched using >> the pre-resolved module graph. >> >> Verified manually by running `java -m $MODULE` on the modules with main >> class. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedbacks Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16463#pullrequestreview-1710747941
Re: RFR: 8317620: Build JDK tools with ModuleMainClass attribute
On Wed, 1 Nov 2023 19:58:07 GMT, Mandy Chung wrote: > Tool modules can be created via `jmod --main-class` option such that > `ModuleMainClass` attribute will be added in `module-info.class` and the > module's main class can be launched via `java -m ` without > specifying the name of the main class. > > In addition, for modules with `ModuleMainClass` attribute, jlink will > pre-resolve the module graph such that when such module is launched at > runtime (without `--add-modules` or `--limit-modules` option), the runtime > can skip the module resolution and speed up the startup time. > > This PR extends the build system to allow a module to specify the main class > under `make/modules/$MODULE/Jmod.gmk` file.Also JDK tools with a single > entry point (or a primary entry point) are candidates to add > `ModuleMainClass` attribute in `module-info.class` to benefit from the jlink > optimization. For example, `java -m jdk.jpackage` will be launched using > the pre-resolved module graph. > > Verified manually by running `java -m $MODULE` on the modules with main class. make/modules/jdk.jdi/Jmod.gmk line 26: > 24: # > 25: > 26: JMOD_FLAGS_main_class := --main-class com.sun.tools.example.debug.tty.TTY I'm not sure about this one. Every few years there is a suggestion to deprecate or remove jdb. It's the example debugger from the original JPDA effort in the JDK 1.2 timeframe and hasn't really developed much since then. The main reason is stays around is that it is needed by our tests, and some people say it's useful to have something in production environments where the main stream debuggers can't go. I'm not opposed to "java -m jdk.jdi", it's just that this is a legacy/example tool that isn't the main focus of the jdk.jdi module. - PR Review Comment: https://git.openjdk.org/jdk/pull/16463#discussion_r1379699959
Re: RFR: JDK-8296240: Augment discussion of test tiers in doc/testing.md [v5]
On Tue, 31 Oct 2023 00:23:44 GMT, Joe Darcy wrote: >> Clarify the intention of tier 1 tests. I'll reflow the paragraph and >> regenerate the HTML file once the wording is agreed upon. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Initial check-in of updated HTML file. doc/testing.md line 141: > 139: Roughly speaking, a failure of a test in this tier has the potential to > 140: indicate a problem that would affect many Java programs. Tests in > `tier1` include > 141: tests of HotSpot, core libraries in the `java.base` module, and the > `javac` compiler. I think I'd tweak "core libraries in the java.base module" to say "core classes" or "core APIs", only because there isn't really any notion of a module containing libraries. If someone asks then they'll have to directed to the tier1 definition in test/jdk/TEST.groups as it's hard to come up with a one-line summary. doc/testing.md line 147: > 145: manner. > 146: As a guideline, nearly all individual tests in `tier1` should complete > in less than ten seconds > 147: when run on common configurations used for development. Long-running > tests, "should be complete" is okay, an alternative might be "are expected to complete" so better get across that it is aspirational. - PR Review Comment: https://git.openjdk.org/jdk/pull/16384#discussion_r1377131969 PR Review Comment: https://git.openjdk.org/jdk/pull/16384#discussion_r1377132736
Re: RFR: JDK-8296240: Augment discussion of test tiers in doc/testing.md [v2]
On Sat, 28 Oct 2023 21:03:50 GMT, Joe Darcy wrote: > So in terms of a sentence or two of guidance, I think "aim for 10 seconds or > less almost all of the time for a tier 1 test" is reasonable in this context. Yes, I think making it an aspiration would be better. In passing, you have "selected libraries in the `java.base` module". It might be better to say core APIs in java.base rather than "selected libraries". - PR Comment: https://git.openjdk.org/jdk/pull/16384#issuecomment-1784188449
Re: RFR: JDK-8296240: Augment discussion of test tiers in doc/testing.md [v2]
On Thu, 26 Oct 2023 19:38:55 GMT, Joe Darcy wrote: >> Clarify the intention of tier 1 tests. I'll reflow the paragraph and >> regenerate the HTML file once the wording is agreed upon. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Implement review feedback. The jtreg generated timeStats.txt is useful to see the distribution. For the dev guide, I think it's good to say that tests that run in tier1 must run quickly but I don't think we can suggest 10s as a limit. It might be okay to say that 90% of tests in tier1 are expected to run in <10s and to think carefully when adding tests that take more than XX seconds, I don't know what XX, maybe 30, maybe 60. Right now there are a small number (in percentage terms) of tests running in tier1 with execution times more than a minute but some of these are with debug builds so they will be slower. So I think for the dev guide then some guidance is good. - PR Comment: https://git.openjdk.org/jdk/pull/16384#issuecomment-1782396068
Re: RFR: 8318027: Support alternative name to jdk.internal.vm.compiler [v4]
On Sat, 21 Oct 2023 10:56:01 GMT, Doug Simon wrote: >> The Graal code base has >> [renamed](https://github.com/oracle/graal/commit/1e41203d10db321f86723eac90f6cd0573b08b33) >> its module to `jdk.compiler.graal` as part of preparations for Project >> Galahad. Due to the way Java modules work, this requires a JDK change. The >> core of the issue is that the >> [service](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L37) >> by which HotSpot requests a Graal compilation is defined in JVMCI. Since >> JVMCI is in the boot layer, the service can only be implemented by a >> provider in the boot layer and the package defining the service must be >> exported to the provider's defining module. This export currently targets >> [`jdk.internal.vm.compiler`](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L28) >> and so the binding fails for the new Graal module. To address this, this PR >> reflects the Graal module ren aming, including adjusting the qualified export. >> >> Edit: The target of the rename is now `jdk.graal.compiler` - see >> https://github.com/openjdk/jdk/pull/16189#issuecomment-1773764186 > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Merge tag 'jdk-22+18' into JDK-8318027_rename > >Added tag jdk-22+18 for changeset 3105538d > - rename jdk.compiler.graal to jdk.graal.compiler > - re-fix since tags to reflect current JDK release > - fix copyright dates and @since tags to reflect history > - rename jdk.internal.vm.compiler* to jdk.compiler.graal* Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16189#pullrequestreview-1692387518
Re: RFR: 8318027: Support alternative name to jdk.internal.vm.compiler [v3]
On Fri, 20 Oct 2023 15:45:50 GMT, Doug Simon wrote: >> The Graal code base has >> [renamed](https://github.com/oracle/graal/commit/1e41203d10db321f86723eac90f6cd0573b08b33) >> its module to `jdk.compiler.graal` as part of preparations for Project >> Galahad. Due to the way Java modules work, this requires a JDK change. The >> core of the issue is that the >> [service](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L37) >> by which HotSpot requests a Graal compilation is defined in JVMCI. Since >> JVMCI is in the boot layer, the service can only be implemented by a >> provider in the boot layer and the package defining the service must be >> exported to the provider's defining module. This export currently targets >> [`jdk.internal.vm.compiler`](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L28) >> and so the binding fails for the new Graal module. To address this, this PR >> reflects the Graal module ren aming, including adjusting the qualified export. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > re-fix since tags to reflect current JDK release The module config and test update looks fine. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16189#pullrequestreview-1690440898
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v30]
On Wed, 27 Sep 2023 16:50:33 GMT, Jorn Vernee wrote: >> This patch contains the implementation of the foreign linker & memory API >> JEP for Java 22. The initial patch is composed of commits brought over >> directly from the [panama-foreign >> repo](https://github.com/openjdk/panama-foreign). The main changes found in >> this patch come from the following PRs: >> >> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous >> iterations supported converting Java strings to and from native strings in >> the UTF-8 encoding, we've extended the supported encoding to all the >> encodings found in the `java.nio.charset.StandardCharsets` class. >> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the >> `MemoryLayout::sequenceLayout` factory method which inferred the size of the >> sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A >> client is now required to explicitly specify the sequence size. >> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added: >> `Linker::canonicalLayouts`, which exposes a map containing the >> platform-specific mappings of common C type names to memory layouts. >> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access >> varhandles, as well as byte offset and slice handles derived from memory >> layouts, now feature an additional 'base offset' coordinate that is added to >> the offset computed by the handle. This allows composing these handles with >> other offset computation strategies that may not be based on the same memory >> layout. This addresses use-cases where clients are working with 'dynamic' >> layouts, whose size might not be known statically, such as variable length >> arrays, or variable size matrices. >> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now >> redundant API. Clients can simply use the difference between the base >> address of two memory segments. >> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of >> `SegmentAllocator::allocateArray`, by renaming methods that both allocate + >> initialize memory segments to `allocateFrom`. (see the original PR for the >> problematic case) >> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the >> documentation for variadic functions. >> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to >> make sure the `jdk_foreign` tests can pass on 32-bit machines, taking >> linux-x86 as a test bed. >> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API >> required. The `Linker::nativeLinker` method is not longer allowed to throw >> an `UnsupportedO... > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > drop unneeded @compile tags from jtreg tests Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15103#pullrequestreview-1648059254
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v28]
On Wed, 27 Sep 2023 16:12:46 GMT, Jorn Vernee wrote: > Side note: I don't believe I have to add all the different error message > translations right? Only the English version? That's right, the translations will be updated towards the end of the release. - PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1338874028
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v28]
On Wed, 27 Sep 2023 00:53:25 GMT, Jorn Vernee wrote:
>> This patch contains the implementation of the foreign linker & memory API
>> JEP for Java 22. The initial patch is composed of commits brought over
>> directly from the [panama-foreign
>> repo](https://github.com/openjdk/panama-foreign). The main changes found in
>> this patch come from the following PRs:
>>
>> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous
>> iterations supported converting Java strings to and from native strings in
>> the UTF-8 encoding, we've extended the supported encoding to all the
>> encodings found in the `java.nio.charset.StandardCharsets` class.
>> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the
>> `MemoryLayout::sequenceLayout` factory method which inferred the size of the
>> sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A
>> client is now required to explicitly specify the sequence size.
>> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added:
>> `Linker::canonicalLayouts`, which exposes a map containing the
>> platform-specific mappings of common C type names to memory layouts.
>> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access
>> varhandles, as well as byte offset and slice handles derived from memory
>> layouts, now feature an additional 'base offset' coordinate that is added to
>> the offset computed by the handle. This allows composing these handles with
>> other offset computation strategies that may not be based on the same memory
>> layout. This addresses use-cases where clients are working with 'dynamic'
>> layouts, whose size might not be known statically, such as variable length
>> arrays, or variable size matrices.
>> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now
>> redundant API. Clients can simply use the difference between the base
>> address of two memory segments.
>> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of
>> `SegmentAllocator::allocateArray`, by renaming methods that both allocate +
>> initialize memory segments to `allocateFrom`. (see the original PR for the
>> problematic case)
>> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the
>> documentation for variadic functions.
>> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to
>> make sure the `jdk_foreign` tests can pass on 32-bit machines, taking
>> linux-x86 as a test bed.
>> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API
>> required. The `Linker::nativeLinker` method is not longer allowed to throw
>> an `UnsupportedO...
>
> Jorn Vernee has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fix visibility issues
>
>Reviewed-by: mcimadamore
> - Review comments
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1103:
> 1101: * @throws WrongThreadException if this method is called
> from a thread {@code T},
> 1102: * such that {@code
> isAccessibleBy(T) == false}.
> 1103: * @throws UnsupportedOperationException if {@code charset} is not
> a {@linkplain StandardCharsets standard charset}.
The caller can fix/avoid the exception by providing another value for the
argument so I think IAE is the unchecked exception for this case rather than
UOE.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1338758920
Re: RFR: 8312522: Implementation of Foreign Function & Memory API [v28]
On Wed, 27 Sep 2023 00:53:25 GMT, Jorn Vernee wrote:
>> This patch contains the implementation of the foreign linker & memory API
>> JEP for Java 22. The initial patch is composed of commits brought over
>> directly from the [panama-foreign
>> repo](https://github.com/openjdk/panama-foreign). The main changes found in
>> this patch come from the following PRs:
>>
>> 1. https://github.com/openjdk/panama-foreign/pull/836 Where previous
>> iterations supported converting Java strings to and from native strings in
>> the UTF-8 encoding, we've extended the supported encoding to all the
>> encodings found in the `java.nio.charset.StandardCharsets` class.
>> 2. https://github.com/openjdk/panama-foreign/pull/838 We dropped the
>> `MemoryLayout::sequenceLayout` factory method which inferred the size of the
>> sequence to be `Long.MAX_VALUE`, as this led to confusion among clients. A
>> client is now required to explicitly specify the sequence size.
>> 3. https://github.com/openjdk/panama-foreign/pull/839 A new API was added:
>> `Linker::canonicalLayouts`, which exposes a map containing the
>> platform-specific mappings of common C type names to memory layouts.
>> 4. https://github.com/openjdk/panama-foreign/pull/840 Memory access
>> varhandles, as well as byte offset and slice handles derived from memory
>> layouts, now feature an additional 'base offset' coordinate that is added to
>> the offset computed by the handle. This allows composing these handles with
>> other offset computation strategies that may not be based on the same memory
>> layout. This addresses use-cases where clients are working with 'dynamic'
>> layouts, whose size might not be known statically, such as variable length
>> arrays, or variable size matrices.
>> 5. https://github.com/openjdk/panama-foreign/pull/841 Remove this now
>> redundant API. Clients can simply use the difference between the base
>> address of two memory segments.
>> 6. https://github.com/openjdk/panama-foreign/pull/845 Disambiguate uses of
>> `SegmentAllocator::allocateArray`, by renaming methods that both allocate +
>> initialize memory segments to `allocateFrom`. (see the original PR for the
>> problematic case)
>> 7. https://github.com/openjdk/panama-foreign/pull/846 Improve the
>> documentation for variadic functions.
>> 8. https://github.com/openjdk/panama-foreign/pull/849 Several test fixes to
>> make sure the `jdk_foreign` tests can pass on 32-bit machines, taking
>> linux-x86 as a test bed.
>> 9. https://github.com/openjdk/panama-foreign/pull/850 Make the linker API
>> required. The `Linker::nativeLinker` method is not longer allowed to throw
>> an `UnsupportedO...
>
> Jorn Vernee has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fix visibility issues
>
>Reviewed-by: mcimadamore
> - Review comments
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 640:
> 638: if (!enableNativeAccess.equals("ALL-UNNAMED")) {
> 639: throw new IllegalArgumentException("Only ALL-UNNAMED
> allowed as value for " + ENABLE_NATIVE_ACCESS);
> 640: }
I don't think throwing IAE is right here. It should call abort with a key for
the error message. The value of enableNativeAccess can be used as the parameter
for the message.
test/hotspot/jtreg/compiler/rangechecks/TestRangeCheckHoistingScaledIV.java
line 32:
> 30: * @modules jdk.incubator.vector
> 31: * @compile -source ${jdk.version} TestRangeCheckHoistingScaledIV.java
> 32: * @run main/othervm compiler.rangechecks.TestRangeCheckHoistingScaledIV
Not important but I assume the @compile line can be removed from a number of
tests as it's no longer needed. It was needed for tests that didn't use
@enablePreview.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1338733430
PR Review Comment: https://git.openjdk.org/jdk/pull/15103#discussion_r1338737145
Re: RFR: 8314438: NMT: Performance benchmarks are needed to have a baseline for comparison of improvements
On Tue, 5 Sep 2023 07:53:36 GMT, Afshin Zafari wrote: > A new benchmark for measuring the NMT overhead in `summary` and `detail` > modes. > The tests are run using: > > make CONF=debug test TEST="micro:java.util.NMTBenchmark" > MICRO="RESULTS_FORMAT=json" > > The results are written to a JSON file that can be visualized using [JMH > Visualizer](https://jmh.morethan.io/). > > ### Notes > A separate [issue](https://bugs.openjdk.org/browse/JDK-8316814) is created > for preparing a progfram for validating and analyzing the JMH outputs. > Another separate [issue](https://bugs.openjdk.org/browse/JDK-8316813) is > created for measuring the virtual memory tracing parts of NMT. test/micro/org/openjdk/bench/java/util/NMTBenchmark.java line 24: > 22: */ > 23: > 24: package org.openjdk.bench.java.util; The micros in test/micro/org/openjdk/bench/java/util are usually to test methods in java.util.**. Should this be moved to test/micro/org/openjdk/bench/vm/runtime? - PR Review Comment: https://git.openjdk.org/jdk/pull/15563#discussion_r1335502840
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v6]
On Wed, 20 Sep 2023 11:21:51 GMT, Daniel Fuchs wrote: > Thanks Tim. Should 8308995 be listed in the `@bug` clause of these two tests? I don't think so as these tests are just used to check that changes haven't broken anything. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1727532006
Re: RFR: 8308995: Update Network IO JFR events to be static mirror events [v4]
On Wed, 6 Sep 2023 15:55:21 GMT, Tim Prinzing wrote: >>> https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling >>> https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event >>> generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, >>> event for select ops >> >> Do you have a sense yet on events when the channel is configured >> non-blocking? I realise the threshold is 20ms so they are probably not >> recorded today but I wonder if these code paths will eventually include `|| >> !blocking` or if it useful to record data on all socket read/write ops. > >> > https://bugs.openjdk.org/browse/JDK-8310979 - better exception handling >> > https://bugs.openjdk.org/browse/JDK-8310978 - missing code paths for event >> > generation https://bugs.openjdk.org/browse/JDK-8310994 - non-blocking, >> > event for select ops >> >> Do you have a sense yet on events when the channel is configured >> non-blocking? I realise the threshold is 20ms so they are probably not >> recorded today but I wonder if these code paths will eventually include `|| >> !blocking` or if it useful to record data on all socket read/write ops. > > I think it's useful if trying to trace the calls (i.e. set to 0ms). > Apparently the security manager was being used for that by some. > @tprinzing > Your change (at version > [9fa2745](https://github.com/openjdk/jdk/commit/9fa2745960aea0bc45642081bac89fb5ef65809e)) > is now ready to be sponsored by a Committer. @tprinzing I don't mind sponsoring but I think it would help if you could sync up the branch and provide a summary on the testing was done. The jdk_net and jdk_nio test groups are in tier2. The jdk_jfr test group is in tier3. - PR Comment: https://git.openjdk.org/jdk/pull/14342#issuecomment-1725248917
