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 reason the gtest failed was that we build a static library libgtest.a, which is linked with the gtest libjvm.so. With the changes in this PR, libgtest.a was being built using the `ld -r` + `objcopy --localize-hidden` method. This caused some weird issues with gcc, related to C++ code and the `COMDAT` object info. I failed to track down any proper solution, so instead I added a patch where the libraries that we explicitly declare as `STATIC_LIBRARIES` are linked as before, without the partial linking step. These libraries are only intended for internal consumption (that is, they are linked to and used by another, "external" library), and so the extra protection added by the partial linking is not really needed. It's a bit sad that this did not work, but it is no big deal. It won't affect files released in the image, and it will not be a regression as compared to now. - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2178961562
Re: RFR: 8333268: Fixes for static build [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19478/files - new: https://git.openjdk.org/jdk/pull/19478/files/4ab70df3..b88d813e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19478=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19478=02-03 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8333268: Fixes for static build [v3]
> 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: Do not use partial linking when building static libraries for internal consumption - Changes: - all: https://git.openjdk.org/jdk/pull/19478/files - new: https://git.openjdk.org/jdk/pull/19478/files/e1c46562..4ab70df3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19478=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19478=01-02 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods
On Tue, 18 Jun 2024 16:30:37 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 Build changes are trivially fine. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19774#pullrequestreview-2128470410
Re: RFR: 8333268: Fixes for static build [v2]
On Tue, 18 Jun 2024 16:19:39 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 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1644855137
Re: RFR: 8333268: Fixes for static build
On Thu, 30 May 2024 19:35:44 GMT, Magnus Ihse Bursie wrote: > Do os::lookup_function need to be implemented on Windows too, for symmetry, > even if it is only used on Unix platforms? @AlanBateman suggested to add `lookup_function` to os_windows.cpp as well, but just let it contain ShouldNotReachHere. That sounds like a good solution to me. - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2176657975
Re: RFR: 8333268: Fixes for static build [v2]
> 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 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19478/files - new: https://git.openjdk.org/jdk/pull/19478/files/6b24a789..e1c46562 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19478=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19478=00-01 Stats: 2608 lines in 114 files changed: 1321 ins; 955 del; 332 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8333268: Fixes for static build
On Thu, 30 May 2024 13:00:21 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). The GHA tests fails when building gtest on Linux. This will require some investigation. - PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2168647325
Re: RFR: 8333268: Fixes for static build
On Thu, 30 May 2024 13:00:21 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).
Some open questions:
* Do `os::lookup_function` need to be implemented on Windows too, for symmetry,
even if it is only used on Unix platforms?
* Many of the changes in Hotspot boils down to `os::dll_load` doing the wrong
thing when running with a static build. Perhaps we should provide a better
function that knows how to find and load a symbol for both static and dynamic
builds, and use that instead of making a lot of tests for static/dynamic on
each location we need to look up a symbol from some other JDK library.
* I managed to replace most of the #ifdef STATIC_BUILD with runtime checks.
There are some places remaining though. Apart from the #ifdefs needed for
JNI/JVMTI, which will need spec changes to address, there are code in
java_md_macosx.m, jio.c and awt_Mlib.c that I did not manage to turn into
runtime checks. They will need some more thorough work than just changing an
`#ifdef` to an `if () {`.
* And of course, the code in the build system to share all .o files except the
two linktype files is still under development...
I moved this away from Draft state, since I think it needs some visibility,
especially since it touches several different parts of the code base, and such
reviews tend to take time.
I think the code here is good and basically okay to integrate. This patch will
not on it's own solve the entire problem of building a proper static launcher,
but it takes several important steps along the way. I think the changes here
are reasonable to integrate into mainline at this point.
-
PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2140743300
PR Comment: https://git.openjdk.org/jdk/pull/19478#issuecomment-2168635393
RFR: 8333268: Fixes for static build
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). - Commit messages: - 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 Changes: https://git.openjdk.org/jdk/pull/19478/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19478=00 Issue: https://bugs.openjdk.org/browse/JDK-8333268 Stats: 440 lines in 28 files changed: 203 ins; 74 del; 163 mod Patch: https://git.openjdk.org/jdk/pull/19478.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19478/head:pull/19478 PR: https://git.openjdk.org/jdk/pull/19478
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 13:34:45 GMT, Julian Waters wrote: >> I find the extra trailing newlines through below shell command: >> >> for i in `find . -iname "Makefile*" | sed "/./build/d"` ; do tail -n 2 $i | >> grep -c "^$" | grep -q "^1$" ; if [[ 0 -eq $? ]] ; then echo $i ; fi ; done >> >> >> There are only two files has been found: >> >> ./test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile >> ./test/jdk/java/rmi/reliability/benchmark/bench/Makefile > > Ah, I had not realized that there was more than 1 newline. GitHub's UI > confused me here, so we're good to go GitHub's UI assumes the final line has an line break. If it is missing, it displays a red at the end of the last line. If there is an empty line showing up in the UI, then it is an additional empty line. - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1639649265
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Looks good to me. I assume that you have run an extensive set of tests to verify that this does not break, even in higher tiers? - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2118094672
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 Marked as reviewed by ihse (Reviewer). The wording was much better than what I suggested. Thanks. - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2101492920 PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151870228
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]
On Wed, 5 Jun 2024 17:31:44 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 six > additional commits since the last revision: > > - Move JImageHelper > - Update wording on multi-hop. > - Remove printStackTrace() > - Fix comment. Stream.toList() > - Use pattern matching for instanceof in JRTArchive::equals > - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME As Erik says. You need to add something like: `DEFAULT_DESC: [the inverse of --enable-runtime-link-image]`. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2151836837
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v29]
On Mon, 3 Jun 2024 19:10:22 GMT, Alan Bateman wrote: > Does that proposal sound good? What you basically is saying is that the default value of `packaged-modules` should be `! runtime-link-image` (i.e. the inverse)? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2146996167
Integrated: 8333301: Remove static builds using --enable-static-build
On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie wrote: > The original way of building static libraries in the JDK was to use the > configure argument --enable-static-build, which set the value of the make > variable STATIC_BUILD. (Note that this is not the same as the source code > definition STATIC_BUILD, which will be set by other means as well.) > > This method only ever worked on macOS, and has long since stopped working. It > was originally introduced for the Mobile Project, but I've now learn that not > even they use it anymore. > > It just adds clutter to the build system, and should be removed. This pull request has now been integrated. Changeset: f0bffbce Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/f0bffbce35bb06e724857e8651dd429c4f9df284 Stats: 218 lines in 14 files changed: 4 ins; 206 del; 8 mod 801: Remove static builds using --enable-static-build Reviewed-by: sgehwolf, erikj - PR: https://git.openjdk.org/jdk/pull/19487
RFR: 8333301: Remove static builds using --enable-static-build
The original way of building static libraries in the JDK was to use the configure argument --enable-static-build, which set the value of the make variable STATIC_BUILD. (Note that this is not the same as the source code definition STATIC_BUILD, which will be set by other means as well.) This method only ever worked on macOS, and has long since stopped working. It was originally introduced for the Mobile Project, but I've now learn that not even they use it anymore. It just adds clutter to the build system, and should be removed. - Commit messages: - 801: Remove static builds using --enable-static-build Changes: https://git.openjdk.org/jdk/pull/19487/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19487=00 Issue: https://bugs.openjdk.org/browse/JDK-801 Stats: 218 lines in 14 files changed: 4 ins; 206 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19487.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19487/head:pull/19487 PR: https://git.openjdk.org/jdk/pull/19487
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36: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. >> >> 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: > > rename the template to jaxp-strict.properties.template LGTM - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2078188172
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. I would recommend naming the new file `jaxp-strict.properties.template` instead. This would follow the pattern we have used in the JDK, and I think it is much better at providing clarify as to what this file actually is -- a template for a `.properties` file. - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2128979310
Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null [v3]
On Fri, 24 May 2024 07:24:13 GMT, Matthias Baesken wrote:
>> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing
>> jtreg tests afterwards I run into this error :
>>
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime
>> error: null pointer passed as argument 2, which is declared to never be null
>> #0 0x7fd95bec78d8 in spawnChild
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
>> #1 0x7fd95bec78d8 in startChild
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
>> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
>> #3 0x7fd93797a06d ()
>>
>> this is the memcpy call getting an unexpected null pointer :
>> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
>> Something similar was discussed and fixed here
>> https://bugs.python.org/issue27570 for Python .
>>
>> Similar issue in OpenJDK _
>> https://bugs.openjdk.org/browse/JDK-8332473
>> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed
>> as argument 1, which is declared to never be null
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> handle special case that memcpy src is NULL but a len larger than 0 was
> given
This looks much safer. Thank you!
I think the code can be simplified a bit, as commented. It does not matter
much, so you can keep the current code as well if you think it looks better.
src/java.base/unix/native/libjava/ProcessImpl_md.c line 563:
> 561: offset = copystrings(buf, offset, >envv[0]);
> 562: if (c->pdir != NULL) {
> 563: if (sp.dirlen > 0) {
As long as c->pdir is non-null, I think the code below is safe to execute.
`memcpy(a, b, len)` should be okay if `len` is 0, as long as `a` and `b` are
non-null, right?
So this check here is not needed, I think.
-
PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2128960961
PR Review Comment: https://git.openjdk.org/jdk/pull/19329#discussion_r1613102923
Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null [v2]
On Thu, 23 May 2024 07:26:14 GMT, Matthias Baesken wrote: >> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing >> jtreg tests afterwards I run into this error : >> >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime >> error: null pointer passed as argument 2, which is declared to never be null >> #0 0x7fd95bec78d8 in spawnChild >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562 >> #1 0x7fd95bec78d8 in startChild >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612 >> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712 >> #3 0x7fd93797a06d () >> >> this is the memcpy call getting an unexpected null pointer : >> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null. >> Something similar was discussed and fixed here >> https://bugs.python.org/issue27570 for Python . >> >> Similar issue in OpenJDK _ >> https://bugs.openjdk.org/browse/JDK-8332473 >> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed >> as argument 1, which is declared to never be null > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remarks Roger Riggs As a general principle, I think you should detect errors and report them instead of ignoring them and continue. I don't know any details of this specific code, though. The scenario with a null pointer and non-0 length seems unlikely and would only happen if there is a bug in the calling code. Nevertheless, I think it would be more prudent to guard against this condition. - PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2127421468
Re: RFR: 8332749: Broken link in MemorySegment.Scope.html
On Thu, 23 May 2024 11:39:11 GMT, Per Minborg wrote: > This PR proposes to fix a broken link in the `MemorySegment.Scope` class. The fix looks fine but the bug title sounds like you are modifying an html file... - PR Comment: https://git.openjdk.org/jdk/pull/19366#issuecomment-2127413592
Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null [v2]
On Thu, 23 May 2024 07:26:14 GMT, Matthias Baesken wrote: >> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing >> jtreg tests afterwards I run into this error : >> >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime >> error: null pointer passed as argument 2, which is declared to never be null >> #0 0x7fd95bec78d8 in spawnChild >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562 >> #1 0x7fd95bec78d8 in startChild >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612 >> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec >> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712 >> #3 0x7fd93797a06d () >> >> this is the memcpy call getting an unexpected null pointer : >> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null. >> Something similar was discussed and fixed here >> https://bugs.python.org/issue27570 for Python . >> >> Similar issue in OpenJDK _ >> https://bugs.openjdk.org/browse/JDK-8332473 >> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed >> as argument 1, which is declared to never be null > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remarks Roger Riggs This looks much better. However, if we ever call this with a non-zero `sp.dirlen` and a null `c->pdir`, we'd be in trouble. In the old code, we would have crashed. Now, we will just silently ignore this, and God knows what will happen after that part. I don't have the full context on how this function is used, but if there is some way you can report back with an error to the caller in that case, I think it would be appropriate. - PR Review: https://git.openjdk.org/jdk/pull/19329#pullrequestreview-2073609282
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 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: > > Address review comments Build changes look good. Thanks for trimming down NATIVE_ACCESS_MODULES. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2070573791
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a stricter default configuration [v12]
On Tue, 21 May 2024 20:28:37 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: > > add a note to module-info make/modules/java.xml/Copy.gmk line 35: > 33: $(eval $(call SetupCopyFiles, COPY_XML_MODULE_CONF, \ > 34: DEST := $(CONF_DST_DIR), \ > 35: FILES := $(wildcard > $(TOPDIR)/src/java.xml/share/conf/jaxp*.properties*), \ I don't think you need, nor should have, the asterisk after the extension. You are only copying `.properties` files. Suggestion: FILES := $(wildcard $(TOPDIR)/src/java.xml/share/conf/jaxp*.properties), \ - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1609565653
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Wed, 22 May 2024 07:44:08 GMT, Magnus Ihse Bursie wrote: >> ... still missing... > > Actually, this is a bit strange. I thought jcheck would look for missing > newline at EOF, and that properties files were included in the check > nowadays. I'll need to check this out. I did some testing and it turns out that this is indeed not checked. I believe this is a miss in the Skara reimplementation of jcheck. I've opened https://bugs.openjdk.org/browse/SKARA-2265 to track this. Nevertheless, it would be good if you could fix this. :) - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609491669
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Wed, 22 May 2024 07:42:39 GMT, Magnus Ihse Bursie wrote: >> src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line >> 165: >> >>> 163: >>> 164: runtime.link.info=Linking based on the current run-time image. >>> 165: runtime.link.jprt.path.extra=(run-time image) >> >> Missing newline at last line. > > ... still missing... Actually, this is a bit strange. I thought jcheck would look for missing newline at EOF, and that properties files were included in the check nowadays. I'll need to check this out. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609459326
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Thu, 4 Apr 2024 20:52:59 GMT, Magnus Ihse Bursie wrote: >> Severin Gehwolf has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Use shorter name for the build tool >> - Review feedback from Erik J. >> - Remove dependency on CDS which isn't needed anymore > > src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line > 165: > >> 163: >> 164: runtime.link.info=Linking based on the current run-time image. >> 165: runtime.link.jprt.path.extra=(run-time image) > > Missing newline at last line. ... still missing... - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1609457478
Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null
On Tue, 21 May 2024 14:28:38 GMT, Matthias Baesken wrote: > When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing > jtreg tests afterwards I run into this error : > > /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: > null pointer passed as argument 2, which is declared to never be null > #0 0x7fd95bec78d8 in spawnChild > /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562 > #1 0x7fd95bec78d8 in startChild > /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612 > #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec > /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712 > #3 0x7fd93797a06d () > > this is the memcpy call getting an unexpected null pointer : > memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null. > Something similar was discussed and fixed here > https://bugs.python.org/issue27570 for Python . > > Similar issue in OpenJDK _ > https://bugs.openjdk.org/browse/JDK-8332473 > 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed > as argument 1, which is declared to never be null How come `c->pdir` can be null? Is it if `sp.dirlen` is 0? - PR Comment: https://git.openjdk.org/jdk/pull/19329#issuecomment-2124082247
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Thu, 9 May 2024 07:50:00 GMT, Julian Waters wrote:
>> WIP
>>
>> This changeset contains hsdis for Windows/gcc Port. It supports both the
>> binutils and capstone backends, though the LLVM backend is left out due to
>> compatibility issues encountered during the build. Currently, which gcc
>> distributions are supported is still to be clarified, as several, ranging
>> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason,
>> the build system hack in place at the moment to compile the binutils backend
>> on Windows is still left in place, for now.
>
> Julian Waters has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains two commits:
>
> - Add check for compiler in configure
> - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
Hi Julian, sorry for not getting back to you.
The problem from my PoV is that this is a very large and intrusive patch in the
build of the actual product, for a claimed problem in the tangential hsdis
library. I have not understood a pressing business need to get a Windows/gcc
port for hsdis, which means I can't really prioritize trying to understand this
patch.
As you know, the build system does not currently really separate between "the
OS is Windows" and "the toolchain is Microsoft". I understand that you want to
fix that, and in theory I support it. However, you must also realize that
making a complete fix of this requires a lot of work, for something that there
is no clearly defined need. (After all, cl.exe works fine to create the build,
has no apparent issues, and is as far as an "official" compiler for Windows as
you can get.) That makes it fall squarely in the WIBNIs bucked ("wouldn't it be
nice if...").
If you can fix just the hsdis build without changing anything outside the hsdis
Makefiles, that would be a different story. Such a change would be limited in
scope, easy to say it will not affect the product proper, and be easier to
review.
-
PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2112546029
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v28]
On Wed, 15 May 2024 11:55:39 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 108 commits: > > - 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 > - Fix coment > - Fix comment > - ... and 98 more: https://git.openjdk.org/jdk/compare/957eb611...8cca277e Thanks! Now I am 100% happy with the build changes. :) - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2057984229
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca make/Images.gmk line 100: > 98: > 99: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) > 100: JLINK_JDK_EXTRA_OPTS := $(JLINK_JDK_EXTRA_OPTS) > --generate-linkable-runtime I just noticed this slight improvement: Suggestion: JLINK_JDK_EXTRA_OPTS += --generate-linkable-runtime - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1594774220
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v26]
On Tue, 7 May 2024 16:52:12 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 105 commits: > > - 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 > - Fix coment > - Fix comment > - Fix typo > - Revert some now unneded build changes > - Follow build tools naming convention > - ... and 95 more: https://git.openjdk.org/jdk/compare/23a72a1f...67aea4ca The new build changes look extremely trivial. From a pure build PoV, this is a much simpler solution. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2045924409
Re: Usage of iconv()
On 2024-04-25 20:30, Philip Race wrote: On 4/24/24 4:24 AM, Magnus Ihse Bursie wrote: That is a good question. libiconv is used only on macOS and AIX, for a few libraries, as you say. I just tried removing -liconv from the macOS dependencies and recompiled, just to see what would happen. There were three instances for macOS: libsplashscreen, libjdwp and libinstrument. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. This is called from the launcher, in java.base/share/native/libjli/java.c libjdwp uses it in utf_util.c, where it is used to convert file name and command lines, iiuc. It is likely that we have similar (but better) ways to convert charsets elsewhere in our libraries that can be used instead of libiconv. I guess these are not the only two places where a filename must be converted from the platform charset to UTF8. So whatever replacement there might be, it needs to be something that is available very early in the life of the VM, in fact before there is a VM running. Agreed. But it seems to be that this is something that needs to be handled by libjli, to properly deal with paths and command lines. I'm wondering if the places which to *not* use iconv (or similar) is actually incorrect. /Magnus -phil.
Re: Usage of iconv()
That is a good question. libiconv is used only on macOS and AIX, for a few libraries, as you say. I just tried removing -liconv from the macOS dependencies and recompiled, just to see what would happen. There were three instances for macOS: libsplashscreen, libjdwp and libinstrument. Out of these, libinstrument compiled and linked fine without the -liconv argument. It looks like iconv is referenced in unix/.../EncodingSupport_md.c, but otoh it looks like it is as much (or as little) referenced on macOS as on linux (where we never have linked with -liconv) so it is perhaps just dead code. I did not study it in detail. The code looks very much duplicated from libjdwp. The other two actually failed linking, so they do use libiconv. libsplashscreen uses it in splashscreen_sys.m, where it is used to convert the jar file name. libjdwp uses it in utf_util.c, where it is used to convert file name and command lines, iiuc. It is likely that we have similar (but better) ways to convert charsets elsewhere in our libraries that can be used instead of libiconv. I guess these are not the only two places where a filename must be converted from the platform charset to UTF8. /Magnus On 2024-04-23 14:11, Bernd Eckenfels wrote: Du to a glibc security alert about a charset in iconv() I checked OpenJDK (since I was quite sure encoding is handled in JCL), however there are a few utilities (related to libinstrument and splash Screens) which use iconv. If I see it correctly it’s mostly used for utf8 so it should not expose this particular globe weakness, but I still wonder if that dependency is needed. For some platforms like AIX it even drags on an additional library dependency. (Not to mention different charger tables and especially ugly locale dependencies for containers). Gruß Bernd — https://bernd.eckenfels.net
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Please mark the PR as draft it is not intended for review. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2074481887
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 13:56:32 GMT, Julian Waters wrote: > WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. There's a huge amount of changes for just hsdis... You might have to separate out the infrastructure changes that seem to amount to most of the changes here. This is going to take me a while to get through. - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072458273
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output I agree; the launcher uses two different modes for discovery. If you want to troubleshoot, knowing which of these are attempted is helpful. @AlanBateman If this is not acceptable to add, I must once again iterate my question: What is the use case for the tracing functionality then? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2072349646
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output What is the use case for this tracing functionality? I recently was quite helped by it when debugging static java launching. And in that case, the more logging the better. But that sounds like a very special case. Is this something that end users are supposed to use to help solve problems? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2070309758
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v25]
On Mon, 15 Apr 2024 13:23:22 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Files copy
src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 42:
> 40: */
> 41: static boolean loadLibraryOnlyIfPresent() {
> 42: return true;
I've been following this PR to and fro for a while, so I might have missed
something. But it looks like the code and the documentation is completely at
odds -- the documentation claims the function should return `false`, but in
reality it returns `true`. If the code is correct, the documentation needs to
be fixed or clarified.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1565925292
Re: RFR: 8325163: Enable -Wpedantic on clang [v2]
On Mon, 5 Feb 2024 10:58:17 GMT, Magnus Ihse Bursie wrote: >> Inspired by (the later backed-out) >> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to >> enable `-Wpedantic` for clang. This has already found some irregularities in >> the code, like mistakenly using `#import` instead of `#include`. In this >> patch, I disable warnings for these individual buggy or badly written files, >> but I intend to post follow-up issues on the respective teams to have them >> properly fixed. >> >> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since >> individual warnings in `-Wpedantic` cannot be disabled. This means that code >> like this: >> >> >> #define DEBUG_ONLY(code) code; >> >> DEBUG_ONLY(foo()); >> >> >> will result in a `; ;`. This breaks the C standard, but is benign, and we >> use it all over the place. On clang, we can ignore this by >> `-Wno-extra-semi`, but this is not available on gcc. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > FIx dtrace build I'll close this now. I guess if we want to make progress on this, we will probably have to evaluate every individual warning separately, and add those which does make sense (e.g. `import-preprocessor-directive-pedantic`), and ignore the rest. I personally will not have time nor interest in driving this forward, at least not right now. - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-2056659089
Withdrawn: 8325163: Enable -Wpedantic on clang
On Fri, 2 Feb 2024 15:22:03 GMT, Magnus Ihse Bursie wrote: > Inspired by (the later backed-out) > [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to > enable `-Wpedantic` for clang. This has already found some irregularities in > the code, like mistakenly using `#import` instead of `#include`. In this > patch, I disable warnings for these individual buggy or badly written files, > but I intend to post follow-up issues on the respective teams to have them > properly fixed. > > Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since > individual warnings in `-Wpedantic` cannot be disabled. This means that code > like this: > > > #define DEBUG_ONLY(code) code; > > DEBUG_ONLY(foo()); > > > will result in a `; ;`. This breaks the C standard, but is benign, and we use > it all over the place. On clang, we can ignore this by `-Wno-extra-semi`, but > this is not available on gcc. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17687
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Thu, 4 Apr 2024 16:46:32 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 three > additional commits since the last revision: > > - Use shorter name for the build tool > - Review feedback from Erik J. > - Remove dependency on CDS which isn't needed anymore Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-1991612359
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v24]
On Fri, 5 Apr 2024 08:21:09 GMT, Severin Gehwolf wrote: >> Just to clarify: I did not say the name needed to be long, just that many >> (if not all) tools has used the convention of using the package name >> `build.tools.` and the class name `.java`. >> >> I think the new name sounds . > > Thanks. Yes, the long name was my doing. Sorry. Kind of aligning with the "Donaudampfschiffahrtsgesellschaftskapitän" prejudice of German. ;-) (In Sweden, we have "flaggstångsknoppsförgyllare" so you are not alone) - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1553253017
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v24]
On Thu, 4 Apr 2024 15:38:36 GMT, Erik Joelsson wrote: >> FYI: This was trying to address a comment from @magicus >> https://github.com/openjdk/jdk/pull/14787#discussion_r1514894494 Happy to >> not follow that convention and/or rename the tool. > > I was not aware of such a convention and I can't say I agree with it. It just > seems redundant and unnecessary, but I suppose we should wait for Magnus to > respond. Just to clarify: I did not say the name needed to be long, just that many (if not all) tools has used the convention of using the package name `build.tools.` and the class name `.java`. I think the new name sounds . - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1552430689
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v25]
On Thu, 4 Apr 2024 16:46:32 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 three > additional commits since the last revision: > > - Use shorter name for the build tool > - Review feedback from Erik J. > - Remove dependency on CDS which isn't needed anymore Overall, I think the build system changes in the current revision of the patch look good, but please let me know for a deeper review when things have stabilized. src/jdk.jlink/share/classes/jdk/tools/jlink/resources/jlink.properties line 165: > 163: > 164: runtime.link.info=Linking based on the current run-time image. > 165: runtime.link.jprt.path.extra=(run-time image) Missing newline at last line. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2038205036 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1552425957
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
The build changes look okay.
Do you have any plan of going through all the Java modules and fixing the
issues, or opening JBS issues to have them fixed? Or will these lint warnings
remain disabled for the foreseeable future?
-
Marked as reviewed by ihse (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18527#pullrequestreview-1976255818
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v2]
On Tue, 2 Apr 2024 19:19:59 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.57... > > Volodymyr Paprotski has updated the pull request incrementally with one > additional commit since the last revision: > > remove use of jdk.crypto.ec Build changes are trivially fine. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18583#pullrequestreview-1976208258
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
I apologize for missing this typo. :-( Thanks for fixing it for me, and so
quickly!
-
PR Comment: https://git.openjdk.org/jdk/pull/18484#issuecomment-2021631327
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Thu, 21 Mar 2024 15:27:06 GMT, Severin Gehwolf wrote: >> make/Images.gmk line 131: >> >>> 129: # in FixPath call in order to avoid needing to use strip. >>> 130: RL_JIMAGE_PATH_ARG := $(call >>> FixPath,$(JDK_LINK_OUTPUT_DIR)/lib/modules) >>> 131: RL_MOD_PATH_ARG := $(call FixPath,$(IMAGES_OUTPUTDIR)/jmods) >> >> I'd much rather prefer to use strip and have proper space after commas. This >> is the approach taken elsewhere in the build system. >> >> Suggestion: >> >> RL_JIMAGE_PATH_ARG := $(strip $(call FixPath, >> $(JDK_LINK_OUTPUT_DIR)/lib/modules)) >> RL_MOD_PATH_ARG := $(strip $(call FixPath, $(IMAGES_OUTPUTDIR)/jmods)) > > Thanks! This will also likely change. I'm thinking of just generating the > diff file and putting it into `lib/` of the JDK image. That avoids needing to > call this build-time only jlink plugin and this `FixPath` magic. I see. I'm sorry if I did not fully understand how the ongoing discussions affected build source changes. Maybe I should just put the build part review of this PR on the backburner, and then you can ping me when you think you have a solution that will stick, and I can check how it holds up from a build perspective? >> make/jdk/src/jdk.unsupported_jlink_runtime/share/classes/build/tools/runtimelink/CreateLinkableRuntimePlugin.java >> line 1: >> >>> 1: /* >> >> This file does not at all fit in with the pattern of other build tools, >> which are not module-based but instead live in packages `build.tools.*` in >> the `make/jdk/src/classes` directory. We will need to find a better >> arrangement for this. >> >> First question, do this class really need to be in a separate module? (I'm >> afraid the answer is "yes" but I need to ask it anyway). > >> First question, do this class really need to be in a separate module? (I'm >> afraid the answer is "yes" but I need to ask it anyway). > > Yes, because it uses the `Plugin` ServiceLoader extension using the boot > ModuleLayer. But it's going to be a moot point because this'll get reworked > due to concerns raised by @mlchung (having the plugin pipeline running when > producing a linkable runtime jimage). Ok, fine. Will the new solution still include a build-time only tool? - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534149409 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534146496
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 16:55:14 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:
>
> Move CreateLinkableRuntimePlugin to build folder
>
> Keep runtime link supporting classes in package
> jdk.tools.jlink.internal.runtimelink
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JimageDiffGenerator.java
line 36:
> 34:
> 35: /**
> 36: * Used by the build-only jlink plugin CreateLinkableRuntimePlugin.
Why does this file not live next to the jlink plugin then? Does it have to be
part of the jdk.jlink module?
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/runtimelink/JimageDiffGenerator.java
line 40:
> 38: public class JimageDiffGenerator {
> 39:
> 40: private static final boolean DEBUG = false;
This seems like left-over debug code. If this should go into product code I
suggest you either remove it, or alternatively make it possible to change at
runtime, if the debug functionality will be needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534102351
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534103809
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 16:55:14 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: > > Move CreateLinkableRuntimePlugin to build folder > > Keep runtime link supporting classes in package > jdk.tools.jlink.internal.runtimelink make/jdk/src/jdk.unsupported_jlink_runtime/share/classes/build/tools/runtimelink/CreateLinkableRuntimePlugin.java line 1: > 1: /* This file does not at all fit in with the pattern of other build tools, which are not module-based but instead live in packages `build.tools.*` in the `make/jdk/src/classes` directory. We will need to find a better arrangement for this. First question, do this class really need to be in a separate module? (I'm afraid the answer is "yes" but I need to ask it anyway). - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534094775
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v23]
On Tue, 19 Mar 2024 16:55:14 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: > > Move CreateLinkableRuntimePlugin to build folder > > Keep runtime link supporting classes in package > jdk.tools.jlink.internal.runtimelink make/Images.gmk line 124: > 122: > 123: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) > 124: Please avoid newlines after if statements. Suggestion: make/Images.gmk line 131: > 129: # in FixPath call in order to avoid needing to use strip. > 130: RL_JIMAGE_PATH_ARG := $(call > FixPath,$(JDK_LINK_OUTPUT_DIR)/lib/modules) > 131: RL_MOD_PATH_ARG := $(call FixPath,$(IMAGES_OUTPUTDIR)/jmods) I'd much rather prefer to use strip and have proper space after commas. This is the approach taken elsewhere in the build system. Suggestion: RL_JIMAGE_PATH_ARG := $(strip $(call FixPath, $(JDK_LINK_OUTPUT_DIR)/lib/modules)) RL_MOD_PATH_ARG := $(strip $(call FixPath, $(IMAGES_OUTPUTDIR)/jmods)) make/Images.gmk line 145: > 143: $(eval $(call SetupJavaCompilation, BUILD_JDK_RUNLINK_CLASSES, \ > 144: COMPILER := buildjdk, \ > 145: DISABLED_WARNINGS := try, \ Why do we get warnings in the java code? make/Images.gmk line 171: > 169: > 170: JLINK_JDK_TARGETS += $(jlink_runtime_jdk) > 171: Please avoid newlines before endif statements. Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534080047 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534079021 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534082359 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1534080744
RFR: 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
Similar to [JDK-8328157](https://bugs.openjdk.org/browse/JDK-8328157), we want to move the setting of LDFLAGS to LDFLAGS_JDK[LIB/EXE into SetupJdkLibrary and SetupJdkExecutable. - Commit messages: - 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk Changes: https://git.openjdk.org/jdk/pull/18343/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18343=00 Issue: https://bugs.openjdk.org/browse/JDK-8328177 Stats: 155 lines in 32 files changed: 29 ins; 99 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/18343.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18343/head:pull/18343 PR: https://git.openjdk.org/jdk/pull/18343
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v4]
On Sat, 16 Mar 2024 00:08:04 GMT, Sandhya Viswanathan wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Make all ALIGN/.align aligned > > Looks good to me. Thanks @sviswa7! - PR Comment: https://git.openjdk.org/jdk/pull/18268#issuecomment-2003271932
Integrated: 8328074: Add jcheck whitespace checking for assembly files
On Wed, 13 Mar 2024 11:18:20 GMT, Magnus Ihse Bursie wrote: > As part of the ongoing effort to enable jcheck whitespace checking to all > text files, it is now time to address assembly (.S) files. The hotspot > assembly files were fixed as part of the hotspot mapfile removal, so only a > single incorrect jdk library remains. > > The main issue with `libjsvml` was inconsistent line starts, sometimes using > tabs. I used the `expand` unix command line tool to replace these with spaces. This pull request has now been integrated. Changeset: c342188f Author: Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/c342188fd978dd94e7788fb0fb0345fd8c0eaa9a Stats: 280027 lines in 73 files changed: 0 ins; 0 del; 280027 mod 8328074: Add jcheck whitespace checking for assembly files Reviewed-by: erikj, sviswanathan - PR: https://git.openjdk.org/jdk/pull/18268
Re: RFR: 8325163: Enable -Wpedantic on clang [v2]
On Mon, 5 Feb 2024 10:58:17 GMT, Magnus Ihse Bursie wrote: >> Inspired by (the later backed-out) >> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to >> enable `-Wpedantic` for clang. This has already found some irregularities in >> the code, like mistakenly using `#import` instead of `#include`. In this >> patch, I disable warnings for these individual buggy or badly written files, >> but I intend to post follow-up issues on the respective teams to have them >> properly fixed. >> >> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since >> individual warnings in `-Wpedantic` cannot be disabled. This means that code >> like this: >> >> >> #define DEBUG_ONLY(code) code; >> >> DEBUG_ONLY(foo()); >> >> >> will result in a `; ;`. This breaks the C standard, but is benign, and we >> use it all over the place. On clang, we can ignore this by >> `-Wno-extra-semi`, but this is not available on gcc. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > FIx dtrace build Keep it for a bit more, bot. - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-2000843635
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v4]
On Wed, 13 Mar 2024 11:59:36 GMT, Magnus Ihse Bursie wrote: >> As part of the ongoing effort to enable jcheck whitespace checking to all >> text files, it is now time to address assembly (.S) files. The hotspot >> assembly files were fixed as part of the hotspot mapfile removal, so only a >> single incorrect jdk library remains. >> >> The main issue with `libjsvml` was inconsistent line starts, sometimes using >> tabs. I used the `expand` unix command line tool to replace these with >> spaces. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Make all ALIGN/.align aligned Maybe someone in Hotspot wants to take a look? - PR Comment: https://git.openjdk.org/jdk/pull/18268#issuecomment-1999834049
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v20]
On Thu, 14 Mar 2024 14:24:57 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 comment in autoconf file Maybe we need a tristate instead, "normal image only", "normal image + jmods", "linkable image only"? - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1998625567
RFR: 8328157: Move C[XX]FLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
We are setting one of the flags `CFLAGS_JDKLIB`, `CXXFLAGS_JDKLIB`, `CFLAGS_JDKEXE` or `CXXFLAGS_JDKEXE` to `CFLAGS` or `CXXFLAGS`, respectively, in basically all calls to `SetupJdkLibrary` and `SetupJdkExecutable`. These flag variables contain a lot of duplication. The first step towards bringing some sanity to this mess is to move the setting of these variables into `SetupJdkLibrary/SetupJdkExecutable`. In a few places these standard flags are not set, partially or fullly. This is handled by the new arguments `DEFAULT_CFLAGS := false` (to disable the entire setting) and `C[XX]FLAGS_FILTER_OUT` (which excludes some specific flag) to `SetupJdkLibrary/SetupJdkExecutable`. - Commit messages: - 8328157: Move C[XX]FLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk Changes: https://git.openjdk.org/jdk/pull/18301/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18301=00 Issue: https://bugs.openjdk.org/browse/JDK-8328157 Stats: 170 lines in 31 files changed: 45 ins; 66 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/18301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18301/head:pull/18301 PR: https://git.openjdk.org/jdk/pull/18301
Re: RFR: 8328157: Move C[XX]FLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
On Thu, 14 Mar 2024 12:36:05 GMT, Magnus Ihse Bursie wrote: > We are setting one of the flags `CFLAGS_JDKLIB`, `CXXFLAGS_JDKLIB`, > `CFLAGS_JDKEXE` or `CXXFLAGS_JDKEXE` to `CFLAGS` or `CXXFLAGS`, respectively, > in basically all calls to `SetupJdkLibrary` and `SetupJdkExecutable`. > > These flag variables contain a lot of duplication. > > The first step towards bringing some sanity to this mess is to move the > setting of these variables into `SetupJdkLibrary/SetupJdkExecutable`. > > In a few places these standard flags are not set, partially or fullly. This > is handled by the new arguments `DEFAULT_CFLAGS := false` (to disable the > entire setting) and `C[XX]FLAGS_FILTER_OUT` (which excludes some specific > flag) to `SetupJdkLibrary/SetupJdkExecutable`. I have verified with `COMPARE_BUILD` that there is no significant difference in the build result with or without this patch on Oracle default CI platforms (linux x64/aarch64, macos x64/aarch64, windows x64). - PR Comment: https://git.openjdk.org/jdk/pull/18301#issuecomment-1997359912
RFR: 8328146: Set LIBCXX automatically
We are adding LIBCXX to LIBS in calls to SetupJdkLibrary whenever LINK_TYPE is C++. We should do this automatically in SetupJdkLibrary for C++ linking. I also removed the superfluous `-lc` from some places where it had been added. - Commit messages: - 8328146: Set LIBCXX automatically Changes: https://git.openjdk.org/jdk/pull/18298/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18298=00 Issue: https://bugs.openjdk.org/browse/JDK-8328146 Stats: 27 lines in 10 files changed: 8 ins; 4 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/18298.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18298/head:pull/18298 PR: https://git.openjdk.org/jdk/pull/18298
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks Build changes look good now. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935108939
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I have opened https://bugs.openjdk.org/browse/JDK-8328079 to track the ccache regression. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1994283777
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v3]
On Wed, 13 Mar 2024 11:45:33 GMT, Magnus Ihse Bursie wrote: >> As part of the ongoing effort to enable jcheck whitespace checking to all >> text files, it is now time to address assembly (.S) files. The hotspot >> assembly files were fixed as part of the hotspot mapfile removal, so only a >> single incorrect jdk library remains. >> >> The main issue with `libjsvml` was inconsistent line starts, sometimes using >> tabs. I used the `expand` unix command line tool to replace these with >> spaces. > > Magnus Ihse Bursie has refreshed the contents of this pull request, and > previous commits have been removed. Incremental views are not available. The > pull request now contains two commits: > > - Enable jcheck whitespace checks for .S files > - Run expand on libjsvml I tagged this `core-libs` as that is what @sviswa7 did for changes to libjsvml in https://github.com/openjdk/jdk/pull/8508. I hope this is correct. (We should really add rules to Skara for this library.) - PR Comment: https://git.openjdk.org/jdk/pull/18268#issuecomment-1994211386
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v3]
On Wed, 13 Mar 2024 11:26:14 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has refreshed the contents of this pull request, and >> previous commits have been removed. Incremental views are not available. The >> pull request now contains two commits: >> >> - Enable jcheck whitespace checks for .S files >> - Run expand on libjsvml > > src/jdk.incubator.vector/linux/native/libjsvml/jsvml_d_acos_linux_x86.S line > 37: > >> 35: .text >> 36: # mark_begin; >> 37:.align16,0x90 > > .align seems to ironically not be aligned with the rest of the directives Bad indentation like this is not something jcheck can detect or will complain about. However, I agree that it looks horrible. What's more, this seem to be problematic in multiple locations -- many, but not all, instances of `.align` (and `ALIGN` on Windows) are unaligned. I guess it is due to copy/paste error. I'm sort of reluctant to fix this in this PR, but then again, it just looks too bad. - PR Review Comment: https://git.openjdk.org/jdk/pull/18268#discussion_r1523085132
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files [v4]
> As part of the ongoing effort to enable jcheck whitespace checking to all > text files, it is now time to address assembly (.S) files. The hotspot > assembly files were fixed as part of the hotspot mapfile removal, so only a > single incorrect jdk library remains. > > The main issue with `libjsvml` was inconsistent line starts, sometimes using > tabs. I used the `expand` unix command line tool to replace these with spaces. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Make all ALIGN/.align aligned - Changes: - all: https://git.openjdk.org/jdk/pull/18268/files - new: https://git.openjdk.org/jdk/pull/18268/files/ce1d4c9f..4e729cb6 Webrevs: - full: Webrev is not available because diff is too large - incr: https://webrevs.openjdk.org/?repo=jdk=18268=02-03 Stats: 625 lines in 71 files changed: 0 ins; 0 del; 625 mod Patch: https://git.openjdk.org/jdk/pull/18268.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18268/head:pull/18268 PR: https://git.openjdk.org/jdk/pull/18268
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Wed, 13 Mar 2024 09:49:13 GMT, Aleksey Shipilev wrote: >> make/modules/java.base/Launcher.gmk line 81: >> >>> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >>> 80: OPTIMIZATION := LOW, \ >>> 81: CFLAGS := $(CFLAGS_JDKEXE) \ >> >> There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing >> like flags we try to fill the line. >> Also, the indentation rules are that a broken line should be indented with >> four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. > > My fault, I suggested Chad to do this. So what's the preferred formatting > here? Like BUILD_JEXEC block above does it? > > > CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ > -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ Yeah, that looks good. I was thinking just appending it at the end like: CFLAGS := $(CFLAGS_JDKEXE) -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava \ $(VERSION_CFLAGS), \ but your version definitely looks better! - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522963660
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Wed, 13 Mar 2024 09:50:20 GMT, Joachim Kern wrote: > e.g. We should change the HOTSPOT_TOOLCHAIN_TYPE=xlc to aix, because it is > not toolchain, but OS related. As a consequence the globalDefinitions_xlc.hpp > will become globalDefinitions_aix.hpp No, it's not that simple. First, the `globalDefinitions_` files are included on a per-toolchain basis, not OS basis. Secondly, I think you will want to have the stuff in globalDefinitions_gcc.h. In fact, if you compare globalDefinitions_gcc.h and globalDefinitions_xlc.h side-by-side, you see that they are much more similar than you'd perhaps naively expect. The remaining differences will probably be better expressed as #ifdef AIX inside globalDefinitions_gcc.h, I assume. (But of course, in the end this is up to the hotspot team.) I recommend doing such a side-by-side check yourself to get an understanding of the actual differences. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1994049434
Re: RFR: 8327701: Remove the xlc toolchain [v4]
On Wed, 13 Mar 2024 08:50:10 GMT, Matthias Baesken wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING >> - We need CC_VERSION_OUTPUT before we can check it > > Seems `HOTSPOT_TOOLCHAIN_TYPE=xlc ` and `TARGET_COMPILER_xlc` macros stay, > is this intended ? > (not saying this is a wrong thing to do, but I believed first that all the > xlc toolchain references are replaced by clang?) @MBaesken Yes, I discussed this in length above: https://github.com/openjdk/jdk/pull/18172#issuecomment-1985939418. In short, changing that would mean changing behavior, and that is not something I want to do in a removal refactoring. Also, it is up to you guys to make it work correctly. But I really believe you should address this. Let me know if you need help with the build aspects. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1993862922
Integrated: 8327701: Remove the xlc toolchain
On Fri, 8 Mar 2024 15:29:58 GMT, Magnus Ihse Bursie wrote: > As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. This pull request has now been integrated. Changeset: 107cb536 Author: Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/107cb536e75509ad63b245d20772eb2c3f73d595 Stats: 327 lines in 19 files changed: 24 ins; 266 del; 37 mod 8327701: Remove the xlc toolchain Reviewed-by: jwaters, erikj - PR: https://git.openjdk.org/jdk/pull/18172
Integrated: 8327460: Compile tests with the same visibility rules as product code
On Wed, 6 Mar 2024 11:33:30 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. This pull request has now been integrated. Changeset: cc9a8aba Author: Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/cc9a8aba67f4e240c8de2d1ae15d1b80bfa446a0 Stats: 304 lines in 39 files changed: 69 ins; 159 del; 76 mod 8327460: Compile tests with the same visibility rules as product code Reviewed-by: erikj, jvernee, dholmes, alanb - PR: https://git.openjdk.org/jdk/pull/18135
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING make/modules/java.base/Launcher.gmk line 81: > 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ > 80: OPTIMIZATION := LOW, \ > 81: CFLAGS := $(CFLAGS_JDKEXE) \ There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing like flags we try to fill the line. Also, the indentation rules are that a broken line should be indented with four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522267500
Re: RFR: 8327701: Remove the xlc toolchain [v4]
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Replace CC_VERSION_OUTPUT with CC_VERSION_STRING - We need CC_VERSION_OUTPUT before we can check it - Changes: - all: https://git.openjdk.org/jdk/pull/18172/files - new: https://git.openjdk.org/jdk/pull/18172/files/e7949499..577715b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18172=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18172=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172 PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 12:27:08 GMT, Joachim Kern wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Revert SEARCH_PATH changes
>
> make/autoconf/toolchain.m4 line 940:
>
>> 938: if test "x$OPENJDK_TARGET_OS" = xaix; then
>> 939: # Make sure we have the Open XL version of clang on AIX
>> 940: $ECHO "$CC_VERSION_OUTPUT" | $GREP "IBM Open XL C/C++ for AIX" >
>> /dev/null
>
> This does not work since $CC_VERSION_OUTPUT is unset. We need
> CC_VERSION_OUTPUT=`${XLC_TEST_PATH}ibm-clang++_r --version 2>&1 | $HEAD
> -n 1`
> before, as in the previous code some lines above which you removed.
Yeah, you are right. I confused it with CC_VERSION_STRING.
... duh.
Actually, that is what I should be using instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521667916
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 12:44:55 GMT, Matthias Baesken wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > With this change added, currently configure fails > > > checking for ibm-llvm-cxxfilt... > /opt/IBM/openxlC/17.1.1/tools/ibm-llvm-cxxfilt > configure: error: ibm-clang_r version output check failed, output: > configure exiting with result code > > maybe related to what Joachim pointed out ? @MBaesken Yes, it was the bug Joachim found. It should be fixed now, together will all other comments (except the example version string in the comments). Please re-test. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1991913029
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:15:27 GMT, Magnus Ihse Bursie wrote: >> make/autoconf/toolchain.m4 line 409: >> >>> 407: #Target: powerpc-ibm-aix7.2.0.0 >>> 408: #Thread model: posix >>> 409: #InstalledDir: /opt/IBM/openxlC/17.1.0/bin >> >> Please correct: >> IBM Open XL C/C++ for AIX 17.1.**1** (xxx), clang version >> 1**5**.0.0 >> #Target: **powerpc-ibm-aix7.2.**5**.7** >> #Thread model: posix >> #InstalledDir: /opt/IBM/openxlC/17.1.**1**/bin > > That was just intended as an example code. At one point, this has been > printed by the compiler output (at least according to the source I found on > the net). I tried to find a bunch of different examples from different > platforms and versions. > > But sure, if it makes you happier, I can replace it with a more up to date > example. Actually, your example seems doctored (the `x`) string. I think I'll keep the real world example. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521659238
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 09:14:27 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > make/autoconf/toolchain.m4 line 409: > >> 407: #Target: powerpc-ibm-aix7.2.0.0 >> 408: #Thread model: posix >> 409: #InstalledDir: /opt/IBM/openxlC/17.1.0/bin > > Please correct: > IBM Open XL C/C++ for AIX 17.1.**1** (xxx), clang version > 1**5**.0.0 > #Target: **powerpc-ibm-aix7.2.**5**.7** > #Thread model: posix > #InstalledDir: /opt/IBM/openxlC/17.1.**1**/bin That was just intended as an example code. At one point, this has been printed by the compiler output (at least according to the source I found on the net). I tried to find a bunch of different examples from different platforms and versions. But sure, if it makes you happier, I can replace it with a more up to date example. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521656819
Re: RFR: 8327701: Remove the xlc toolchain [v3]
On Mon, 11 Mar 2024 13:45:46 GMT, Joachim Kern wrote: >> OK this was a flaw in my introduction of clang toolchain for AIX. The >> intention was to keep the xlc options in form of their clang counterparts. I >> will try with a corrected version for clang on AIX and will come back to you. > > OK, the `-Wl,-bbigtoc` is not a compiler option but a linker option and it is > already set in the linker options. > But the `-fpic -mcmodel=large` should be set to avoid creating a jump to > out-of-order code. > > So we can replace the > > JVM_PICFLAG="$PICFLAG" > JDK_PICFLAG="$PICFLAG" > > > code some lines below by > > > if test "x$TOOLCHAIN_TYPE" = xclang && test "x$OPENJDK_TARGET_OS" = xaix; > then > JVM_PICFLAG="-fpic -mcmodel=large" > else > JVM_PICFLAG="$PICFLAG" > fi > JDK_PICFLAG="$PICFLAG" I'm not entirely happy to do this here, as it is a bug fix, and it will change behavior, in contrast to the rest of the patch which is just about removing unused code. But it's a small fix, and I'm messing with the part of the code anyway, so if you are sure that this is the right thing to do I'll sneak it in here. Please make sure it works. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521652510
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:07:15 GMT, Magnus Ihse Bursie wrote: >>> Is Open XL C/C++ considered a compiler or more akin to a development >>> environment like Xcode is for macOS? Depending on which, we could just say >>> clang is the compiler for AIX without needing to say that Open XL is >>> treated like clang, etc >> >> You are raising a good point. Our current concept of "toolchain" is not >> working as smoothly as it should be. We should probably split it into >> "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain >> compiler" (like clang), and/or possibly also some concept of "toolchain sdk". >> >> The work you are doing of untangling Windows from Visual Studio would >> probably influence such a redesign, seeing what part of the Windows >> adaptions which arise from cl, and which are constant given that you compile >> for windows (like the Windows SDK). >> >> But this is a more architectural issue that will need to be resolved in >> another PR, and probably discussed quite a bit beforehand. > >> why did this remove the link to the Supported Build Platforms page? > > You make it sound like it is a bad thing, but in fact I promoted the AIX > build to be of the same status as all other platforms. I cowardly did not put > anything specific about the AIX build in the readme before, but just referred > to the wiki. Now I decided we should actually treat this as a proper platform > and describe the compiler version in the readme, since it is checked in > configure, just like all other platforms. > > There is still a link to the Supported Build Platforms wiki page in the > general part of the readme. > It is clang 15 not 13. Clang 13 was in 17.1.0 Thanks! Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521646622
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Tue, 12 Mar 2024 15:04:56 GMT, Magnus Ihse Bursie wrote: >> Is Open XL C/C++ considered a compiler or more akin to a development >> environment like Xcode is for macOS? Depending on which, we could just say >> clang is the compiler for AIX without needing to say that Open XL is treated >> like clang, etc >> >> Also, why did this remove the link to the Supported Build Platforms page? > >> Is Open XL C/C++ considered a compiler or more akin to a development >> environment like Xcode is for macOS? Depending on which, we could just say >> clang is the compiler for AIX without needing to say that Open XL is treated >> like clang, etc > > You are raising a good point. Our current concept of "toolchain" is not > working as smoothly as it should be. We should probably split it into > "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain > compiler" (like clang), and/or possibly also some concept of "toolchain sdk". > > The work you are doing of untangling Windows from Visual Studio would > probably influence such a redesign, seeing what part of the Windows adaptions > which arise from cl, and which are constant given that you compile for > windows (like the Windows SDK). > > But this is a more architectural issue that will need to be resolved in > another PR, and probably discussed quite a bit beforehand. > why did this remove the link to the Supported Build Platforms page? You make it sound like it is a bad thing, but in fact I promoted the AIX build to be of the same status as all other platforms. I cowardly did not put anything specific about the AIX build in the readme before, but just referred to the wiki. Now I decided we should actually treat this as a proper platform and describe the compiler version in the readme, since it is checked in configure, just like all other platforms. There is still a link to the Supported Build Platforms wiki page in the general part of the readme. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521642995
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 09:30:20 GMT, Julian Waters wrote: > Is Open XL C/C++ considered a compiler or more akin to a development > environment like Xcode is for macOS? Depending on which, we could just say > clang is the compiler for AIX without needing to say that Open XL is treated > like clang, etc You are raising a good point. Our current concept of "toolchain" is not working as smoothly as it should be. We should probably split it into "toolchain environment" (or whatever), like Xcode or Open XL, and "toolchain compiler" (like clang), and/or possibly also some concept of "toolchain sdk". The work you are doing of untangling Windows from Visual Studio would probably influence such a redesign, seeing what part of the Windows adaptions which arise from cl, and which are constant given that you compile for windows (like the Windows SDK). But this is a more architectural issue that will need to be resolved in another PR, and probably discussed quite a bit beforehand. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1521637464
Re: RFR: 8327701: Remove the xlc toolchain [v3]
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Set JVM_PICFLAG="-fpic -mcmodel=large" for AIX - Open XL 17.1.1.4 is clang 15. - Changes: - all: https://git.openjdk.org/jdk/pull/18172/files - new: https://git.openjdk.org/jdk/pull/18172/files/53a05019..e7949499 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18172=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18172=01-02 Stats: 7 lines in 3 files changed: 4 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172 PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 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: > > Only show runtime image suffix for JDK modules Thanks for all the descriptions! They were all very clear and well-written. I'm going to paste some of them into the JBS issue for future record keeping. With this background, I fully agree with the chosen solution. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1991815728
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Mon, 11 Mar 2024 13:06:55 GMT, Erik Joelsson wrote: >> Why the rm? Because jlink refuses to run if the output dir already exists. > > I don't see a race. The `rm` was there in the original code and is no scarier > in the modified version. The jdk image is constructed by a combination of > targets and recipes. The first one to run has to be jlink, then we overlay > more stuff on top. If the makefile dependency resolution concludes that we > need to rerun the jlink step, we clear out the directory completely to make > sure we aren't leaving anything in there that's no longer valid. This is > basically a precaution to guarantee a correct incremental build. It's not > incurring a big build time penalty as jlink would have overwritten all files > it would have created anyway. However, if a module was removed, or a file was > removed from a module, the `rm` helps guarantee that we don't include such a > removed file from a previous build attempt in the final image. > > Or it may even be as Severin says, that jlink refuses to run, I don't > remember. Ok, good, thanks. I did not see the whole picture there. - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1521535618
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 20:04:48 GMT, Roger Riggs wrote: >> Chad Rakoczy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code cleanup > > make/modules/java.base/Launcher.gmk line 85: > >> 83: -DVERSION_INTERIM=$(VERSION_INTERIM) \ >> 84: -DVERSION_UPDATE=$(VERSION_UPDATE) \ >> 85: -DVERSION_PATCH=$(VERSION_PATCH), \ > > Using all 4 is way overkill for the problem at hand. Just the > FEATURE_VERSION is sufficient. > We all know better than to make incompatible changes in minor versions let > alone update or patch version. There is already a `$(VERSION_CFLAGS)` variable defined. It will set all those (and some more). Please use it instead. But then, as Roger says, it is probably overkill to *check* anything but the feature version. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1521525658
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Mon, 11 Mar 2024 06:57:58 GMT, Alan Bateman wrote: > Good cleanup. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1991712859
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Mon, 11 Mar 2024 02:31:02 GMT, David Holmes wrote: >> 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 > > test/hotspot/jtreg/runtime/ErrorHandling/libTestDwarfHelper.h line 24: > >> 22: */ >> 23: >> 24: #include > > Seems unneeded. So what I did here was change: #include "jni.h" #include into #include #include "export.h" #include "jni.h" The reordering was strictly not needed, but putting user includes in front of system ones looked like bad coding to me, and put me in a difficult spot in where to add the new `#include "export.h"` -- next to the current user include even if I thought that was wrong, or have the system includes "sandwitched" between two user includes. Or do you mean that it seems unneeded to include `jni.h` at all? Yes, I agree, but it was there before, and I don't want to make any other changes to the tests. This change is scary enough as it is :-). If you want, I can file a follow-up to remove this include instead. - PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521510510
Re: RFR: 8327460: Compile tests with the same visibility rules as product code
On Mon, 11 Mar 2024 06:57:42 GMT, Alan Bateman 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. Yeah. I understand. I just wanted to give an explanation for why this particular test needed changes that were not present elsewhere (since other exported methods all were tested on Windows). - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1991712352
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Fri, 8 Mar 2024 18:12:40 GMT, Jorn Vernee wrote:
>> 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
>
> test/jdk/java/foreign/CallGeneratorHelper.java line 216:
>
>> 214: if (header) {
>> 215: System.out.println(
>> 216: "#include \"export.h\"\n"
>
> We don't generate these header files any more, so the changes to this file
> are not really needed.
I still wouldn't like to keep the bad hard-coded defines. Are you okay with me
pushing these changes, and then you can remove the parts of the test that are
not actually used anymore? If the code is not used it should not matter much to
you either way.
(I mean I could back out these changes, but then we'd have the bad code in
place while waiting for you to remove it, putting pressure on you to actually
remove it.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521503942
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 10:10:23 GMT, Magnus Ihse Bursie wrote: >> I think what matters for this test test most is which platforms we build >> `jspawnhelper` for, and those platforms indeed are: >> >> >> ifeq ($(call isTargetOs, macosx aix linux), true) >> $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ >> >> >> So I'd say we just add `(os.family == "mac")` here. I would find it a bit >> weird to ask for `os.family != "windows"`, which would implicitly rely on >> exhaustiveness of current os family types. > > Hm, that is not ideal code in the makefile. `jspawnhelper` is called from > `src/java.base/unix/classes/java/lang/ProcessImpl.java`, so it is relied upon > for all Unix implementation. Granted, this is currently the same as the list > "macosx aix linux", but it would be better to express the same logic in the > makefile as in the code. JDK-8327675 was just integrated, which replaces the build logic for jspawnhelper to check for "unix" instead of enumerating all our unixes. I suggest the same pattern should be used here. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1518152090
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 @JornVernee Are you okay with this solution? No JNI in foreign tests. - PR Comment: https://git.openjdk.org/jdk/pull/18135#issuecomment-1986062755
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 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: > > Only show runtime image suffix for JDK modules make/Images.gmk line 128: > 126: JDK_RUN_TIME_IMAGE_SUPPORT_DIR := > $(SUPPORT_OUTPUTDIR)/images/jlink-run-time > 127: JDK_JMODS_DIFF := > $(JDK_RUN_TIME_IMAGE_SUPPORT_DIR)/jimage_packaged_modules_diff.data > 128: JLINK_RUNTIME_CREATE_EXTRA += > --create-linkable-runtime=$(JDK_JMODS_DIFF) My understanding is that the `--create-linkable-runtime` is the primary argument for the jlink invocation in jlink_runtime_jdk. As such, I think it would be much better if this was inlined in place in the jlink_runtime_jdk COMMAND. The "extra" makes it sound like it is optional. In contrast, the value of JLINK_RUNTIME_CREATE_EXTRA that is set above for JLINK_KEEP_PACKAGED_MODULES is indeed "extra". - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1517986887
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 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: > > Only show runtime image suffix for JDK modules make/Images.gmk line 96: > 94: > 95: ifeq ($(JLINK_KEEP_PACKAGED_MODULES), true) > 96: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true) I don't get it. Why don't you use the JDK_LINK_OUTPUT_DIR from just above? make/Images.gmk line 104: > 102: endif > 103: > 104: Is this extra line intentional? make/Images.gmk line 144: > 142: OUTPUT_DIR := $(JDK_IMAGE_DIR), \ > 143: SUPPORT_DIR := $(JDK_RUN_TIME_IMAGE_SUPPORT_DIR), \ > 144: PRE_COMMAND := $(RM) -r $(JDK_IMAGE_DIR), \ This looks scary! Why the rm? Are you sure you are not racing with some other rule that tries to read from the JDK_IMAGE_DIR? - PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1517984170 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1517983052 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1517982726
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v12]
On Fri, 8 Dec 2023 15:44:07 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 46 commits: >> >> - Add @enablePreview for JImageValidator that uses classfile API >> - Fix SystemModulesPlugin after merge >> - Merge branch 'master' into jdk-8311302-jmodless-link >> - Don't show the verbose hint when already verbose >> - Use '_files' over '_resources' as the suffix for listing resources >> - Remove the hidden option hint. >> >>Also adjust the messages being printed when performing >>a run-time image link. >> - Localize messages, switch expression >> - Rename RunImageArchive => JRTArchive and RunImageLinkException => >> RuntimeImageLinkException >> >>Also moved the stamp file to jdk.jlink module. The resources files per >>module now get unconditionally created (empty if no resources not in the >>jimage). >> - First round of addressing review feedback. >> >>- Share resource names (JlinkTask and JlinkResourcesListPlugin) >>- Exclude resources in JlinkResourcesListPlugin the same way >> as done for other plugins. >> - Rename AddRunImageResourcesPlugin => JlinkResourcesListPlugin >> - ... and 36 more: https://git.openjdk.org/jdk/compare/87516e29...a797ea69 > > I tried out the latest commit (a797ea69). > > The output "The default module path, '$java.home/jmods' not present. Use > --verbose to show the full list of plugin options applied" is bit confusing > as it looks like jlink failed but it actually succeeded. Blowing away the > generated image and retrying with --verbose tripped this assert > > java.lang.AssertionError: handling of scratch options failed > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.logPackagedModuleEquivalent(JlinkTask.java:675) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImageProvider(JlinkTask.java:581) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:430) > at jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:302) > at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:56) > at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34) > Caused by: jdk.tools.jlink.internal.TaskHelper$BadArgs: > (...my-original-jdk-directory..)/build/linux-x64/images/jdk/jmods already > exists > at > jdk.jlink/jdk.tools.jlink.internal.TaskHelper.newBadArgs(TaskHelper.java:730) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.lambda$static$12(JlinkTask.java:183) > at > jdk.jlink/jdk.tools.jlink.internal.TaskHelper$Option.process(TaskHelper.java:177) > at > jdk.jlink/jdk.tools.jlink.internal.TaskHelper$OptionsHelper.handleOptions(TaskHelper.java:600) > at > jdk.jlink/jdk.tools.jlink.internal.JlinkTask.logPackagedModuleEquivalent(JlinkTask.java:672) > ... 5 more > > I haven't dug into this yet but I'm puzzled that the file path to where the > original build was created is in the exception messages, is that recorded? > @AlanBateman @mlchung I've now pushed an update of this patch which now uses > a build-time approach as discussed elsewhere. In order to produce a linkable > runtime JDK image, one needs to set --enable-runtime-link-image at configure > time. 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. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1986042463
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote: >> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building >> the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect >> clang by another name, and it uses the clang toolchain in the JDK build. >> Thus the old xlc toolchain is no longer supported, and should be removed. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert SEARCH_PATH changes Also, I believe that the `HOTSPOT_TOOLCHAIN_TYPE=xlc` quirk actually is bad. This means that the clang functionality in `compilerWarnings_gcc.hpp` (where the `_gcc` is hotspot-speak for "clang or gcc") is being ignored, and it means that globalDefinitions_xlc.hpp is in big parts a direct copy of globalDefinitions_gcc.hpp, but apparently lagging in some fixes that has been made in that file. And it means a lot of lines like this: #if defined(TARGET_COMPILER_gcc) || defined(TARGET_COMPILER_xlc) But cleaning up that is left as an exercise to the AIX team; my goal here just primarily to get rid of the old xlc stuff from the build system. - PR Comment: https://git.openjdk.org/jdk/pull/18172#issuecomment-1985939418
Re: RFR: 8327701: Remove the xlc toolchain [v2]
> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert SEARCH_PATH changes - Changes: - all: https://git.openjdk.org/jdk/pull/18172/files - new: https://git.openjdk.org/jdk/pull/18172/files/9f4a059d..53a05019 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18172=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18172=00-01 Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18172/head:pull/18172 PR: https://git.openjdk.org/jdk/pull/18172
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:44:48 GMT, Magnus Ihse Bursie wrote: >> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building >> the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect >> clang by another name, and it uses the clang toolchain in the JDK build. >> Thus the old xlc toolchain is no longer supported, and should be removed. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert SEARCH_PATH changes make/autoconf/toolchain.m4 line 444: > 442: COMPILER_NAME=$2 > 443: SEARCH_LIST="$3" > 444: SEARCH_PATH="$PATH" I am note 100% sure about this; I think the intention of some of the old code amounted to this, but it was not entirely clear. But, then again, I think this is a good idea, and I think we should do this on *all* platforms -- search the toolchain path before the normal path. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1517884839
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:41:16 GMT, Magnus Ihse Bursie wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > make/autoconf/toolchain.m4 line 444: > >> 442: COMPILER_NAME=$2 >> 443: SEARCH_LIST="$3" >> 444: SEARCH_PATH="$PATH" > > I am note 100% sure about this; I think the intention of some of the old code > amounted to this, but it was not entirely clear. > > But, then again, I think this is a good idea, and I think we should do this > on *all* platforms -- search the toolchain path before the normal path. Hm, as I write this, it strikes me as odd that we should not do this already. And of course we do, in `TOOLCHAIN_PRE_DETECTION`, so this snippet snippet is actually redundant. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1517889315
Re: RFR: 8327701: Remove the xlc toolchain
On Fri, 8 Mar 2024 15:29:58 GMT, Magnus Ihse Bursie wrote: > As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain is no longer supported, and should be removed. make/autoconf/toolchain.m4 line 420: > 418: # Remove "Thread model:" and further details from the version > string, and > 419: # collapse into a single line > 420: COMPILER_VERSION_STRING=`$ECHO $COMPILER_VERSION_OUTPUT | \ These changes are not strictly needed, but it makes printing the clang version nicer, and compensates for the removal of the extra version information that was printed by some of the removed code. - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1517880241
