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 make/common/native/Link.gmk line 72: > 70: define CreateStaticLibrary > 71: # Include partial linking when building the static library with clang > on linux > 72: ifeq ($(call isTargetOs, linux macosx), true) Is this mainly for `clang` support for now? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648293391
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 make/modules/java.desktop/lib/AwtLibraries.gmk line 257: > 255: JDK_LIBS := libawt java.base:libjava, \ > 256: LIBS_unix := $(LIBDL) $(LIBM) $(X_LIBS) -lX11 -lXext -lXi > -lXrender \ > 257: -lXtst, \ Same question as for the STATIC_LIB_EXCLUDE_OBJS change with `libjli`. Are the duplicate symbol issues resolved by symbol hiding? I think it's still better to not include those .o files to avoid unnecessary footprint overhead in the binary. - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648292220
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 make/modules/java.base/lib/CoreLibraries.gmk line 148: > 146: $(LIBJLI_EXTRA_FILE_LIST)) > 147: > 148: # Do not include these libz objects in the static libjli library. Why this is no longer needed? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648290693
Re: RFR: 8333268: Fixes for static build [v2]
On Tue, 18 Jun 2024 17:57:29 GMT, Magnus Ihse Bursie wrote:
>> 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 static-linking-progress
>> - Merge branch 'master' into static-linking-progress
>> - Move the exported JVM_IsStaticallyLinked to a better location
>> - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD
>> - Copy fix for init_system_properties_values on linux
>> - Make sure we do not try to build static libraries on Windows
>> - 8333268: Fixes for static build
>
> src/hotspot/os/linux/os_linux.cpp line 605:
>
>> 603:
>> 604: // Get rid of /{client|server|hotspot}, if binary is libjvm.so.
>> 605: // Or, cut off /.
>
> @jianglizhou This code is based on changes in the Hermetic Java repo, but I
> do not fully understand neither the comment nor what the purpose is. If you
> could explain this a bit I'd be grateful.
The specific related commit in the hermetic Java branch is
https://github.com/openjdk/leyden/commit/53aa8f0cf418ab5f435a4b9996c7754fb8505d4b.
The change in os_linux.cpp here is to make sure that the libjvm.so related
path manipulation is conditionally done only. The check at line 599 looks for
"/libjvm.so" substring, so we only chop off (`*pslash = `\0` at line 601) that
part when necessary. In the static JDK case, there is no `libjvm.so` and the
path string is `/bin/javastatic`, which should not be affected.
Otherwise, it could fail.
I found the code was not very easy to follow when running into problems and
fixing for static support. So I added a bit more comments in the code here. The
comment above about `/{client|server|hotspot}` was there originally. I think we
no longer have those directories. We can cleanup that later, since it needs
some more testing.
@magicus, thanks a lot for extracting/reworking/cleaning-up the static Java
changes from the hermetic Java branch. That's a substantial amount of work!
I have one quick comment about the removal of `STATIC_LIB_EXCLUDE_OBJS`
changes. Will post it as separate comment for the related code.
I'll also look closely of the vm & jdk changes and compare those with the
changes in the hermetic Java branch this week.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648283151
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v7]
> This PR adds a new JDK tool, called `jnativescan`, that can be used to find > code that accesses native functionality. Currently this includes `native` > method declarations, and methods marked with `@Restricted`. > > The tool accepts a list of class path and module path entries through > `--class-path` and `--module-path`, and a set of root modules through > `--add-modules`, as well as an optional target release with `--release`. > > The default mode is for the tool to report all uses of `@Restricted` methods, > and `native` method declaration in a tree-like structure: > > > app.jar (ALL-UNNAMED): > main.Main: > main.Main::main(String[])void references restricted methods: > java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment > main.Main::m()void is a native method declaration > > > The `--print-native-access` option can be used print out all the module names > of modules doing native access in a comma separated list. For class path > code, this will print out `ALL-UNNAMED`. > > Testing: > - `langtools_jnativescan` tests. > - Running the tool over jextract's libclang bindings, which use the FFM API, > and thus has a lot of references to `@Restricted` methods. > - tier 1-3 Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: add extra test for missing root modules - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/75c9a6af..a4723494 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19774=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19774=05-06 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19774.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774 PR: https://git.openjdk.org/jdk/pull/19774
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 I've massaged the parsing code to where the help message now looks like this: ... Option Description -- --- -?, -h, --help help --add-modules List of root modules to scan --class-path The class path as used at runtime --module-path The module path as used at runtime --print-native-access print a comma separated list of modules that may perform native access operations. ALL-UNNAMED is used to indicate unnamed modules. --release The runtime version that will run the application --version Print version information and exit I'm not sure if joptsimple has a way to display the option arguments like `[,]` to indicate multiple options (at least I couldn't find it immediately). - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2181397654
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v6]
> 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: review comments Alan - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/06f53e31..75c9a6af Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19774=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19774=04-05 Stats: 98 lines in 2 files changed: 59 ins; 26 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/19774.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774 PR: https://git.openjdk.org/jdk/pull/19774
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 16:13:04 GMT, Alan Bateman wrote: > 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. It seems that it is possible in joptsimple to attach a description to the argument as well, and get something like: --module-pathThe class path as used at runtime It looks like we could also customize the argument type and get something like: --module-path (we'd just need to add a `ModulePath` class with a `valueOf(String)` method) - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2181315973
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: 8334166: Enable binary check
On Wed, 12 Jun 2024 22:38:37 GMT, Zhao Song wrote: > @kevinrushforth said in > [SKARA-2289](https://bugs.openjdk.org/browse/SKARA-2289), 'In general, our > repositories contain source code and not binary files. There are exceptions > to this for images and other similar resources, but otherwise the policy for > most repos is to avoid binary files'. Skara is able to identify binary files > when executing jcheck, but this check is not enabled. Thank you all for the review! - PR Comment: https://git.openjdk.org/jdk/pull/19683#issuecomment-2181128327
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: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Thu, 20 Jun 2024 11:38:26 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/man/jnativescan.1 line 121: >> >>> 119: .TP >>> 120: \f[V]--release\f[R] \f[I]version\f[R] >>> 121: Used to specify the Java SE release that specifies the set of >>> restricted >> >> In principle, the release could also affect which methods are native or not? > > Yes, but we don't warn on `native` method references, only on declarations. > So, it would only affect which methods might be `native` in the user code. > But I think that is already implied by the note on multi-release jars? You are right... declarations inside the JDK are... inside the JDK - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647820666
Re: RFR: 8334618: ubsan: support setting additional ubsan check options [v2]
On Thu, 20 Jun 2024 12:48:40 GMT, Matthias Baesken wrote: >> Sometimes it would be helpful to have configure-support for adding >> additional ubsan check options. >> E.g. support new configure option >> '--with-additional-ubsan-checks=' . > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > use UTIL_ARG_WITH Logic looks ok, but I would prefer if you could break up the lines a bit. - PR Review: https://git.openjdk.org/jdk/pull/19802#pullrequestreview-2130433438
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
On Thu, 20 Jun 2024 12:51:22 GMT, Evemose wrote: > wouldn't it be better to create one uniform tool See my reply here: https://github.com/openjdk/jdk/pull/19774#issuecomment-2179078565 - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2180653743
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: 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 Just as a guest here. Just theoretically, wouldn't it be better to create one uniform tool to detect all "undesired" APIs. This would be more flexible, and potentially could be adapted to use some kind of "meticulous" mode, that will also indicate usage of public APIs, that, while never will be removed, are recommended to remove in favor of new alternatives (like old Date API or old collections". Just a thought btw, not a proposal for now - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2180594098
Re: RFR: 8334618: ubsan: support setting additional ubsan check options
On Thu, 20 Jun 2024 11:31:05 GMT, Matthias Baesken wrote: > Sometimes it would be helpful to have configure-support for adding additional > ubsan check options. > E.g. support new configure option > '--with-additional-ubsan-checks=' . Thanks for the advice, UTIL_ARG_WITH makes the change much smaller. - PR Comment: https://git.openjdk.org/jdk/pull/19802#issuecomment-2180581690
Re: RFR: 8334618: ubsan: support setting additional ubsan check options [v2]
> Sometimes it would be helpful to have configure-support for adding additional > ubsan check options. > E.g. support new configure option > '--with-additional-ubsan-checks=' . Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: use UTIL_ARG_WITH - Changes: - all: https://git.openjdk.org/jdk/pull/19802/files - new: https://git.openjdk.org/jdk/pull/19802/files/7c8cba89..6246d85a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19802=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19802=00-01 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19802.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19802/head:pull/19802 PR: https://git.openjdk.org/jdk/pull/19802
Re: RFR: 8334618: ubsan: support setting additional ubsan check options
On Thu, 20 Jun 2024 11:31:05 GMT, Matthias Baesken wrote: > Sometimes it would be helpful to have configure-support for adding additional > ubsan check options. > E.g. support new configure option > '--with-additional-ubsan-checks=' . I'm not very fond of adding new configure options, but I guess this is ok. A thought, rather than a raw AC_ARG_WITH, newer code should try to adopt UTIL_ARG_WITH instead, for example: UTIL_ARG_WITH(NAME: additional-ubsan-checks, TYPE: string, DEFAULT: [], DESC: [Custom ubsan checks], OPTIONAL: true) UBSAN_CHECKS="-fsanitize=undefined -fsanitize=float-divide-by-zero -fno-sanitize=shift-base -fno-sanitize=alignment $ADDITIONAL_UBSAN_CHECKS" More information about UTIL_ARG_WITH and friends can be found in their implementation documentation here: https://github.com/openjdk/jdk/blob/5cad0b4df7f5ccb6d462dc948c2ea5ad5da6e2ed/make/autoconf/util.m4#L579 - PR Comment: https://git.openjdk.org/jdk/pull/19802#issuecomment-2180537138
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v5]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/4c6abddf..06f53e31 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19774=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19774=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19774.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774 PR: https://git.openjdk.org/jdk/pull/19774
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v4]
> 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: man page review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19774/files - new: https://git.openjdk.org/jdk/pull/19774/files/b5468440..4c6abddf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19774=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19774=02-03 Stats: 26 lines in 1 file changed: 5 ins; 0 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/19774.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19774/head:pull/19774 PR: https://git.openjdk.org/jdk/pull/19774
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:35:07 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - review comments >> - add man page > > src/jdk.jdeps/share/man/jnativescan.1 line 166: > >> 164: .fi >> 165: .PP >> 166: \f[V]app.jar (ALL-UNNAMED)\f[R] is the path to the jar file, with the > > This is a good explanation, I'm not sure whether it's necessary, as the > output seems quite self-explanatory. But I'm ok either way. I think that people that get to this section of the man page are the people who either don't understand the output, or (probably more likely) have an idea but want to be sure of what things mean. In both those cases, I think walking through the output is useful. The description of the command line options is probably enough for more people? - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647458338
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Thu, 20 Jun 2024 11:42:04 GMT, Jorn Vernee wrote: >> src/jdk.jdeps/share/man/jnativescan.1 line 127: >> >>> 125: This option should be set to the version of the runtime under which the >>> 126: application is eventually intended to be run. >>> 127: If this flag is omitted, the version of \f[V]jnativescan\f[R] is used >>> as >> >> Maybe we should point to `Runtime.version()` instead? > > Not sure. If a client is just using the tool, how would they know what > `Runtime.version()` returns? I mean, it's the version of the JDK, but to find > that out they'd have to use another JDK tool (e.g. jshell) to print it out, > or look at the release file. The `jnativescan` version on the other hand is > readily accessible through the `--version` flag. I'll add a note like: `, which is the same as the version of the JDK that the tool belongs to.` - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647442344
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods [v3]
On Wed, 19 Jun 2024 21:30:49 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - review comments >> - add man page > > src/jdk.jdeps/share/man/jnativescan.1 line 121: > >> 119: .TP >> 120: \f[V]--release\f[R] \f[I]version\f[R] >> 121: Used to specify the Java SE release that specifies the set of restricted > > In principle, the release could also affect which methods are native or not? Yes, but we don't warn on `native` method references, only on declarations. So, it would only affect which methods might be `native` in the user code. But I think that is already implied by the note on multi-release jars? > src/jdk.jdeps/share/man/jnativescan.1 line 127: > >> 125: This option should be set to the version of the runtime under which the >> 126: application is eventually intended to be run. >> 127: If this flag is omitted, the version of \f[V]jnativescan\f[R] is used as > > Maybe we should point to `Runtime.version()` instead? Not sure. If a client is just using the tool, how would they know what `Runtime.version()` returns? I mean, it's the version of the JDK, but to find that out they'd have to use another JDK tool (e.g. jshell) to print it out, or look at the release file. The `jnativescan` version on the other hand is readily accessible through the `--version` flag. - PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647436934 PR Review Comment: https://git.openjdk.org/jdk/pull/19774#discussion_r1647441115
RFR: 8334618: ubsan: support setting additional ubsan check options
Sometimes it would be helpful to have configure-support for adding additional ubsan check options. E.g. support new configure option '--with-additional-ubsan-checks=' . - Commit messages: - JDK-8334618 Changes: https://git.openjdk.org/jdk/pull/19802/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19802=00 Issue: https://bugs.openjdk.org/browse/JDK-8334618 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19802.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19802/head:pull/19802 PR: https://git.openjdk.org/jdk/pull/19802
Re: Build failing when disabling the serialgc
Many thanks Erik and Julian for the pointers, I'll explore the --with-build-jdk suggestion, and opened a bug report: - https://bugs.openjdk.org/browse/JDK-8334617 On Thu, Jun 20, 2024 at 9:53 AM wrote: > > On 6/20/24 01:01, Sanne Grinovero wrote: > > Hi all, > > > > I was hoping to build a custom JVM which would only have G1GC as a > > garbage collector, for some experiments. > > > > Essentially I'm running: > > > >> bash configure --with-jvm-variants=custom > >> --with-jvm-features=cds,compiler1,compiler2,g1gc,jfr,jni-check,jvmci,jvmti,management,services,link-time-opt > >> --enable-generate-classlist --disable-manpages > >> --with-vendor-name=Experiments > > However then during the "make images" step, this will fail when the > > jmod(s) are being created, with a long sequence of errors such as: > > > > Creating java.security.jgss.jmod > > Error occurred during initialization of VM > > Option -XX:+UseSerialGC not supported > > > > The same build succeeds if I add the "serialgc" in the list of > > selected features. > > > > Should I open a bug report? > > I've verified that both the `master` branch and `jdk23` are affected, > > and so is the `premain` branch on the Leyden fork. > Yes, I think that may be warranted, however see workaround below. > > I'm assuming that it should be supported to build a JVM without the > > serialgc implementation since it's listed as a feature and therefore > > I'd expect it to be OK to disable it, however I'm wondering about this > > explicitly stated need during the jmod building process; is there > > perhaps a specific need for these to be built with the serialgc, or is > > this just a hint meant to make the build more efficient? > > There is no specific need, this is just for efficiency. Using the > "small" java configuration, saves considerable user time for small short > lived java processes. We currently don't take any limitations in the > configuration for the built JDK into account when using it as the > "BUILD_JDK" during the build. Perhaps we should. On the other hand, if > you are building a limited JDK through configuration options, it can > often be adviceable to supply a separate BUILD_JDK (for performing the > build steps that need a JDK N). This can be done by first building a > standard configuration and then pointing to that JDK image with the > configure arg --with-build-jdk. > > /Erik > > > Many thanks, > > Sanne Grinovero > > >
Re: Build failing when disabling the serialgc
On 6/20/24 01:01, Sanne Grinovero wrote: Hi all, I was hoping to build a custom JVM which would only have G1GC as a garbage collector, for some experiments. Essentially I'm running: bash configure --with-jvm-variants=custom --with-jvm-features=cds,compiler1,compiler2,g1gc,jfr,jni-check,jvmci,jvmti,management,services,link-time-opt --enable-generate-classlist --disable-manpages --with-vendor-name=Experiments However then during the "make images" step, this will fail when the jmod(s) are being created, with a long sequence of errors such as: Creating java.security.jgss.jmod Error occurred during initialization of VM Option -XX:+UseSerialGC not supported The same build succeeds if I add the "serialgc" in the list of selected features. Should I open a bug report? I've verified that both the `master` branch and `jdk23` are affected, and so is the `premain` branch on the Leyden fork. Yes, I think that may be warranted, however see workaround below. I'm assuming that it should be supported to build a JVM without the serialgc implementation since it's listed as a feature and therefore I'd expect it to be OK to disable it, however I'm wondering about this explicitly stated need during the jmod building process; is there perhaps a specific need for these to be built with the serialgc, or is this just a hint meant to make the build more efficient? There is no specific need, this is just for efficiency. Using the "small" java configuration, saves considerable user time for small short lived java processes. We currently don't take any limitations in the configuration for the built JDK into account when using it as the "BUILD_JDK" during the build. Perhaps we should. On the other hand, if you are building a limited JDK through configuration options, it can often be adviceable to supply a separate BUILD_JDK (for performing the build steps that need a JDK N). This can be done by first building a standard configuration and then pointing to that JDK image with the configure arg --with-build-jdk. /Erik Many thanks, Sanne Grinovero
Re: Build failing when disabling the serialgc
Hi Sanne, The reason the build fails when disabling SerialGC is due to the build process explicitly calling the newly compiled Java with -XX:+UseSerialGC to do small workloads. This is not a bug in the JVM, but rather an unfortunate consequence of the way the build system is structured. I believe you would need to patch out certain uses of -XX:+UseSerialGC in the build process with -XX:+UseG1GC instead for this to work, see for instance the following line that shows up during configure: checking flags for boot jdk java command for small workloads... -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1 I'm not sure whether it is a must to use SerialGC when compiling jmods or doing other small workloads during the build process, but I'm leaning towards it just being there to make the build more efficient best regards, Julian
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 Build changes look ok. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19478#pullrequestreview-2129807207
Re: RFR: 8334166: Enable binary check
On Wed, 12 Jun 2024 22:38:37 GMT, Zhao Song wrote: > @kevinrushforth said in > [SKARA-2289](https://bugs.openjdk.org/browse/SKARA-2289), 'In general, our > repositories contain source code and not binary files. There are exceptions > to this for images and other similar resources, but otherwise the policy for > most repos is to avoid binary files'. Skara is able to identify binary files > when executing jcheck, but this check is not enabled. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19683#pullrequestreview-2129797070
Build failing when disabling the serialgc
Hi all, I was hoping to build a custom JVM which would only have G1GC as a garbage collector, for some experiments. Essentially I'm running: > bash configure --with-jvm-variants=custom > --with-jvm-features=cds,compiler1,compiler2,g1gc,jfr,jni-check,jvmci,jvmti,management,services,link-time-opt > --enable-generate-classlist --disable-manpages --with-vendor-name=Experiments However then during the "make images" step, this will fail when the jmod(s) are being created, with a long sequence of errors such as: Creating java.security.jgss.jmod Error occurred during initialization of VM Option -XX:+UseSerialGC not supported The same build succeeds if I add the "serialgc" in the list of selected features. Should I open a bug report? I've verified that both the `master` branch and `jdk23` are affected, and so is the `premain` branch on the Leyden fork. I'm assuming that it should be supported to build a JVM without the serialgc implementation since it's listed as a feature and therefore I'd expect it to be OK to disable it, however I'm wondering about this explicitly stated need during the jmod building process; is there perhaps a specific need for these to be built with the serialgc, or is this just a hint meant to make the build more efficient? Many thanks, Sanne Grinovero
Re: RFR: 8331553: Windows JVM leaks Event and Thread handles when multiple threads are used [v2]
On Wed, 19 Jun 2024 16:50:22 GMT, Daniel Jeliński wrote: >> We use 2 ParkEvent instances per thread. The ParkEvent objects are never >> freed, but they are recycled when a thread dies, so the number of live >> ParkEvent instances is proportional to the maximum number of threads that >> were live at any time. >> >> On Windows, the ParkEvent object wraps a kernel Event object. Kernel objects >> are a limited and costly resource. In this PR, I replace the use of kernel >> events with user-space synchronization. >> >> The new implementation uses WaitOnAddress and WakeByAddressSingle methods to >> implement synchronization. The methods are available since Windows 8. We >> only support Windows 10 and newer, so OS support should not be a problem. >> >> WaitOnAddress was observed to return spuriously, so I added the necessary >> code to recalculate the timeout and continue waiting. >> >> Tier1-5 tests passed. Performance tests were... inconclusive. For example, >> `ThreadOnSpinWaitProducerConsumer` reported 30% better results, while >> `LockUnlock.testContendedLock` results were 50% worse. >> >> Thoughts? > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Update comment What I don't understand: Events are kernel events. We can release Events. WaitOnAddress builds up a kernel-side hash table, but is there a way to ever release these addresses and shrink the table, if you don't need the synchronization anymore? Can you ever release the underlying memory? - PR Comment: https://git.openjdk.org/jdk/pull/19778#issuecomment-2179889219
Re: RFR: 8331553: Windows JVM leaks Event and Thread handles when multiple threads are used [v2]
On Wed, 19 Jun 2024 14:53:27 GMT, Viktor Klang wrote: >> yeah, it's the C++ construction where the constructor and the destructor >> have side effects. It increases the system timer resolution, unless >> `ForceTimeHighResolution` is set. `ForceTimeHighResolution`, contrary to its >> name and help text, forces [low timer >> resolution](https://bugs.openjdk.org/browse/JDK-6435126). Sigh. > > Oh. That's interesting. Of course pre-existing, but the typical pattern we use with RAII objects that may or may not do something is to give it a constructor argument, e.g. `bool activate`, that controls if it is activated. Allocating an RAII object with new/delete seems odd. - PR Review Comment: https://git.openjdk.org/jdk/pull/19778#discussion_r1647001238
