Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v21]
On Mon, 2 Dec 2024 15:12:24 GMT, Magnus Ihse Bursie wrote: >> Hotspot changes look fine. >> >> Thanks > > @dholmes-ora @coleenp @erikj79 Thanks for your reviews! @magicus Thanks for integrating the changes, particularly reworking and making the statically linked launcher build changes cleaner! - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2512408268
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Wed, 13 Nov 2024 01:40:23 GMT, Jiangli Zhou wrote: >> @jianglizhou Thank you for your assistance in figuring out the problem. I >> guess I throw out too much code from the hermetic-java-runtime branch when >> trying to minimize the changes to only build-related stuff. The jimage >> changes were so busy so I probably ignored them a bit too much. > > @magicus Just want to check if there's anything that you are waiting from me > for this PR. My understanding is that you'll update the PR to fix the > incremental build and debuginfo before the last review iteration. Please let > me know if there are anything else. Thanks! > @jianglizhou > > > I think we would need to decide what is the dll_dir with the static JDK > > support and hermetic support. > > From my understanding, `_sun_boot_library_path` does not have any meaning > when running with static builds. As I said before, I think we really ought to > take a step back, consider the wider picture, and refactor the way we > interact with internal native libraries. Determining how to handle > `_sun_boot_library_path` for static builds are definitely part of that work. There might still be some usages of `_sun_boot_library_path` with static JDK. I recently made changes (in our local copy and not yet in https://github.com/openjdk/leyden/tree/hermetic-java-runtime) to use `_sun_boot_library_path` to locate the `jspawnhelper` in `/lib` when spawning child process is needed, for supporting specific hermetic Java testing mode (without building a full hermetic image). At some point, we want to call out the supported execution modes (e.g. hermetic mode, dynamic/tradition mode, hermetic testing mode, etc) in https://docs.google.com/document/d/1LWy9vFDis5-hbJwrFUBH1rc8w8f3oL6O3sOzQXjXVUM/edit?usp=sharing as part of the ongoing discussions with Alan, Ron and others. Agreed that `_sun_boot_library_path` should be part of the bigger picture design/discussion. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2486244160
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Mon, 4 Nov 2024 21:30:10 GMT, Magnus Ihse Bursie wrote: >>> I can confirm that with your patch, and clang, and a complete wipe + >>> rebuild, the .java file loading works. I'm currently testing with gcc as >>> well. >> >> Good. >> >> I tested using gcc. > > @jianglizhou Thank you for your assistance in figuring out the problem. I > guess I throw out too much code from the hermetic-java-runtime branch when > trying to minimize the changes to only build-related stuff. The jimage > changes were so busy so I probably ignored them a bit too much. @magicus Just want to check if there's anything that you are waiting from me for this PR. My understanding is that you'll update the PR to fix the incremental build and debuginfo before the last review iteration. Please let me know if there are anything else. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2472135324
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v8]
On Mon, 4 Nov 2024 21:24:54 GMT, Magnus Ihse Bursie wrote: > > There is no `static-jdk/bin/java.debuginfo`. I do see there's a > > `./support/static-native/launcher/java.debuginfo`. > > Ah, I missed that part. So it's just about copying it to the right place? > Fine, that is trivial to add. Did you verify that these symbols really > worked, though? That is my main concern. The `./support/static-native/launcher/java.debuginfo` works. I can use it in `gdb` to step through code. I haven't specifically looked into how `java.debuginfo` is copied for normal `images/jdk/bin/java`. It's better to handle the `java.debuginfo` for `images/static-jdk/bin/java` the same. As a related note, with `--with-native-debug-symbols`, I believe there is no external `.debuginfo` file. But as long as the `.debuginfo` file for static build is handle the same as for normal build, it will work automatically. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2455832890
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Mon, 4 Nov 2024 21:12:31 GMT, Magnus Ihse Bursie wrote: > I can confirm that with your patch, and clang, and a complete wipe + rebuild, > the .java file loading works. I'm currently testing with gcc as well. Good. I tested using gcc. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2455740530
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Fri, 1 Nov 2024 16:25:59 GMT, Magnus Ihse Bursie wrote: >> As a prerequisite for Hermetic Java, we need a statically linked `java` >> launcher. It should behave like the normal, dynamically linked `java` >> launcher, except that all JDK native libraries should be statically, not >> dynamically, linked. >> >> This patch is the first step towards this goal. It will generate a >> `static-jdk` image with a statically linked launcher. This launcher is >> missing several native libs, however, and does therefore not behave like a >> proper dynamic java. One of the reasons for this is that local symbol hiding >> in static libraries are not implemented yet, which causes symbol clashes >> when linking all static libraries together. This will be addressed in an >> upcoming patch. >> >> All changes in the `src` directory are copied from, or inspired by, changes >> made in [the hermetic-java-runtime branch in Project >> Leyden](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 pull request now contains 10 commits: > > - Merge branch 'master' into static-jdk-image > - Fix bug in filtering out -Wl,--exclude-libs,ALL > - Don't hardcode server variant > - Setup LDFLAGS_STATIC_JDK > - Update GetJREPath comment and remove unnecessary JLI_IsStaticallyLinked > check > - Add lookup asserts > - Remove superfluous SRC. > - Merge branch 'master' into static-jdk-image > - Makefile changes needed for static-launcher and static-jdk-image targets > - Incorporate changes from leyden/hermetic-java-runtime that allows running > a static launcher Fixed a typo in above comment: **does** should be **doesn't** > I notice incremental build with your current PR does update > `static-jdk/bin/java` properly. This should be fixed as well. Should be: I notice incremental build with your current PR doesn't update `static-jdk/bin/java` properly. This should be fixed as well. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2455487724
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Sun, 3 Nov 2024 20:23:32 GMT, Jiangli Zhou wrote: >> 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 10 commits: >> >> - Merge branch 'master' into static-jdk-image >> - Fix bug in filtering out -Wl,--exclude-libs,ALL >> - Don't hardcode server variant >> - Setup LDFLAGS_STATIC_JDK >> - Update GetJREPath comment and remove unnecessary JLI_IsStaticallyLinked >> check >> - Add lookup asserts >> - Remove superfluous SRC. >> - Merge branch 'master' into static-jdk-image >> - Makefile changes needed for static-launcher and static-jdk-image targets >> - Incorporate changes from leyden/hermetic-java-runtime that allows running >> a static launcher > > https://github.com/openjdk/jdk/pull/21861 for adding DEF_STATIC_JNI_OnLoad in > libjimage and libsaproc native libraries. > @jianglizhou I'm trying to follow your comments here... Am I correct in > understanding that you are saying that the missing DEF_STATIC_JNI_OnLoad is > not, in fact, the problem that causes .java files to not load? Because I > tried applying your patch from #21861, and it did not help. The missing`DEF_STATIC_JNI_OnLoad` in jimage.cpp **is** the cause. With the fix, running `static-jdk/bin/java` with `HelloWorld.java` now works for me without failure: build/linux-x86_64-server-slowdebug/images/static-jdk$ bin/java ~/tests/HelloWorld.java HelloWorld > Because I tried applying your patch from #21861, and it did not help. Did you do a clean build? I notice incremental build with your current PR doesn't update `static-jdk/bin/java` properly. This should be fixed as well. Please do `make clean` and rebuild the `images` and `static-jdk-image` to test if you are still running into any failure. If you are still seeing failures, please send me the specific build and run commands. > > > I think we would need to decide what is the `dll_dir` with the static JDK > > support and hermetic support. > > Do you mean that this the core issue related to the .java launching failure? No, the `dll_dir` issue that I pointed out earlier was not the cause. With static JDK support, `libjimage` is statically linked with `static-jdk/bin/java`. When `loadLibrary()` is called to "load" the `jimage` native library, the system first looks for built-in `jimage` native library by looking up `JNI_OnLoad_jimage` symbol. If the symbol exists, it means the `libjimage` is built in and there is no need to `dlopen` the native library from `dll_dir`. That's why the `dll_dir` issue is not the root cause. If it can't find `JNI_OnLoad_jimage` symbol, the system would try to load `libjimage.so` from `dll_dir`. However, that should never happen with static JDK. `JNI_OnLoad_jimage` symbol is defined by `DEF_STATIC_JNI_OnLoad` macro. That's why the missing `DEF_STATIC_JNI_OnLoad` in jimage.cpp is the root cause of the `loadLibrary()` failure. > > I also find it curious that it does work on macOS, but not on Linux. This > indicates, to me, that there is some difference in platform-dependent code > that is different between Linux and mac, and there is not much such code in > this PR. So maybe it is some pre-existing difference in code that provokes > this difference in behavior. I think the macOS might have other bugs that masks this failure. As we discussed in hermetic Java meetings, let's focus on supporting the Linux platform initially. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2455483797
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v9]
On Fri, 1 Nov 2024 16:25:59 GMT, Magnus Ihse Bursie wrote: >> As a prerequisite for Hermetic Java, we need a statically linked `java` >> launcher. It should behave like the normal, dynamically linked `java` >> launcher, except that all JDK native libraries should be statically, not >> dynamically, linked. >> >> This patch is the first step towards this goal. It will generate a >> `static-jdk` image with a statically linked launcher. This launcher is >> missing several native libs, however, and does therefore not behave like a >> proper dynamic java. One of the reasons for this is that local symbol hiding >> in static libraries are not implemented yet, which causes symbol clashes >> when linking all static libraries together. This will be addressed in an >> upcoming patch. >> >> All changes in the `src` directory are copied from, or inspired by, changes >> made in [the hermetic-java-runtime branch in Project >> Leyden](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 pull request now contains 10 commits: > > - Merge branch 'master' into static-jdk-image > - Fix bug in filtering out -Wl,--exclude-libs,ALL > - Don't hardcode server variant > - Setup LDFLAGS_STATIC_JDK > - Update GetJREPath comment and remove unnecessary JLI_IsStaticallyLinked > check > - Add lookup asserts > - Remove superfluous SRC. > - Merge branch 'master' into static-jdk-image > - Makefile changes needed for static-launcher and static-jdk-image targets > - Incorporate changes from leyden/hermetic-java-runtime that allows running > a static launcher https://github.com/openjdk/jdk/pull/21861 for adding DEF_STATIC_JNI_OnLoad in libjimage and libsaproc native libraries. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2453565591
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v8]
On Sat, 2 Nov 2024 01:05:27 GMT, Jiangli Zhou wrote:
> > > I finally noticed that you are testing a precompiled HelloWorld class,
> > > and I have been running with a source file argument to have java compile
> > > it on the fly.
> > > When I try using a pre-compiled HelloWorld, the linux port works for me
> > > too.
> > > @jianglizhou Can you please verify if you can run this with a .java file
> > > directly? To be clear: this works fine on my mac with this PR, and (as I
> > > said), I'm pretty certain it used to work at least at some point during
> > > development on this PR.
> > > ```
> > > ihse@sthihse:/localhome/git/jdk-ALT$ cat > HelloWorld.java
> > > public class HelloWorld {
> > > public static void main(String[] args) {
> > > System.out.println("Hello, world!");
> > > }
> > > }
> > > ihse@sthihse:/localhome/git/jdk-ALT$ javac HelloWorld.java
> > > ihse@sthihse:/localhome/git/jdk-ALT$
> > > ./build/linux-x64/images/static-jdk/bin/java HelloWorld
> > > Hello, world!
> > > ihse@sthihse:/localhome/git/jdk-ALT$
> > > ./build/linux-x64/images/static-jdk/bin/java HelloWorld.java
> > > Error: Unable to load main class
> > > com.sun.tools.javac.launcher.SourceLauncher in module jdk.compiler
> > > Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library
> > > path: /localhome/git/jdk-ALT/build/linux-x64/images/static-jdk/bin
> > > Runtime.exit(1) logging failed: Could not initialize class
> > > jdk.internal.module.SystemModuleFinders$SystemImage
> > > ihse@sthihse:/localhome/git/jdk-ALT$
> > > ```
> >
> >
> > I can reproduce the issue with running a `.java` directly using
> > static-jdk/bin/java built from your branch. The issue does not exist with
> > the `bin/javastatic` built from
> > https://github.com/openjdk/leyden/tree/hermetic-java-runtime (I just
> > tested). I have a hunch of where is problem might be. I'll do some
> > debugging.
> > ```
> > $ bin/java /usr/local/google/home/jianglizhou/tests/HelloWorld.java
> > Error: Unable to load main class
> > com.sun.tools.javac.launcher.SourceLauncher in module jdk.compiler
> > Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library
> > path:
> > /usr/local/google/home/jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk/bin
> > Runtime.exit(1) logging failed: Could not initialize class
> > jdk.internal.module.SystemModuleFinders$SystemImage
> > ```
>
> Ok, I've found the cause. It's because `/static-jdk/bin`
> instead of `/static-jdk/lib` is set as the
> `_sun_boot_library_path` by `Arguments::set_dll_dir()`, during
> `os::init_system_properties_values()`. The path is retrieved via
> `StaticProperty.sunBootLibraryPath()` and used by `ClassLoader.loadLibrary()`
> to locate `jimage`. Hence the failure.
>
> Looking at why it only fails with running with `.java` but not with `.class`,
> a quick looks shows me that the following call path is only taken when
> running with `.java`:
>
> ```
> at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2410)
> at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:927)
> at java.base/java.lang.System.loadLibrary(System.java:2078)
> at
> java.base/jdk.internal.jimage.NativeImageBuffer$1.run(NativeImageBuffer.java:43)
> at
> java.base/jdk.internal.jimage.NativeImageBuffer$1.run(NativeImageBuffer.java:40)
> at
> java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
> at
> java.base/jdk.internal.jimage.NativeImageBuffer.(NativeImageBuffer.java:39)
> at
> java.base/jdk.internal.jimage.BasicImageReader.(BasicImageReader.java:97)
> at
> java.base/jdk.internal.jimage.ImageReader$SharedImageReader.(ImageReader.java:229)
> at
> java.base/jdk.internal.jimage.ImageReader$SharedImageReader.open(ImageReader.java:243)
> at java.base/jdk.internal.jimage.ImageReader.open(ImageReader.java:67)
> at java.base/jdk.internal.jimage.ImageReader.open(ImageReader.java:71)
> at
> java.base/jdk.internal.jimage.ImageReaderFactory$1.apply(ImageReaderFactory.java:70)
> at
> java.base/jdk.internal.jimage.ImageReaderFactory$1.apply(ImageReaderFactory.java:67)
> at
> java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1714)
> at
> java.base/jdk.internal.jimage.ImageReaderFactory.get(ImageReaderFactory.java:61)
> at
> java.base/jdk
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v8]
On Fri, 1 Nov 2024 22:24:28 GMT, Jiangli Zhou wrote:
> > I finally noticed that you are testing a precompiled HelloWorld class, and
> > I have been running with a source file argument to have java compile it on
> > the fly.
> > When I try using a pre-compiled HelloWorld, the linux port works for me too.
> > @jianglizhou Can you please verify if you can run this with a .java file
> > directly? To be clear: this works fine on my mac with this PR, and (as I
> > said), I'm pretty certain it used to work at least at some point during
> > development on this PR.
> > ```
> > ihse@sthihse:/localhome/git/jdk-ALT$ cat > HelloWorld.java
> > public class HelloWorld {
> > public static void main(String[] args) {
> > System.out.println("Hello, world!");
> > }
> > }
> > ihse@sthihse:/localhome/git/jdk-ALT$ javac HelloWorld.java
> > ihse@sthihse:/localhome/git/jdk-ALT$
> > ./build/linux-x64/images/static-jdk/bin/java HelloWorld
> > Hello, world!
> > ihse@sthihse:/localhome/git/jdk-ALT$
> > ./build/linux-x64/images/static-jdk/bin/java HelloWorld.java
> > Error: Unable to load main class
> > com.sun.tools.javac.launcher.SourceLauncher in module jdk.compiler
> > Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library
> > path: /localhome/git/jdk-ALT/build/linux-x64/images/static-jdk/bin
> > Runtime.exit(1) logging failed: Could not initialize class
> > jdk.internal.module.SystemModuleFinders$SystemImage
> > ihse@sthihse:/localhome/git/jdk-ALT$
> > ```
>
> I can reproduce the issue with running a `.java` directly using
> static-jdk/bin/java built from your branch. The issue does not exist with the
> `bin/javastatic` built from
> https://github.com/openjdk/leyden/tree/hermetic-java-runtime (I just tested).
> I have a hunch of where is problem might be. I'll do some debugging.
>
> ```
> $ bin/java /usr/local/google/home/jianglizhou/tests/HelloWorld.java
> Error: Unable to load main class com.sun.tools.javac.launcher.SourceLauncher
> in module jdk.compiler
> Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library path:
> /usr/local/google/home/jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk/bin
> Runtime.exit(1) logging failed: Could not initialize class
> jdk.internal.module.SystemModuleFinders$SystemImage
> ```
Ok, I've found the cause. It's because `/static-jdk/bin`
instead of `/static-jdk/lib` is set as the
`_sun_boot_library_path` by `Arguments::set_dll_dir()`, during
`os::init_system_properties_values()`. The path is retrieved via
`StaticProperty.sunBootLibraryPath()` and used by `ClassLoader.loadLibrary()`
to locate `jimage`. Hence the failure.
Looking at why it only fails with running with `.java` but not with `.class`, a
quick looks shows me that the following call path is only taken when running
with `.java`:
at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2410)
at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:927)
at java.base/java.lang.System.loadLibrary(System.java:2078)
at
java.base/jdk.internal.jimage.NativeImageBuffer$1.run(NativeImageBuffer.java:43)
at
java.base/jdk.internal.jimage.NativeImageBuffer$1.run(NativeImageBuffer.java:40)
at
java.base/java.security.AccessController.doPrivileged(AccessController.java:319)
at
java.base/jdk.internal.jimage.NativeImageBuffer.(NativeImageBuffer.java:39)
at
java.base/jdk.internal.jimage.BasicImageReader.(BasicImageReader.java:97)
at
java.base/jdk.internal.jimage.ImageReader$SharedImageReader.(ImageReader.java:229)
at
java.base/jdk.internal.jimage.ImageReader$SharedImageReader.open(ImageReader.java:243)
at java.base/jdk.internal.jimage.ImageReader.open(ImageReader.java:67)
at java.base/jdk.internal.jimage.ImageReader.open(ImageReader.java:71)
at
java.base/jdk.internal.jimage.ImageReaderFactory$1.apply(ImageReaderFactory.java:70)
at
java.base/jdk.internal.jimage.ImageReaderFactory$1.apply(ImageReaderFactory.java:67)
at
java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1714)
at
java.base/jdk.internal.jimage.ImageReaderFactory.get(ImageReaderFactory.java:61)
at
java.base/jdk.internal.jimage.ImageReaderFactory.getImageReader(ImageReaderFactory.java:85)
at
java.base/jdk.internal.module.SystemModuleFinders$SystemImage.(SystemModuleFinders.java:385)
at
java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.findImageLocation(SystemModuleFinders.java:429)
at
java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.read(SystemModule
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v8]
On Fri, 1 Nov 2024 16:09:20 GMT, Magnus Ihse Bursie wrote:
> I finally noticed that you are testing a precompiled HelloWorld class, and I
> have been running with a source file argument to have java compile it on the
> fly.
>
> When I try using a pre-compiled HelloWorld, the linux port works for me too.
>
> @jianglizhou Can you please verify if you can run this with a .java file
> directly? To be clear: this works fine on my mac with this PR, and (as I
> said), I'm pretty certain it used to work at least at some point during
> development on this PR.
>
> ```
> ihse@sthihse:/localhome/git/jdk-ALT$ cat > HelloWorld.java
> public class HelloWorld {
> public static void main(String[] args) {
> System.out.println("Hello, world!");
> }
> }
> ihse@sthihse:/localhome/git/jdk-ALT$ javac HelloWorld.java
> ihse@sthihse:/localhome/git/jdk-ALT$
> ./build/linux-x64/images/static-jdk/bin/java HelloWorld
> Hello, world!
> ihse@sthihse:/localhome/git/jdk-ALT$
> ./build/linux-x64/images/static-jdk/bin/java HelloWorld.java
> Error: Unable to load main class com.sun.tools.javac.launcher.SourceLauncher
> in module jdk.compiler
> Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library path:
> /localhome/git/jdk-ALT/build/linux-x64/images/static-jdk/bin
> Runtime.exit(1) logging failed: Could not initialize class
> jdk.internal.module.SystemModuleFinders$SystemImage
> ihse@sthihse:/localhome/git/jdk-ALT$
> ```
I can reproduce the issue with running a `.java` directly using
static-jdk/bin/java built from your branch. The issue does not exist with the
`bin/javastatic` built from
https://github.com/openjdk/leyden/tree/hermetic-java-runtime (I just tested). I
have a hunch of where is problem might be. I'll do some debugging.
$ bin/java /usr/local/google/home/jianglizhou/tests/HelloWorld.java
Error: Unable to load main class com.sun.tools.javac.launcher.SourceLauncher in
module jdk.compiler
Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library path:
/usr/local/google/home/jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk/bin
Runtime.exit(1) logging failed: Could not initialize class
jdk.internal.module.SystemModuleFinders$SystemImage
-
PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2452668656
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive [v5]
On Thu, 31 Oct 2024 07:48:40 GMT, Aleksey Shipilev wrote: >> This is forked from >> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) and filed as a >> general issue for archived boxed Integer cache when it's recreated at >> runtime. In short, current code drops the entire primitive cache when the >> CDS archived version of the cache is too short. This poses a problem with >> code that uses CDS archived cache instances, since the boxed equality would >> break when comparing the CDS-archived value and the IntegerCache value >> recreated at runtime. >> >> Ioi suggested a fix here: >> https://github.com/openjdk/jdk/pull/21672#issuecomment-2434359711. I >> separately arrived to the same idea. This PR implements it. `IntegerCache` >> gets the special treatment, because it is the only cache that can be tuned. >> Other caches just prevent the use of bad archived cache (which I think >> should realistically never happen) without re-creating it. >> >> I tested this patch with original reproducer from >> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) -- and it starts >> to pass. >> >> Additional testing: >> - [x] macos-aarch64-server-fastdebug, >> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) reproducer now >> passes >> - [x] macos-aarch64-server-fastdebug, new regression test fails without the >> fix, passes with it >> - [x] linux-aarch64-server-fastdebug, `all` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > archivedIdx -> archivedSize The latest version looks cleaner. - Marked as reviewed by jiangli (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21737#pullrequestreview-2408323125
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive [v5]
On Thu, 31 Oct 2024 07:32:01 GMT, Aleksey Shipilev wrote:
>> The runtime cache would only store the additional boxed Integers not in the
>> archived cache, without affecting the archived boxed Integers. Something
>> like the following would work (I didn't test all corner cases):
>>
>>
>> static final int archivedSize;
>> ...
>> archivedSize = archivedCache == null ? 0 : archivedCache.length;
>> if (archivedCache == null || size > archivedCache.length) {
>> int runtimeCacheSize = size - archivedSize;
>> Integer[] c = new Integer[runtimeCacheSize];
>> int j = low + archivedSize;
>> for(int i = 0; i < runtimeCacheSize; i++) {
>> c[i] = new Integer(j++);
>> }
>>
>> cache = c;
>> if (archivedCache == null) {
>> archivedCache = c;
>> }
>> } else {
>> cache = null;
>> }
>>
>>
>> `Integer.valueOf()` would retrieve the cached boxed Integer either from the
>> archived cache or the runtime created cache, if the value is within the
>> runtime [low, high].
>>
>>
>> public static Integer valueOf(int i) {
>> if (i >= IntegerCache.low && i <= IntegerCache.high) {
>> int index = i + (-IntegerCache.low);
>> if (IntegerCache.archivedSize != 0 && IntegerCache.archivedSize
>> >= index) {
>> return IntegerCache.archivedCache[index];
>> } else {
>> return IntegerCache.cache[index - IntegerCache.archivedSize];
>> }
>> }
>> return new Integer(i);
>> }
>
> Ah, I see. I don't think we want to do this, though. I vaguely remember
> `Integer.valueOf` code shape being important for things like C2 optimizations
> around EA and autoboxing elimination. The beauty of staying within a single
> array is avoiding a whole class of these issues.
I think it's mostly ok if we don't take the archivedCache+runtimeCache
approach. The overhead is not too large in common cases, as likely the
archivedCache would have the default size if runtime changes to use a different
`high` value.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21737#discussion_r1824689846
Integrated: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Fri, 25 Oct 2024 21:25:54 GMT, Jiangli Zhou wrote: > Moved from > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. > > Existing comment threads from closed > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 This pull request has now been integrated. Changeset: 688e92e7 Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/688e92e7f5febddd2935cb7f500dd3f10fbd9401 Stats: 13 lines in 1 file changed: 0 ins; 1 del; 12 mod 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper Reviewed-by: iklam, shade, alanb - PR: https://git.openjdk.org/jdk/pull/21722
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v3]
On Wed, 30 Oct 2024 22:07:34 GMT, Jiangli Zhou wrote: >> Moved from >> https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: >> >> Please review the fix that uses String type for the mapped value in >> ModuleLoaderMap.Mapper map (Map). Please see details in >> https://bugs.openjdk.org/browse/JDK-8342642, thanks. >> >> Existing comment threads from closed >> https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Remove outdated comment. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/21722#issuecomment-2448853394
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Tue, 29 Oct 2024 16:22:47 GMT, Aleksey Shipilev wrote:
>> Jiangli Zhou has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjust comment.
>
> src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java line 92:
>
>> 90: } else if (PLATFORM_LOADER_NAME.equals(loader)) {
>> 91: return PLATFORM_CLASSLOADER;
>> 92: } else { // BOOT_LOADER_INDEX
>
> Drop the `// BOOT_LOADER_INDEX`?
Looks like the trivial update also needs to be reviewed, after the previous
review/approval. @shipilev can you review again? Thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21722#discussion_r1823640088
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Thu, 24 Oct 2024 23:36:19 GMT, Jiangli Zhou wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Don't hardcode server variant > >> > > When trying to sort out the LDFLAGS issues, it turned out that I could >> > > not run the linux launcher at all, not even when checking out older >> > > commits of this PR. I am almost at a loss here; I assume that this >> > > worked when I created this PR (even though I only did ad hoc testing at >> > > the time), and I'm not sure if my testing then was inadequate or if >> > > something else has changed during that time with my environment. >> > > I'm trying to retrace my steps in how I got to this branch, but I have >> > > unfortunately deleted much of the intermediate steps. >> > > @jianglizhou Can you check if you can build and run a simple >> > > HelloWorld.java with the static launcher in this PR on linux? >> > > `images/static-jdk/bin/java --version` works for me, but not >> > > `images/static-jdk/bin/java HelloWorld.java`, which fails with an error >> > > that indicates it cannot locate the jimage library. >> > >> > >> > @magicus I just noticed your above notes now (I'm been chasing an >> > unrelated long G1 Evacuation Pause issue). I'll test your changes >> > tomorrow. Will update you in the thread. >> >> Sorry for the delay. I finally found some time to look into this today. >> Running the latest `static-jdk` `bin/java` built from your branch fails for >> me as well: >> >> ``` >> $ bin/java >> Error: failed , because bin/java: undefined symbol: JNI_CreateJavaVM >> ``` >> >> I only took a quick look, but I think this looks like >> https://bugs.openjdk.org/browse/JDK-8339522 that I filed in September. >> >> ``` >> $ nm bin/java | grep JNI_CreateJavaVM >> 0162733c t JNI_CreateJavaVM >> 01626e75 t _ZL22JNI_CreateJavaVM_innerPP7JavaVM_PPvS2_ >> jianglizhou@leia:~/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk$ >> nm -D bin/java | grep JNI_CreateJavaVM >> ``` >> >> The VM dynamic symbols are not exported properly. This is related to the >> [#20837 >> (comment)](https://github.com/openjdk/jdk/pull/20837#discussion_r1744611776) >> discussion comment thread. > > Checking the `ld` command line, I think it is indeed > https://bugs.openjdk.org/browse/JDK-8339522 issue. `-Wl,--exclude-libs,ALL` > is included in the command: > > $ cat ./support/static-native/launcher/BUILD_LAUNCHER_java_run_ld.cmdline > /usr/bin/gcc -Wl,-z,defs -Wl,-z,relro -Wl,-z,now -Wl,--no-as-needed > -Wl,-z,noexecstack -m64 -pie -Wl,-z,defs -Wl,-z,relro -Wl,-z,now > -Wl,--no-as-needed -Wl,--exclude-libs,ALL -Wl,-z,noexecstack -m64 > -Wl,-rpath,$ORIGIN -Wl,--disable-new-dtags -Wl,-rpath,$ORIGIN/../lib > -Wl,--disable-new-dtags -o /...jianglizhou/github/jdk_pull_20837/build/... > @jianglizhou I fixed the bug with `-Wl,--exclude-libs,ALL` not being properly > filtered out. > > However, I still get the jimage problem: > > ``` > $ build/linux-x64/images/static-jdk/bin/java > closed/make/scripts/GenerateSecureId.java > Error: Unable to load main class com.sun.tools.javac.launcher.SourceLauncher > in module jdk.compiler > Caused by: java.lang.UnsatisfiedLinkError: no jimage in system library path: > /localhome/git/jdk-ALT/build/linux-x64/images/static-jdk/bin > Runtime.exit(1) logging failed: Could not initialize class > jdk.internal.module.SystemModuleFinders$SystemImage > ``` Hi @magicus, I applied your latest commit, https://github.com/openjdk/jdk/pull/20837/commits/1d375bb4060540930f119881e91dbc27154c493e. I built JDK images for `linux-x86_64-server-slowdebug` with the new change. `gcc` was used in my build. I was able to run `HelloWorld` using the static-jdk/bin/java without failure. I think that's good news. ~/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk$ bin/java -cp ~/tests/hw.jar HelloWorld HelloWorld Do you want to try applying your patch from this PR in a clean github local branch to see if that makes any difference? - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2448803294
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v3]
On Wed, 30 Oct 2024 22:07:34 GMT, Jiangli Zhou wrote: >> Moved from >> https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: >> >> Please review the fix that uses String type for the mapped value in >> ModuleLoaderMap.Mapper map (Map). Please see details in >> https://bugs.openjdk.org/browse/JDK-8342642, thanks. >> >> Existing comment threads from closed >> https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 >> - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Remove outdated comment. Thanks for the reviews and discussions! - PR Comment: https://git.openjdk.org/jdk/pull/21722#issuecomment-2448613586
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Tue, 29 Oct 2024 16:22:47 GMT, Aleksey Shipilev wrote:
>> Jiangli Zhou has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Adjust comment.
>
> src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java line 92:
>
>> 90: } else if (PLATFORM_LOADER_NAME.equals(loader)) {
>> 91: return PLATFORM_CLASSLOADER;
>> 92: } else { // BOOT_LOADER_INDEX
>
> Drop the `// BOOT_LOADER_INDEX`?
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21722#discussion_r1823470882
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v3]
> Moved from > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. > > Existing comment threads from closed > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Remove outdated comment. - Changes: - all: https://git.openjdk.org/jdk/pull/21722/files - new: https://git.openjdk.org/jdk/pull/21722/files/3234aa23..a25e0b46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21722&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21722&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21722.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21722/head:pull/21722 PR: https://git.openjdk.org/jdk/pull/21722
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive
On Tue, 29 Oct 2024 11:58:44 GMT, Aleksey Shipilev wrote: > Second problem is the behavior for boxed Integers outside [-128; 127] range. > JLS 5.1.7 has no provisions about the identity of these boxes: neither a > guarantee they would be cached, nor a guarantee that they would _not_ be > cached. JLS 5.1.7 note explicitly calls that out: "For other values, the rule > disallows any assumptions about the identity of the boxed values on the > programmer's part. This allows (but does not require) sharing of some or all > of these references.". That reassuring. Thanks for finding that in the spec! I think that removes the cancer. - PR Comment: https://git.openjdk.org/jdk/pull/21737#issuecomment-2448143169
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive [v4]
On Tue, 29 Oct 2024 13:04:18 GMT, Aleksey Shipilev wrote:
>> src/java.base/share/classes/java/lang/Integer.java line 964:
>>
>>> 962: Integer[] c = new Integer[size];
>>> 963: int j = low;
>>> 964: // Use all cached values from the archive to avoid
>>> breaking
>>
>> Alternatively, you could use a separate array to store **only** the runtime
>> created cached instances, without copying the references from the archived
>> cache. Both the archived cache and runtime added cache are used. That has
>> slightly less memory and efficiency overhead, but almost unnoticeable.
>
> I don't think I understand. The whole point is to make sure the runtime cache
> instances are the same as archive cache instances, right? Using both archived
> and runtime caches leads to identity inconsistencies: if we `==` compare the
> archived `Integer` box and the runtime `Integer` box of the same value in
> [-128; 127], they should equal each other. So we have to make sure we use the
> parts we have already committed to use (from the archive), and add whatever
> cache elements are missing at runtime. I don't see how it works otherwise.
The runtime cache would only store the additional boxed Integers not in the
archived cache, without affecting the archived boxed Integers. Something like
the following would work (I didn't test all corner cases):
static final int archivedSize;
...
archivedSize = archivedCache == null ? 0 : archivedCache.length;
if (archivedCache == null || size > archivedCache.length) {
int runtimeCacheSize = size - archivedSize;
Integer[] c = new Integer[runtimeCacheSize];
int j = low + archivedSize;
for(int i = 0; i < runtimeCacheSize; i++) {
c[i] = new Integer(j++);
}
cache = c;
if (archivedCache == null) {
archivedCache = c;
}
} else {
cache = null;
}
`Integer.valueOf()` would retrieve the cached boxed Integer either from the
archived cache or the runtime created cache, if the value is within the runtime
[low, high].
public static Integer valueOf(int i) {
if (i >= IntegerCache.low && i <= IntegerCache.high) {
int index = i + (-IntegerCache.low);
if (IntegerCache.archivedSize != 0 && IntegerCache.archivedSize >=
index) {
return IntegerCache.archivedCache[index];
} else {
return IntegerCache.cache[index - IntegerCache.archivedSize];
}
}
return new Integer(i);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21737#discussion_r1823234380
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Mon, 28 Oct 2024 10:54:29 GMT, Alan Bateman wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adjust comment. > > src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java line 58: > >> 56: >> 57: /** >> 58: * Map from module to a class loader name. The name is resolved >> to the > > Would you mind adjusting this to "Map from module name to class loader name"? > Otherwise switching this to using a string is okay. @AlanBateman Can you approve the PR? Thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/21722#discussion_r1820942450
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive
On Mon, 28 Oct 2024 23:15:54 GMT, Vladimir Ivanov wrote: > As an example, if `IntegerCache.high` is set to `1` during archiving, then > `ModuleLoaderMap.PLATFORM_LOADER_INDEX=1` ends up pointing to archived > instance and `ModuleLoaderMap.APP_LOADER_INDEX=2` points to non-archived > instance which are then persisted in the archive as part of the sub-graph > rooted at `ModuleLoaderMap.map`. The minimum Integer cache `high` value is 127 (https://github.com/openjdk/jdk/blob/f0075d593db657182e1857e54710a1052e9d1cf0/src/java.base/share/classes/java/lang/Integer.java#L946). `Integer.valueOf` [spec](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Integer.html#equals(java.lang.Object)) also mentions "will always cache values in the range -128 to 127, inclusive, and may cache other values outside of this range". I agree with @iwanowww', the concern about the issue described by @iwanowww's comment above still exists when archived (heap object) subgraph contains instances of boxed type outside the cached range and when equality of identity hash comparison is assumed. - PR Comment: https://git.openjdk.org/jdk/pull/21737#issuecomment-2442940697
Re: RFR: 8343019: Primitive caches must use boxed instances from the archive
On Mon, 28 Oct 2024 10:16:40 GMT, Aleksey Shipilev wrote:
> This is forked from
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) and filed as a
> general issue for archived boxed Integer cache when it's recreated at
> runtime. In short, current code drops the entire primitive cache when the CDS
> archived version of the cache is too short. This poses a problem with code
> that uses CDS archived cache instances, since the boxed equality would break
> when comparing the CDS-archived value and the IntegerCache value recreated at
> runtime.
>
> Ioi suggested a fix here:
> https://github.com/openjdk/jdk/pull/21672#issuecomment-2434359711. I
> separately arrived to the same idea. This PR implements it. `IntegerCache`
> gets the special treatment, because it is the only cache that can be tuned.
> Other caches just prevent the use of bad archived cache (which I think should
> realistically never happen) without re-creating it.
>
> Unfortunately, I was unable to create a standalone CDS test for this: I
> somehow need to be able to put my own archived `Integer` instance into the
> archive, and I don't clearly see how. I tested this patch with original
> reproducer from [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) --
> and it starts to pass. I don't think we would be able to convert that
> reproducer into a unit test, since
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) itself would make
> the reproducer ineffective.
>
> Additional testing:
> - [x] macos-aarch64-server-fastdebug,
> [JDK-8342642](https://bugs.openjdk.org/browse/JDK-8342642) reproducer now
> passes
> - [x] linux-aarch64-server-fastdebug, `all`
src/java.base/share/classes/java/lang/Byte.java line 128:
> 126: }
> 127: archivedCache = c;
> 128: } else if (archivedCache.length != size) {
The `else` case for the non-int boxed type should never happen. Add `assert
archivedCache.length == size;` in the if case instead?
src/java.base/share/classes/java/lang/Integer.java line 964:
> 962: Integer[] c = new Integer[size];
> 963: int j = low;
> 964: // Use all cached values from the archive to avoid
> breaking
Alternatively, you could use a separate array to store **only** the runtime
created cached instances, without copying the references from the archived
cache. Both the archived cache and runtime added cache are used. That has
slightly less memory and efficiency overhead, but almost unnoticeable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21737#discussion_r1819852002
PR Review Comment: https://git.openjdk.org/jdk/pull/21737#discussion_r1819857977
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Mon, 28 Oct 2024 10:54:29 GMT, Alan Bateman wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adjust comment. > > src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java line 58: > >> 56: >> 57: /** >> 58: * Map from module to a class loader name. The name is resolved >> to the > > Would you mind adjusting this to "Map from module name to class loader name"? > Otherwise switching this to using a string is okay. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/21722#discussion_r1819407836
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
> Moved from > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. > > Existing comment threads from closed > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Adjust comment. - Changes: - all: https://git.openjdk.org/jdk/pull/21722/files - new: https://git.openjdk.org/jdk/pull/21722/files/6ecc2151..3234aa23 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21722&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21722&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/21722.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21722/head:pull/21722 PR: https://git.openjdk.org/jdk/pull/21722
RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
Moved from https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: Please review the fix that uses String type for the mapped value in ModuleLoaderMap.Mapper map (Map). Please see details in https://bugs.openjdk.org/browse/JDK-8342642, thanks. Existing comment threads from closed https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 - Commit messages: - - Reflect review feedback: - Use String instances as the loader indexes in ModuleLoaderMap.Mapper map. Changes: https://git.openjdk.org/jdk/pull/21722/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21722&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342642 Stats: 12 lines in 1 file changed: 0 ins; 1 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/21722.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21722/head:pull/21722 PR: https://git.openjdk.org/jdk/pull/21722
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
> Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. Jiangli Zhou has updated the pull request incrementally with 109 additional commits since the last revision: - Reflect review feedback: - Rename [APP|PLATFORM]_LOADER_INDEX to [APP|PLATFORM]_LOADER_NAME. - Update loader map index related comments. - Change to use '.equals()' (from '==') when comparing loader map values. Runs on Debian 6.9.10 With '==' Performance counter stats for 'images/jdk/bin/java -cp /.../home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): 133.08 msec task-clock:u #1.181 CPUs utilized ( +- 0.29% ) 0 context-switches:u #0.000 /sec 0 cpu-migrations:u #0.000 /sec 5,940 page-faults:u# 44.636 K/sec ( +- 0.02% ) 139,151,067 cycles:u #1.046 GHz ( +- 0.24% ) 158,618,665 instructions:u #1.14 insn per cycle ( +- 0.00% ) 27,343,220 branches:u # 205.471 M/sec ( +- 0.00% ) 746,103 branch-misses:u #2.73% of all branches ( +- 0.63% ) 0.112697 +- 0.000386 seconds time elapsed ( +- 0.34% ). With '.equals()': Performance counter stats for 'images/jdk/bin/java -cp /usr/local/google/home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): 133.26 msec task-clock:u #1.186 CPUs utilized ( +- 0.34% ) 0 context-switches:u #0.000 /sec 0 cpu-migrations:u #0.000 /sec 5,941 page-faults:u# 44.581 K/sec ( +- 0.02% ) 141,082,552 cycles:u #1.059 GHz ( +- 0.25% ) 158,619,823 instructions:u #1.12 insn per cycle ( +- 0.00% ) 27,343,260 branches:u # 205.184 M/sec ( +- 0.00% ) 743,257 branch-misses:u #2.72% of all branches ( +- 0.68% ) 0.112399 +- 0.000505 seconds time elapsed ( +- 0.45% ) - 8343086: [BACKOUT] JDK-8295269 G1: Improve slow startup due to predictor initialization Reviewed-by: sangheki - 8339289: Enhance Attach API to support arbitrary length arguments - Windows Reviewed-by: kevinw, sspitsyn - 8342934: TYPE_USE annotations printed with error causing "," in toString output Reviewed-by: iris, vromero - 8338426: Test java/nio/channels/Selector/WakeupNow.java failed Reviewed-by: jpai, alanb - 8295269: G1: Improve slow startup due to predictor initialization Reviewed-by: iwalulya, sjohanss - 8342930: New tests from JDK-8335912 are failing Reviewed-by: jpai - 8343063: RISC-V: remove redundant reg copy in generate_resolve_blob Reviewed-by: fyang, rehn - 8343060: RISC-V: enable TestFloat16VectorConvChain for riscv Reviewed-by: fyang - 8342953: RISC-V: Fix definition of RISCV_HWPROBE_EXT_ZVFHMIN Reviewed-by: mli, rehn - ... and 99 more: https://git.openjdk.org/jdk/compare/ade90fb5...94f35861 - Changes: - all: https://git.openjdk.org/jdk/pull/21672/files - new: https://git.openjdk.org/jdk/pull/21672/files/ade90fb5..94f35861 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=21672&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=21672&range=00-01 Stats: 32526 lines in 383 files changed: 11133 ins; 19908 del; 1485 mod Patch: https://git.openjdk.org/jdk/pull/21672.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21672/head:pull/21672 PR: https://git.openjdk.org/jdk/pull/21672
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Fri, 25 Oct 2024 21:25:54 GMT, Jiangli Zhou wrote: > Moved from > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. > > Existing comment threads from closed > https://github.com/openjdk/jdk/pull/21672/commits/ade90fb51d1e3652910a3b46775522b730306e16: > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436358591 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437465648 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2438567642 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2436262114 > - https://github.com/openjdk/jdk/pull/21672#issuecomment-2437501231 Follow up of the `==` and `.equals()` discussion: I ran a comparison using HelloWorld on my Linux machine. The difference looks noise. - With '==' Performance counter stats for 'images/jdk/bin/java -cp /.../home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): 133.08 msec task-clock:u #1.181 CPUs utilized ( +- 0.29% ) 0 context-switches:u #0.000 /sec 0 cpu-migrations:u #0.000 /sec 5,940 page-faults:u# 44.636 K/sec ( +- 0.02% ) 139,151,067 cycles:u #1.046 GHz ( +- 0.24% ) 158,618,665 instructions:u #1.14 insn per cycle ( +- 0.00% ) 27,343,220 branches:u # 205.471 M/sec ( +- 0.00% ) 746,103 branch-misses:u #2.73% of all branches ( +- 0.63% ) 0.112697 +- 0.000386 seconds time elapsed ( +- 0.34% ). - With '.equals()': Performance counter stats for 'images/jdk/bin/java -cp /usr/local/google/home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): 133.26 msec task-clock:u #1.186 CPUs utilized ( +- 0.34% ) 0 context-switches:u #0.000 /sec 0 cpu-migrations:u #0.000 /sec 5,941 page-faults:u# 44.581 K/sec ( +- 0.02% ) 141,082,552 cycles:u #1.059 GHz ( +- 0.25% ) 158,619,823 instructions:u #1.12 insn per cycle ( +- 0.00% ) 27,343,260 branches:u # 205.184 M/sec ( +- 0.00% ) 743,257 branch-misses:u #2.72% of all branches ( +- 0.68% ) 0.112399 +- 0.000505 seconds time elapsed ( +- 0.45% ) - PR Comment: https://git.openjdk.org/jdk/pull/21722#issuecomment-2438908920
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Fri, 25 Oct 2024 20:53:09 GMT, Jiangli Zhou wrote: > There is an issue from my last push. It includes unintended merged changes > from head. It's probably because I did a cherrypick for my fix when creating > the initial PR. Let me close this PR and create a new one. I'll link all > existing comment threads from this PR to the new PR. I moved the change to a clean branch and created https://github.com/openjdk/jdk/pull/21722. - PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2438904352
Withdrawn: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Wed, 23 Oct 2024 22:14:13 GMT, Jiangli Zhou wrote: > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/21672
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper [v2]
On Fri, 25 Oct 2024 20:11:36 GMT, Jiangli Zhou wrote: >> Please review the fix that uses String type for the mapped value in >> ModuleLoaderMap.Mapper map (Map). Please see details in >> https://bugs.openjdk.org/browse/JDK-8342642, thanks. > > Jiangli Zhou has updated the pull request incrementally with 109 additional > commits since the last revision: > > - Reflect review feedback: >- Rename [APP|PLATFORM]_LOADER_INDEX to [APP|PLATFORM]_LOADER_NAME. > >- Update loader map index related comments. > >- Change to use '.equals()' (from '==') when comparing loader map values. > >Runs on Debian 6.9.10 > >With '==' > Performance counter stats for 'images/jdk/bin/java -cp > /.../home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): > >133.08 msec task-clock:u #1.181 CPUs > utilized ( +- 0.29% ) > 0 context-switches:u #0.000 /sec > 0 cpu-migrations:u #0.000 /sec > 5,940 page-faults:u# 44.636 K/sec > ( +- 0.02% ) > 139,151,067 cycles:u #1.046 GHz > ( +- 0.24% ) > 158,618,665 instructions:u #1.14 insn > per cycle ( +- 0.00% ) >27,343,220 branches:u # 205.471 M/sec > ( +- 0.00% ) > 746,103 branch-misses:u #2.73% of all > branches ( +- 0.63% ) > > 0.112697 +- 0.000386 seconds time elapsed ( +- 0.34% ). > >With '.equals()': > Performance counter stats for 'images/jdk/bin/java -cp > /usr/local/google/home/jianglizhou/tests/hw.jar HelloWorld' (100 runs): > >133.26 msec task-clock:u #1.186 CPUs > utilized ( +- 0.34% ) > 0 context-switches:u #0.000 /sec > 0 cpu-migrations:u #0.000 /sec > 5,941 page-faults:u# 44.581 K/sec > ( +- 0.02% ) > 141,082,552 cycles:u #1.059 GHz > ( +- 0.25% ) > 158,619,823 instructions:u #1.12 insn > per cycle ( +- 0.00% ) >27,343,260 branches:u # 205.184 M/sec > ( +- 0.00% ) > 743,257 branch-misses:u #2.72% of all > branches ( +- 0.68% ) > > 0.112399 +- 0.000505 seconds time elapsed ( +- 0.45% ) > - 8343086: [BACKOUT] ... There is an issue from my last push. It includes unintended merged changes from head. It's probably because I did a cherrypick for my fix when creating the initial PR. Let me close this PR and create a new one. I'll link all existing comment threads from this PR to the new PR. - PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2438788544
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Fri, 25 Oct 2024 10:45:00 GMT, Aleksey Shipilev wrote: > > In that sense, I think we don't have a risk of potentially with > > `used_Integer > IntegerCache.high`. The idea described in Ioi's comment > > above (also brought up by Aleksey Shipilev separately during premain > > meeting) could be sufficient. > > Phew, thanks! I thought I was misunderstanding some fundamental thing here :) > I think `IntegerCache` interaction with CDS archive deserves a fix > regardless. Have you filed the bug for it, @jianglizhou? AFAICS, if we fix > `IntegerCache` <-> CDS interaction, we solve this particular problem as well. I filed https://bugs.openjdk.org/browse/JDK-8343019. I agree with what John said during the meeting yesterday, let's move away from cached Integer in this case (ModuleLoaderMap). > > I am still non-committal about this special fix. We can still do it, but then > this patch effectively changes relying on boxing identity behavior over > `Integer`s to relying on interning behavior over `Strings`, right? If we want > to be 100% safe, shouldn't `==` checks be rewritten to `equals`? And when we > do so, would that affect startup in any meaningful way? Right, it's based on archived interned String behavior. We don't disable any of the archived interned Strings at runtime. So I think we are indeed safe. In an offline chat with @cushon yesterday, Liam also asked about changing `==`. My sense was that there were probably performance considerations when the change was originally made and `==` was chosen (instead of `.equals()`) for performance reason. I think the performance difference mostly would be within noise level. Seems okay to change `.equals()`. I can do some measurements to compare. > > The names of the fields should probably be changed from `_INDEX` to something > else, like `_NAME`? Ok. - PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2438567642
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Wed, 23 Oct 2024 22:14:13 GMT, Jiangli Zhou wrote: > Please review the fix that uses String type for the mapped value in > ModuleLoaderMap.Mapper map (Map). Please see details in > https://bugs.openjdk.org/browse/JDK-8342642, thanks. I brought up this issue in today's Leyden/premain meeting. There was a good discussion on the specific issue, and the general Integer cache archiving and identity hash problem. The conclusion was it's better to avoid using boxed Integer and go with String in ModuleLoaderMap.Mapper. - PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2436262114
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Thu, 24 Oct 2024 05:51:38 GMT, Ioi Lam wrote:
>> @iklam Would it be possible to comment on this? If the system properties to
>> configure the range of integer cache are set at runtime, does it just not
>> use this archived object graph? I'm wondering if it should disable CDS?
>
>> @iklam Would it be possible to comment on this? If the system properties to
>> configure the range of integer cache are set at runtime, does it just not
>> use this archived object graph? I'm wondering if it should disable CDS?
>
> A better fix would be:
>
> If runtime java.lang.Integer.IntegerCache.high < dumptime
> java.lang.Integer.IntegerCache.high
> -- copy the initial elements from the dumptime cache array
> -- fill the rest of the cache array
>
> that way, we will still preserve the object identity of the Integers created
> during dump time.
>
> I think we need to do the same thing for the other box cases.
> > > @iklam Would it be possible to comment on this? If the system properties
> > > to configure the range of integer cache are set at runtime, does it just
> > > not use this archived object graph? I'm wondering if it should disable
> > > CDS?
> >
> >
> > A better fix would be:
> > If runtime java.lang.Integer.IntegerCache.high < dumptime
> > java.lang.Integer.IntegerCache.high -- copy the initial elements from the
> > dumptime cache array -- fill the rest of the cache array
> > that way, we will still preserve the object identity of the Integers
> > created during dump time.
> > I think we need to do the same thing for the other box cases.
>
> I've been thinking about something like that as a longer term fix after this
> quick fix. However, there are some complications, e.g.:
>
> When an archived boxed Integer instance is 'used' in a pre-initialized class
> static field's sub-graph, depending on the runtime IntegerCache.high
> specified by the system property, the specific 'used' Integer may not be
> within the runtime range (used_Integer > IntegerCache.high). In that case,
> the runtime modified cache does not contain the archived boxed Integer
> instance. Then, the usage of the archived instance is invalid, which causes
> the corresponding pre-initialized static field to still be problematic. For
> this loader map, in an extreme example (unlikely to happen) when runtime
> IntegerCache.high=1, it would still not work well. For such cases, we would
> need to invalidate the pre-initialized static field or class. That's the
> reason I mentioned filing a separate bug for the archived Integer cache and
> evaluating together with the general AOT cache work, see
> https://bugs.openjdk.org/browse/JDK-8342642?focusedId=14714939&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acommen
t-tabpanel#comment-14714939.
I just re-looked at the
[IntegerCache](https://github.com/openjdk/jdk/blob/d1540e2a49c7a41eb771fc9896c367187d070dec/src/java.base/share/classes/java/lang/Integer.java#L961)
code, I need to take the above back. Unlike I remembered, IntegerCache doesn't
recreate the cache if runtime cache length () is **less** than the archived
cache:
if (archivedCache == null || size > archivedCache.length) {
// re-create
}
In that sense, I think we don't have a risk of potentially with `used_Integer >
IntegerCache.high`. The idea described in Ioi's comment above (also brought up
by Aleksey Shipilev separately during premain meeting) could be sufficient.
Anyway moving away from archived boxed Integer and identity hash comparison in
this case is safer.
-
PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2436358591
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Tue, 15 Oct 2024 20:22:52 GMT, Magnus Ihse Bursie wrote: >> As a prerequisite for Hermetic Java, we need a statically linked `java` >> launcher. It should behave like the normal, dynamically linked `java` >> launcher, except that all JDK native libraries should be statically, not >> dynamically, linked. >> >> This patch is the first step towards this goal. It will generate a >> `static-jdk` image with a statically linked launcher. This launcher is >> missing several native libs, however, and does therefore not behave like a >> proper dynamic java. One of the reasons for this is that local symbol hiding >> in static libraries are not implemented yet, which causes symbol clashes >> when linking all static libraries together. This will be addressed in an >> upcoming patch. >> >> All changes in the `src` directory are copied from, or inspired by, changes >> made in [the hermetic-java-runtime branch in Project >> Leyden](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: > > Don't hardcode server variant > > > When trying to sort out the LDFLAGS issues, it turned out that I could > > > not run the linux launcher at all, not even when checking out older > > > commits of this PR. I am almost at a loss here; I assume that this worked > > > when I created this PR (even though I only did ad hoc testing at the > > > time), and I'm not sure if my testing then was inadequate or if something > > > else has changed during that time with my environment. > > > I'm trying to retrace my steps in how I got to this branch, but I have > > > unfortunately deleted much of the intermediate steps. > > > @jianglizhou Can you check if you can build and run a simple > > > HelloWorld.java with the static launcher in this PR on linux? > > > `images/static-jdk/bin/java --version` works for me, but not > > > `images/static-jdk/bin/java HelloWorld.java`, which fails with an error > > > that indicates it cannot locate the jimage library. > > > > > > @magicus I just noticed your above notes now (I'm been chasing an unrelated > > long G1 Evacuation Pause issue). I'll test your changes tomorrow. Will > > update you in the thread. > > Sorry for the delay. I finally found some time to look into this today. > Running the latest `static-jdk` `bin/java` built from your branch fails for > me as well: > > ``` > $ bin/java > Error: failed , because bin/java: undefined symbol: JNI_CreateJavaVM > ``` > > I only took a quick look, but I think this looks like > https://bugs.openjdk.org/browse/JDK-8339522 that I filed in September. > > ``` > $ nm bin/java | grep JNI_CreateJavaVM > 0162733c t JNI_CreateJavaVM > 01626e75 t _ZL22JNI_CreateJavaVM_innerPP7JavaVM_PPvS2_ > jianglizhou@leia:~/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk$ > nm -D bin/java | grep JNI_CreateJavaVM > ``` > > The VM dynamic symbols are not exported properly. This is related to the > [#20837 > (comment)](https://github.com/openjdk/jdk/pull/20837#discussion_r1744611776) > discussion comment thread. Checking the `ld` command line, I think it is indeed https://bugs.openjdk.org/browse/JDK-8339522 issue. `-Wl,--exclude-libs,ALL` is included in the command: $ cat ./support/static-native/launcher/BUILD_LAUNCHER_java_run_ld.cmdline /usr/bin/gcc -Wl,-z,defs -Wl,-z,relro -Wl,-z,now -Wl,--no-as-needed -Wl,-z,noexecstack -m64 -pie -Wl,-z,defs -Wl,-z,relro -Wl,-z,now -Wl,--no-as-needed -Wl,--exclude-libs,ALL -Wl,-z,noexecstack -m64 -Wl,-rpath,$ORIGIN -Wl,--disable-new-dtags -Wl,-rpath,$ORIGIN/../lib -Wl,--disable-new-dtags -o /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/static-native/launcher/java /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/static-native/launcher/main.o -Wl,--export-dynamic -Wl,--whole-archive /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libverify/static/libverify.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libjava/static/libjava.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libzip/static/libzip.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-sl owdebug/support/native/java.base/libjimage/static/libjimage.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libjli/static/libjli.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libnet/static/libnet.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libnio/static/libnio.a /...jianglizhou/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/support/native/java.base/libjsig/static/l
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Wed, 23 Oct 2024 01:40:59 GMT, Jiangli Zhou wrote: > > When trying to sort out the LDFLAGS issues, it turned out that I could not > > run the linux launcher at all, not even when checking out older commits of > > this PR. I am almost at a loss here; I assume that this worked when I > > created this PR (even though I only did ad hoc testing at the time), and > > I'm not sure if my testing then was inadequate or if something else has > > changed during that time with my environment. > > I'm trying to retrace my steps in how I got to this branch, but I have > > unfortunately deleted much of the intermediate steps. > > @jianglizhou Can you check if you can build and run a simple > > HelloWorld.java with the static launcher in this PR on linux? > > `images/static-jdk/bin/java --version` works for me, but not > > `images/static-jdk/bin/java HelloWorld.java`, which fails with an error > > that indicates it cannot locate the jimage library. > > @magicus I just noticed your above notes now (I'm been chasing an unrelated > long G1 Evacuation Pause issue). I'll test your changes tomorrow. Will update > you in the thread. Sorry for the delay. I finally found some time to look into this today. Running the latest `static-jdk` `bin/java` built from your branch fails for me as well: $ bin/java Error: failed , because bin/java: undefined symbol: JNI_CreateJavaVM I only took a quick look, but I think this looks like https://bugs.openjdk.org/browse/JDK-8339522 that I filed in September. $ nm bin/java | grep JNI_CreateJavaVM 0162733c t JNI_CreateJavaVM 01626e75 t _ZL22JNI_CreateJavaVM_innerPP7JavaVM_PPvS2_ jianglizhou@leia:~/github/jdk_pull_20837/build/linux-x86_64-server-slowdebug/images/static-jdk$ nm -D bin/java | grep JNI_CreateJavaVM The VM dynamic symbols are not exported properly. This is related to the https://github.com/openjdk/jdk/pull/20837#discussion_r1744611776 discussion comment thread. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2436496612
Re: RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
On Thu, 24 Oct 2024 05:51:38 GMT, Ioi Lam wrote: > > @iklam Would it be possible to comment on this? If the system properties to > > configure the range of integer cache are set at runtime, does it just not > > use this archived object graph? I'm wondering if it should disable CDS? > > A better fix would be: > > If runtime java.lang.Integer.IntegerCache.high < dumptime > java.lang.Integer.IntegerCache.high -- copy the initial elements from the > dumptime cache array -- fill the rest of the cache array > > that way, we will still preserve the object identity of the Integers created > during dump time. > > I think we need to do the same thing for the other box cases. I've been thinking about something like that as a longer term fix after this quick fix. However, there are some complications, e.g.: When an archived boxed Integer instance is 'used' in a pre-initialized class static field's sub-graph, depending on the runtime IntegerCache.high specified by the system property, the specific 'used' Integer may not be within the runtime range (used_Integer > IntegerCache.high). In that case, the runtime modified cache does not contain the archived boxed Integer instance. Then, the usage of the archived instance is invalid, which causes the corresponding pre-initialized static field to still be problematic. For this loader map, in an extreme example (unlikely to happen) when runtime IntegerCache.high=1, it would still not work well. For such cases, we would need to invalidate the pre-initialized static field or class. That's the reason I mentioned filing a separate bug for the archived Integer cache and evaluating together with the general AOT cache work, see https://bugs.openjdk.org/browse/JDK-8342642?focusedId=14714939&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment- tabpanel#comment-14714939. - PR Comment: https://git.openjdk.org/jdk/pull/21672#issuecomment-2435608321
RFR: 8342642: Class loading failure due to archived map issue in ModuleLoaderMap.Mapper
Please review the fix that uses String type for the mapped value in ModuleLoaderMap.Mapper map (Map). Please see details in https://bugs.openjdk.org/browse/JDK-8342642, thanks. - Commit messages: - Use String instances as the loader indexes in ModuleLoaderMap.Mapper map. Changes: https://git.openjdk.org/jdk/pull/21672/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=21672&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8342642 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/21672.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/21672/head:pull/21672 PR: https://git.openjdk.org/jdk/pull/21672
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v7]
On Mon, 21 Oct 2024 13:17:27 GMT, Magnus Ihse Bursie wrote: > When trying to sort out the LDFLAGS issues, it turned out that I could not > run the linux launcher at all, not even when checking out older commits of > this PR. I am almost at a loss here; I assume that this worked when I created > this PR (even though I only did ad hoc testing at the time), and I'm not sure > if my testing then was inadequate or if something else has changed during > that time with my environment. > > I'm trying to retrace my steps in how I got to this branch, but I have > unfortunately deleted much of the intermediate steps. > > @jianglizhou Can you check if you can build and run a simple HelloWorld.java > with the static launcher in this PR on linux? `images/static-jdk/bin/java > --version` works for me, but not `images/static-jdk/bin/java > HelloWorld.java`, which fails with an error that indicates it cannot locate > the jimage library. @magicus I just noticed your above notes now (I'm been chasing an unrelated long G1 Evacuation Pause issue). I'll test your changes tomorrow. Will update you in the thread. - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2430624032
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher [v5]
On Tue, 15 Oct 2024 18:50:27 GMT, Magnus Ihse Bursie wrote: > After thinking a bit more on this, I concluded that we cannot automatically > extract a proper set of ld flags from what's being passed to the individual > libraries. The LDFLAGS needed by the monolithic static library needs to be > explicitly defined. Unfortunately, most of it will be a copy of what is > already duplicated across JVM_LDFLAGS, LDFLAGS_JDKLIB and LDFLAGS_JDKEXE. :-( > But cleaning that mess upp requires a separate PR. @magicus, just to make it clear, do you plan to explicitly define the set of LDFLAGS for static linking as part of this PR? We need to make sure the JVM_LDFLAGS is properly included initially. - PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1801767849
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Thu, 5 Sep 2024 09:57:15 GMT, Magnus Ihse Bursie wrote: >> make/modules/java.desktop/lib/AwtLibraries.gmk line 176: >> >>> 174: >>> 175: ifneq ($(ENABLE_HEADLESS_ONLY), true) >>> 176: # We cannot link with both awt_headless and awt_xawt at the same >>> time >> >> Just a note on that. It's doable to link with both awt_headless and awt_xawt >> with some work. I did some quick experiments on that during the initial >> investigation for hermetic/static Java. > > That would require quite some work then..! The two libraries are meant as > exclusive complements to each other -- they both implement the same "entry > points", but in different ways -- one with X11 support, and one without. For > other reasons (outside of static launcher reasons) I'd like to see some > refactoring in how this is implemented, but that is completely outside this > discussion. > > For the static launcher scenario, I can't even see the point of trying to > include both? What would you accomplish by that? > > The entire point of having two libraries is that you want to be able to have > full workstation capabilities, but then be dependent on the X11 libraries, or > have limited capabilities, but skip the X11 dependency. My initial understanding was that the libawt_headless was mostly as subset of libawt_xawt, which made it possible to statically link both the headless and headful natives. Completely agree that it's outside of the current scope. - PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1746136942
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Thu, 5 Sep 2024 10:03:19 GMT, Magnus Ihse Bursie wrote: >> make/StaticLibs.gmk line 118: >> >>> 116: OPTIMIZATION := HIGH, \ >>> 117: STATIC_LAUNCHER := true, \ >>> 118: LDFLAGS := $(JAVASTATIC_LINK_LDFLAGS), \ >> >> I could be missing something, but I don't see where is >> $JAVASTATIC_LINK_LDFLAGS defined. >> >> On a related notes, I think we need to include $JVM_LDFLAGS when linking the >> static "java". See >> https://bugs.openjdk.org/browse/JDK-8339522?focusedId=14702923&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14702923. > > You are right, this is dead code. Thanks for spotting this. > > During my experimentation, I tried passing along LDFLAGS from the individual > libraries as well, but it turned out not to be a good idea -- the way we have > used them were to modify some special properties on a single dynamic library, > which did not apply to the static library as a whole. > > However, there is a risk that we in the future need to add LDFLAGS to a > library that also needs to be carried over to the static launcher. If this > happens, I guess we need to separate between LDFLAGS_ONLY_FOR_THIS_DLL and > LDFLAGS_ALSO_FOR_STATIC_LINKING. +1 on "separate between LDFLAGS_ONLY_FOR_THIS_DLL and LDFLAGS_ALSO_FOR_STATIC_LINKING" I think we need to get the linker flags sorted out correctly in this initial PR and make sure the needed flags (most importantly the ones used in $JVM_LDFLAGS). - PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1746133505
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Thu, 5 Sep 2024 05:06:55 GMT, Julian Waters wrote: >> make/StaticLibs.gmk line 71: >> >>> 69: # libsspi_bridge has name conflicts with sunmscapi >>> 70: BROKEN_STATIC_LIBS += sspi_bridge >>> 71: # These libs define DllMain which conflict with Hotspot >> >> I'm not aware of the DllMain issue with static linking these libs. Could you >> please explain? The libawt.a and libdt_socket.a are statically linked with >> `javastatic` in >> https://github.com/openjdk/leyden/tree/hermetic-java-runtime/ branch. > > DllMain is a Windows specific initialization method that is called when a > Windows dll (Dynamic library) is loaded, among other things. Since DllMain is > extern "C", it is not mangled and hence likely that having multiple static > libraries that each define it will cause multiple symbol definition errors > during linking. It might be that the reason hermetic Java hasn't encountered > this problem yet is because it mainly tests its code on Linux, while this is > a Windows specific issue, since the names you mention (libawt.a and > libdt_socket.a) are the names of those libraries on Linux, not Windows. > However, the issue likely deeper than that. DllMain is completely wrong to > define when inside a static library, and should not be compiled at all when > making the static versions of these libraries. Simply localizing the DllMain > symbol when creating a static library would be wrong. We'll have to find out > how to run the initialization code for each of these currently dynamic > libraries without DllMai n when compiling them as static libraries @TheShermanTanker thanks for the details on DllMain issue. Right, we have only tested hermetic/static Java on Linux. I agree that hiding DllMain is not the right approach. One possible solution is to put the DllMain in separate .c/.cpp files and only link with those when building the .dll. With the current PR, we mainly focuses on the Linux (or unix-like) port, e.g. `os::lookup_function()` is not supported in the Windows port yet. Any thoughts on if we only limit static linking these affected JDK libraries on Windows initially, and allow statically linking more libs on Linux? We can add those libs on Windows when we resolve the DllMain issue. For running some minimum jtreg testing initially, I think we would want to include `libnet` (and other libs) in the statically linked `java` binary. - PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1746129101
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Thu, 5 Sep 2024 09:50:49 GMT, Magnus Ihse Bursie wrote: > Well, but your proof-of-concept only supports clang on linux, where you have > enabled symbol hiding. The hermetic-java-runtime branch doesn't have general symbol hiding enabled. That's why I'm wondering what the issues are with these libs except for `libjawt` with the current PR. (A side-note on `libjawt.a`: For static linking, we don't need `libjawt.a` and the headless or headful libs can be directly statically linked with.) > > Our conclusion in the zoom talks was that we should strive for getting a > static launcher build pushed into mainline before we have full and proper > support for symbol hiding on all platforms. Right, that would be a good & practical way to add the support in the mainline. - PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1746051842
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Tue, 3 Sep 2024 12:51:13 GMT, Magnus Ihse Bursie wrote: > @jianglizhou Can you please check if there are any other contributors that > should be acknowledged? Thanks for asking! I checked all the related changes in https://github.com/openjdk/leyden/tree/hermetic-java-runtime branch. Following are the related commits from the branch corresponding to the extracted changes included in this PR. There are no other author for those changes. However, all changes have go though prior reviews by my teammates. I didn't ask it for the earlier integration PR, is there way to mention the reviewer contributions? - https://github.com/openjdk/leyden/commit/bceb753f79b4bc767fdfb71d5f68a84430644df6 - https://github.com/openjdk/leyden/commit/a998a9d6ca44a93d4e9859a17de2dca60963de76 - https://github.com/openjdk/leyden/commit/53aa8f0cf418ab5f435a4b9996c7754fb8505d4b - https://github.com/openjdk/leyden/commit/63f84f5c0a98077c8f49a19f026f103b9e29d6e0 - https://github.com/openjdk/leyden/commit/afe9ca06dd86e8983768de80ba1a08f3c68589b4 - https://github.com/openjdk/leyden/commit/7d75a7f4d6aa020b7580fbbf660b2b3e3a41b27 - PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2330368791
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Tue, 3 Sep 2024 12:50:01 GMT, Magnus Ihse Bursie wrote:
> As a prerequisite for Hermetic Java, we need a statically linked `java`
> launcher. It should behave like the normal, dynamically linked `java`
> launcher, except that all JDK native libraries should be statically, not
> dynamically, linked.
>
> This patch is the first step towards this goal. It will generate a
> `static-jdk` image with a statically linked launcher. This launcher is
> missing several native libs, however, and does therefore not behave like a
> proper dynamic java. One of the reasons for this is that local symbol hiding
> in static libraries are not implemented yet, which causes symbol clashes when
> linking all static libraries together. This will be addressed in an upcoming
> patch.
>
> All changes in the `src` directory are copied from, or inspired by, changes
> made in [the hermetic-java-runtime branch in Project
> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
src/java.base/unix/native/libjli/java_md.c line 509:
> 507:
> 508: if (GetApplicationHome(path, pathsize)) {
> 509: if (JLI_IsStaticallyLinked()) {
`GetJREPath()` does not need to be called for the static case. Any reason why
this path is executed for static mode?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744642315
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Tue, 3 Sep 2024 12:50:01 GMT, Magnus Ihse Bursie wrote:
> As a prerequisite for Hermetic Java, we need a statically linked `java`
> launcher. It should behave like the normal, dynamically linked `java`
> launcher, except that all JDK native libraries should be statically, not
> dynamically, linked.
>
> This patch is the first step towards this goal. It will generate a
> `static-jdk` image with a statically linked launcher. This launcher is
> missing several native libs, however, and does therefore not behave like a
> proper dynamic java. One of the reasons for this is that local symbol hiding
> in static libraries are not implemented yet, which causes symbol clashes when
> linking all static libraries together. This will be addressed in an upcoming
> patch.
>
> All changes in the `src` directory are copied from, or inspired by, changes
> made in [the hermetic-java-runtime branch in Project
> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
src/hotspot/share/classfile/classLoader.cpp line 953:
> 951: assert(CanonicalizeEntry == nullptr, "should not load java library
> twice");
> 952: if (is_vm_statically_linked()) {
> 953: CanonicalizeEntry = CAST_TO_FN_PTR(canonicalize_fn_t,
> os::lookup_function("JDK_Canonicalize"));
Can you add an assert to make sure `CanonicalizeEntry` is not NULL?
src/hotspot/share/classfile/classLoader.cpp line 969:
> 967:
> 968: if (is_vm_statically_linked()) {
> 969: JImageOpen = CAST_TO_FN_PTR(JImageOpen_t,
> os::lookup_function("JIMAGE_Open"));
It might be good to assert these are not NULL as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744635408
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744635916
Re: RFR: 8339480: Build static-jdk image with a statically linked launcher
On Tue, 3 Sep 2024 12:50:01 GMT, Magnus Ihse Bursie wrote:
> As a prerequisite for Hermetic Java, we need a statically linked `java`
> launcher. It should behave like the normal, dynamically linked `java`
> launcher, except that all JDK native libraries should be statically, not
> dynamically, linked.
>
> This patch is the first step towards this goal. It will generate a
> `static-jdk` image with a statically linked launcher. This launcher is
> missing several native libs, however, and does therefore not behave like a
> proper dynamic java. One of the reasons for this is that local symbol hiding
> in static libraries are not implemented yet, which causes symbol clashes when
> linking all static libraries together. This will be addressed in an upcoming
> patch.
>
> All changes in the `src` directory are copied from, or inspired by, changes
> made in [the hermetic-java-runtime branch in Project
> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
make/StaticLibs.gmk line 1:
> 1: #
Perhaps also consider adopting StaticLink.gmk file name from the
https://github.com/openjdk/leyden/tree/hermetic-java-runtime/ branch, as we are
mostly doing the static linking here. Creating the static libs is handled
elsewhere.
make/StaticLibs.gmk line 71:
> 69: # libsspi_bridge has name conflicts with sunmscapi
> 70: BROKEN_STATIC_LIBS += sspi_bridge
> 71: # These libs define DllMain which conflict with Hotspot
I'm not aware of the DllMain issue with static linking these libs. Could you
please explain? The libawt.a and libdt_socket.a are statically linked with
`javastatic` in https://github.com/openjdk/leyden/tree/hermetic-java-runtime/
branch.
make/StaticLibs.gmk line 74:
> 72: BROKEN_STATIC_LIBS += awt dt_shmem dt_socket javaaccessbridge
> 73: # These libs are dependent on any of the above disabled libs
> 74: BROKEN_STATIC_LIBS += fontmanager jawt lcms net nio
Which specific dependent cause these libs being excluded? In
https://github.com/openjdk/leyden/tree/hermetic-java-runtime/ branch, these JDK
libs (except `libjawt.a`) are statically linked into `javastatic`.
make/StaticLibs.gmk line 118:
> 116: OPTIMIZATION := HIGH, \
> 117: STATIC_LAUNCHER := true, \
> 118: LDFLAGS := $(JAVASTATIC_LINK_LDFLAGS), \
I could be missing something, but I don't see where is $JAVASTATIC_LINK_LDFLAGS
defined.
On a related notes, I think we need to include $JVM_LDFLAGS when linking the
static "java". See
https://bugs.openjdk.org/browse/JDK-8339522?focusedId=14702923&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14702923.
make/modules/java.desktop/lib/AwtLibraries.gmk line 176:
> 174:
> 175: ifneq ($(ENABLE_HEADLESS_ONLY), true)
> 176: # We cannot link with both awt_headless and awt_xawt at the same time
Just a note on that. It's doable to link with both awt_headless and awt_xawt
with some work. I did some quick experiments on that during the initial
investigation for hermetic/static Java.
src/java.base/unix/native/libjli/java_md.c line 300:
> 298:char jvmcfg[], jint so_jvmcfg) {
> 299: /* Compute/set the name of the executable. This is needed for macOS.
> */
> 300: SetExecname(*pargv);
Why is `SetExecname()` needed for the static case?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744614583
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744604685
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744603414
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744611776
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744616878
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1744620611
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Thu, 29 Aug 2024 08:26:16 GMT, Magnus Ihse Bursie wrote: > Okay. Unless I misunderstand something, there were no additional authors to > be credited for these two commits. That's correct for these. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2317982354
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Tue, 27 Aug 2024 23:15:03 GMT, Jiangli Zhou wrote: >> And the discussion whether the checks are made "dynamically" or "statically" >> is too simplified to be really helpful. >> >> Currently, we compile two sets of all object files, with slightly different >> compiler arguments, one for dynamic libraries and one for static libraries. >> Files that are doing things differently for these two modes have an #ifdef, >> so the alternative way of doing things are not included in the object file. >> >> In your branch, you still have a separate compilation of all files for >> static builds, but you also try to figure out through various means (which >> involves jumping through some hoops to get the bootstrapping right) if this >> is a static build or a dynamic build. In a way, one could argue that this is >> just worse than the current solution, since you are still recompiling all >> files separately for static libraries so you could "know" at build time if >> you are static or not. >> >> What I am trying to do is to get to a point where we can compile almost all >> files just once, and then have two trivially small files that are compiled >> twice, with just a different value of a define that makes the difference. To >> propagate this information to all other object files, they need to call the >> function provided in this object file. So, is it then a "build time" lookup >> or a "runtime lookup", or a "static lookup" vs "dynamic lookup"? The >> semantics does not really matter. The whole point is that the difference in >> build is reduced to an absolute minimum. Sure, this single "lookup" function >> could be created more like the way you are doing in your branch to try to >> figure this out without the help of the build system, but there is really no >> point in that. This is a simple and elegant solution. > > @magicus please also specify contributor properly to so it's clear part of > the change is based on/extracted from > https://github.com/openjdk/leyden/tree/hermetic-java-runtime. > @jianglizhou Are there any other authors on the `hermetic-java-runtime` > branch that should be credited? For any commits in https://github.com/openjdk/leyden/compare/master...hermetic-java-runtime contributed by other contributor(s) or additional contributor(s), I documented that the commit message (e.g. https://github.com/openjdk/leyden/commit/4faa3a964ec550e410c741048c7e0ed99ac64b52). The current PR is related to the following. Please refer to those commit messages. - https://github.com/openjdk/leyden/commit/7d75a7f4d6aa020b7580fbbf660b2b3e3a41b274 - https://github.com/openjdk/leyden/commit/22d8c439157b61acdfe99090d39f91c09388b1b1 - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2315882065
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Tue, 27 Aug 2024 13:55:51 GMT, Magnus Ihse Bursie wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Also update build to link properly > > And the discussion whether the checks are made "dynamically" or "statically" > is too simplified to be really helpful. > > Currently, we compile two sets of all object files, with slightly different > compiler arguments, one for dynamic libraries and one for static libraries. > Files that are doing things differently for these two modes have an #ifdef, > so the alternative way of doing things are not included in the object file. > > In your branch, you still have a separate compilation of all files for static > builds, but you also try to figure out through various means (which involves > jumping through some hoops to get the bootstrapping right) if this is a > static build or a dynamic build. In a way, one could argue that this is just > worse than the current solution, since you are still recompiling all files > separately for static libraries so you could "know" at build time if you are > static or not. > > What I am trying to do is to get to a point where we can compile almost all > files just once, and then have two trivially small files that are compiled > twice, with just a different value of a define that makes the difference. To > propagate this information to all other object files, they need to call the > function provided in this object file. So, is it then a "build time" lookup > or a "runtime lookup", or a "static lookup" vs "dynamic lookup"? The > semantics does not really matter. The whole point is that the difference in > build is reduced to an absolute minimum. Sure, this single "lookup" function > could be created more like the way you are doing in your branch to try to > figure this out without the help of the build system, but there is really no > point in that. This is a simple and elegant solution. @magicus please also specify contributor properly to so it's clear part of the change is based on/extracted from https://github.com/openjdk/leyden/tree/hermetic-java-runtime. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2313737779
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Tue, 27 Aug 2024 13:55:51 GMT, Magnus Ihse Bursie wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Also update build to link properly > > And the discussion whether the checks are made "dynamically" or "statically" > is too simplified to be really helpful. > > Currently, we compile two sets of all object files, with slightly different > compiler arguments, one for dynamic libraries and one for static libraries. > Files that are doing things differently for these two modes have an #ifdef, > so the alternative way of doing things are not included in the object file. > > In your branch, you still have a separate compilation of all files for static > builds, but you also try to figure out through various means (which involves > jumping through some hoops to get the bootstrapping right) if this is a > static build or a dynamic build. In a way, one could argue that this is just > worse than the current solution, since you are still recompiling all files > separately for static libraries so you could "know" at build time if you are > static or not. > > What I am trying to do is to get to a point where we can compile almost all > files just once, and then have two trivially small files that are compiled > twice, with just a different value of a define that makes the difference. To > propagate this information to all other object files, they need to call the > function provided in this object file. So, is it then a "build time" lookup > or a "runtime lookup", or a "static lookup" vs "dynamic lookup"? The > semantics does not really matter. The whole point is that the difference in > build is reduced to an absolute minimum. Sure, this single "lookup" function > could be created more like the way you are doing in your branch to try to > figure this out without the help of the build system, but there is really no > point in that. This is a simple and elegant solution. We had a zoom discussion with @magicus and others on this PR (as part of regular hermetic Java meeting) this morning. @magicus mentioned that he has a PR in progress with the static linking part, which helps address my specific concern. > In your branch, you still have a separate compilation of all files for static > builds, but you also try to figure out through various means (which involves > jumping through some hoops to get the bootstrapping right) if this is a > static build or a dynamic build. In a way, one could argue that this is just > worse than the current solution, since you are still recompiling all files > separately for static libraries so you could "know" at build time if you are > static or not. Yes, the .o files are recompiled for creating the static libraries currently. That causes observable large overhead in terms of both memory and build duration for building JDK itself. In real world constraint environments, both overhead are problematic and cause build issues. So steps toward building the `.so` and `.a` using the same set of `.o` object files should be one of our end goals (just to re-iterate its importance), but would be "ok" without during the initial phases when we are building/integrating hermetic/static support in JDK mainline. - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2313180693
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly Marked as reviewed by jiangli (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20666#pullrequestreview-2264106008
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Thu, 22 Aug 2024 00:30:07 GMT, Jiangli Zhou wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Also update build to link properly > > I compared the extracted changes in this PR with the related parts in > https://github.com/openjdk/jdk/pull/19478. They look ok. My concern (as > discussed in > https://github.com/openjdk/jdk/pull/19478#issuecomment-2278421931) is that > these runtime changes for static JDK can't be tested even they are relatively > simple, without the the actual linking change. Any timeline for the static > linking changes? > @jianglizhou > > > [...] these runtime changes for static JDK can't be tested [...] > > Yes, they can. This is just a pure refactoring of existing code. I have > deliberately kept out addition of the new places where static linking > exceptions are needed in the code. Hi @magicus, perhaps the answer is both `yes` and `no`. Since your `src/hotspot/share/runtime/linkType.cpp` change removes the needs of requiring `#ifdef STATIC_BUILD` macro from various affected JDK source files to handle the differences between dynamic linking and static linking. From that sense, it's probably `yes` (can be tested as before) as `linkType.cpp` still uses `#ifdef STATIC_BUILD`, and the dynamic v.s. static differences are still determined at build time and not at runtime, as @dholmes-ora and @TheShermanTanker have pointed out. In theory, things (especially the dynamic case) could be tested as before since the fundamental is unchanged. That's different from the changes in https://github.com/openjdk/leyden/tree/hermetic-java-runtime, which does the actual runtime checks. Since the mainline doesn't have the needed build changes to have the ability to link a `javastatic` binary, from that point of view all the `static` cases in the PR cannot be tested yet. We could test them by integrating into https://github.com/openjdk/leyden/tree/hermetic-java-runtime and downstream codebase (with full hermetic Java support) after the PR is approved/submitted in the mainline. That might help. To ease some of @dholmes-ora's concern (and my concern as well) that the initial change could affect all Java instances, perhaps providing the build support for statically linking `javastatic` should be done as an immediate follow-up step (I'm continually nudging you toward that direction :-)). We have multiple goals to achieve in the build system for just the static-Java-only part and we probably want to consider adding the support in following sequence: 1) Capability of building a fully statically linked `javastatic` executable 2) Allow linking both `java` (with dynamic linking support) and `javatatic` using the same set of `.o` object files - Eliminate the needs of `#ifdef STATIC_BUILD` macro. Your `linkType.cpp` change seems to be able to limit the macro usage within one file and just conditionally compile the single file only. So that helps. - May involve spec changes for `JNI_OnLoad` and friends to use `JNI_OnLoad_` naming for dynamic linking support. The needed spec change for the static linking case (built in library support) has already been done in the past by others. 3) General solution for duplicating symbol issue - `objcopy` for symbol hiding - PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2311351447
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly I compared the extracted changes in this PR with the related parts in https://github.com/openjdk/jdk/pull/19478. They look ok. My concern (as discussed in https://github.com/openjdk/jdk/pull/19478#issuecomment-2278421931) is that these runtime changes for static JDK can't be tested even they are relatively simple, without the the actual linking change. Any timeline for the static linking changes? - PR Review: https://git.openjdk.org/jdk/pull/20666#pullrequestreview-2252486767
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 I've looked through all JDK and VM changes and left comments in various places. All the rest changes in PR look good. Thanks again for extracting these changes from the leyden/hermeticJava branch and integrating with mainline! My other main question is why the `javastatic` linking work is not included in the PR together with these runtime changes. IIUC from our meetings and mailing list discussions, the initial integration PR needs to include the part for statically linking the `javastatic`. That's a minimum requirement for testing/verifying the runtime changes when integrating into the mainline, which is also the reason why we haven't starting integrating any of the runtime changes so far. Has that been changed? - PR Review: https://git.openjdk.org/jdk/pull/19478#pullrequestreview-2133328296
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
src/java.base/unix/native/libjli/java_md.c line 316:
> 314: SetExecname(*pargv);
> 315:
> 316: if (!JLI_IsStaticallyLinked()) {
Any reason this is diverted from the change in hermetic Java branch,
https://github.com/openjdk/leyden/blob/c1c5fc686c1452550e1b3663a320fba652248505/src/java.base/unix/native/libjli/java_md.c#L300?
I think the setenv part below is not needed for static/hermetic support
either. There is no $JRE/lib with a single executable image. All natives are
statically linked with the executable image.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1649356484
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 src/java.base/macosx/native/libjli/java_md_macosx.m line 1: > 1: /* In the mailing list email discussion thread on hermetic Java, you mentioned running on macosx with a build from hermtic Java branch crashed for you during startup. Is that fully resolved with the changes in this PR? The hermetic Java branch does not have any changes for macosx port. What tests are done for the macosx port for static support? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1649348777
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
src/hotspot/share/utilities/zipLibrary.cpp line 63:
> 61:
> 62: static void* dll_lookup(const char* name, const char* path, bool
> vm_exit_on_failure) {
> 63: if (vm_is_statically_linked()) {
I like this change. It is cleaner than the hermetic Java branch change that
does the `if` static check in `store_function_pointers`
(https://github.com/openjdk/leyden/blob/c1c5fc686c1452550e1b3663a320fba652248505/src/hotspot/share/utilities/zipLibrary.cpp#L75).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1649341844
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
src/hotspot/share/runtime/os.cpp line 521:
> 519: char ebuf[1024];
> 520:
> 521: if (vm_is_statically_linked()) {
This block can be moved before the two variable declarations above, since they
are not needed in the static case.
https://github.com/openjdk/leyden/blob/c1c5fc686c1452550e1b3663a320fba652248505/src/hotspot/share/runtime/os.cpp#L507
handles it that way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r164982
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 src/hotspot/os/bsd/os_bsd.cpp line 1: > 1: /* The changes in os_bsd.cpp are new and are not from https://github.com/openjdk/leyden/tree/hermetic-java-runtime/. Have you tested the bsd port? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1649312808
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/common/native/Link.gmk line 72: > 70: define CreateStaticLibrary > 71: # Include partial linking when building the static library with clang > on linux > 72: ifeq ($(call isTargetOs, linux macosx), true) Is this mainly for `clang` support for now? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648293391
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/modules/java.desktop/lib/AwtLibraries.gmk line 257: > 255: JDK_LIBS := libawt java.base:libjava, \ > 256: LIBS_unix := $(LIBDL) $(LIBM) $(X_LIBS) -lX11 -lXext -lXi > -lXrender \ > 257: -lXtst, \ Same question as for the STATIC_LIB_EXCLUDE_OBJS change with `libjli`. Are the duplicate symbol issues resolved by symbol hiding? I think it's still better to not include those .o files to avoid unnecessary footprint overhead in the binary. - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648292220
Re: RFR: 8333268: Fixes for static build [v4]
On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie wrote: >> This patch contains a set of changes to improve static builds. They will >> pave the way for implementing a full static-only java launcher. The changes >> here will: >> >> 1) Make sure non-exported symbols are made local in the static libraries. >> This means that the risk of symbol conflict is the same for static libraries >> as for dynamic libraries (i.e. in practice zero, as long as a consistent >> naming scheme is used for exported functions). >> >> 2) Remove the work-arounds to exclude duplicated symbols. >> >> 3) Fix some code in hotspot and the JDK libraries that did not work properly >> with a static java launcher. >> >> The latter fixes are copied from or inspired by the work done by >> @jianglizhou and her team as part of the Project Leyden [Hermetic >> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime). > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Add dummy implementation of os::lookup_function for Windows make/modules/java.base/lib/CoreLibraries.gmk line 148: > 146: $(LIBJLI_EXTRA_FILE_LIST)) > 147: > 148: # Do not include these libz objects in the static libjli library. Why this is no longer needed? - PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648290693
Re: RFR: 8333268: Fixes for static build [v2]
On Tue, 18 Jun 2024 17:57:29 GMT, Magnus Ihse Bursie wrote:
>> Magnus Ihse Bursie has updated the pull request with a new target base due
>> to a merge or a rebase. The incremental webrev excludes the unrelated
>> changes brought in by the merge/rebase. The pull request contains seven
>> additional commits since the last revision:
>>
>> - Merge branch 'master' into static-linking-progress
>> - Merge branch 'master' into static-linking-progress
>> - Move the exported JVM_IsStaticallyLinked to a better location
>> - Use runtime lookup of static vs dynamic instead of #ifdef STATIC_BUILD
>> - Copy fix for init_system_properties_values on linux
>> - Make sure we do not try to build static libraries on Windows
>> - 8333268: Fixes for static build
>
> src/hotspot/os/linux/os_linux.cpp line 605:
>
>> 603:
>> 604: // Get rid of /{client|server|hotspot}, if binary is libjvm.so.
>> 605: // Or, cut off /.
>
> @jianglizhou This code is based on changes in the Hermetic Java repo, but I
> do not fully understand neither the comment nor what the purpose is. If you
> could explain this a bit I'd be grateful.
The specific related commit in the hermetic Java branch is
https://github.com/openjdk/leyden/commit/53aa8f0cf418ab5f435a4b9996c7754fb8505d4b.
The change in os_linux.cpp here is to make sure that the libjvm.so related
path manipulation is conditionally done only. The check at line 599 looks for
"/libjvm.so" substring, so we only chop off (`*pslash = `\0` at line 601) that
part when necessary. In the static JDK case, there is no `libjvm.so` and the
path string is `/bin/javastatic`, which should not be affected.
Otherwise, it could fail.
I found the code was not very easy to follow when running into problems and
fixing for static support. So I added a bit more comments in the code here. The
comment above about `/{client|server|hotspot}` was there originally. I think we
no longer have those directories. We can cleanup that later, since it needs
some more testing.
@magicus, thanks a lot for extracting/reworking/cleaning-up the static Java
changes from the hermetic Java branch. That's a substantial amount of work!
I have one quick comment about the removal of `STATIC_LIB_EXCLUDE_OBJS`
changes. Will post it as separate comment for the related code.
I'll also look closely of the vm & jdk changes and compare those with the
changes in the hermetic Java branch this week.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19478#discussion_r1648283151
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]
On Sat, 30 Mar 2024 03:47:10 GMT, Liam Miller-Cushon wrote: > I think that's similar idea to one of the alternatives I mentioned earlier, > won't that allocate for every central directory entry? Ok. Re-reading your earlier comment more closely, I see you mentioned allocating memory option. The allocation of the buffer for reading extra fields is only done when needed (if `cen_offset` is `ZIP64_MAGICVAL` for the validate_lochdr case). I think I'm missing the part about the "allocate for every central directory entry" with the approach. Could you please elaborate a bit more? > This callsite has already read the data we need into a buffer, if we end up > doing something like that it might be better to do the allocation only for > the call site in `validate_lochdr`, and have the shared helper take the > pointer to the buffer instead of having a duplicate read. > > It seems like there are some tradeoffs here, I can definitely restructure > this, but I'd also like to get some feedback from a core-libs owner on the > overall approach before iterating too much more. Yeah, agreed. It's a good idea to have someone work closely with ZIP and have more insights on central directory structure to take a look at this change now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1545465731
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]
On Fri, 29 Mar 2024 18:43:51 GMT, Liam Miller-Cushon wrote:
>> src/java.base/share/native/libjli/parse_manifest.c line 506:
>>
>>> 504: jlong offset = 0;
>>> 505: while (offset < cenext) {
>>> 506: short headerId = ZIPEXT_HDR(base + offset);
>>
>> This block of code is very similar to the corresponding part (reading the
>> ZIP64 extension extra fields) in `validate_lochdr`. Perhaps also refractor
>> to a common helper function?
>
> The other loop uses `readAt` to read in additional data and advance through
> the extra fields, this loop already has access to a buffer that contains all
> of the data for the extra fields and doesn't need to do that.
>
> I considered having the other loop read in all of the data for the extra
> fields so there could be a shared helper, but the data is variable length, so
> that would have required having a large fixed-size buffer or allocating
> memory, which this code seems to be trying to avoid.
>
> Do you see a good way to share the loop?
How about something like the following (I haven't tried to compile or test,
please double check if everything is correct)? The size of the extra fields is
recorded in the 2-byte field, `extra field length`, so we can just read all
extra fields in a temp buffer. It causes less overhead with just one read.
jboolean read_zip64_ext(int fd, jlong censtart, jlong base_offset, Byte *cenhdr,
jboolean check_offset_only, jboolean
read_extra_fields) {
jlong cenlen = CENLEN(cenhdr);
jlong censiz = CENSIZ(cenhdr);
jlong cenoff = CENOFF(cenhdr);
if (cenoff == ZIP64_MAGICVAL ||
(!check_offset_only && (censiz == ZIP64_MAGICVAL || cenlen ==
ZIP64_MAGICVAL))) {
jlong cenext = CENEXT(cenhdr);
if (cenext == 0) {
return JNI_FALSE;
}
jlong start = censtart + CENHDR + CENNAM(cenhdr);
jlong offset = 0;
Byte *p = start;
// Read the extra fields if needed.
if (read_extra_fields) {
*p = (Byte*)malloc(cenext);
if (p == NULL) {
return JNI_FALSE;
}
if (!readAt(fd, start + offset, cenext, p)) {
free(p);
return JNI_FALSE;
}
}
while (offset < cenext) {
short headerId = ZIPEXT_HDR(p + offset);
short headerSize = ZIPEXT_SIZ(p + offset);
if (headerId == ZIP64_EXTID) {
if (!read_zip64_ext(p, &cenlen, &censiz, &cenoff, CENDSK(cenhdr))) {
if (p != start) {
free(p);
}
return JNI_FALSE;
}
break;
}
offset += 4 + headerSize;
}
}
if (p != start) {
free(p);
}
return JNI_TRUE;
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1545055704
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]
On Fri, 29 Mar 2024 18:41:09 GMT, Liam Miller-Cushon wrote:
>> src/java.base/share/native/libjli/parse_manifest.c line 197:
>>
>>> 195: jlong cenoff = CENOFF(cenhdr);
>>> 196: jlong cenext = CENEXT(cenhdr);
>>> 197: if (cenoff == ZIP64_MAGICVAL && cenext > 0) {
>>
>> Probably also need to check if `cenlen` or `censiz` is ZIP64_MAGICVAL?
>
> I think it doesn't matter, because the validation below only uses `cenoff`.
> If `cenoff` fits in 32 bits, we don't need to read the zip64 extra info.
Thanks for the explanation. Could you please add a comment with the info above
the `if` statement?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544820598
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]
On Fri, 29 Mar 2024 17:38:47 GMT, Liam Miller-Cushon wrote:
>> This change fixes a zip64 bug in the launcher that is prevent it from
>> reading the manifest of jars where the 'relative offset of local header'
>> field in the central directory entry is >4GB. As described in APPNOTE.TXT
>> 4.5.3, the offset is too large to be stored in the central directory it is
>> stored in a 'Zip64 Extended Information Extra Field'.
>
> Liam Miller-Cushon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Maximum Zip64 extra field length is 32
src/java.base/share/native/libjli/parse_manifest.c line 506:
> 504: jlong offset = 0;
> 505: while (offset < cenext) {
> 506: short headerId = ZIPEXT_HDR(base + offset);
This block of code is very similar to the corresponding part (reading the ZIP64
extension extra fields) in `validate_lochdr`. Perhaps also refractor to a
common helper function?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544754724
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v4]
On Fri, 29 Mar 2024 17:38:47 GMT, Liam Miller-Cushon wrote:
>> This change fixes a zip64 bug in the launcher that is prevent it from
>> reading the manifest of jars where the 'relative offset of local header'
>> field in the central directory entry is >4GB. As described in APPNOTE.TXT
>> 4.5.3, the offset is too large to be stored in the central directory it is
>> stored in a 'Zip64 Extended Information Extra Field'.
>
> Liam Miller-Cushon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Maximum Zip64 extra field length is 32
src/java.base/share/native/libjli/parse_manifest.c line 197:
> 195: jlong cenoff = CENOFF(cenhdr);
> 196: jlong cenext = CENEXT(cenhdr);
> 197: if (cenoff == ZIP64_MAGICVAL && cenext > 0) {
Probably also need to check if `cenlen` or `censiz` is ZIP64_MAGICVAL?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544752292
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v3]
On Fri, 29 Mar 2024 18:08:56 GMT, Liam Miller-Cushon wrote: >> Could you please point to the related spec for the other `extra field`? > > it's in APPNOTE.TXT 4.5, the extra field structure is `header1+data1 + > header2+data2 . . .`, and we have to iterate through to see if there's an > entry that's a zip64 extended information extra field Thanks! That's helpful. I couldn't find the information earlier. According to the spec (quoted below), finding the `Header ID` field can be used to identify the type of data, which is the logic done here. Could you please a comment above the `while` block with some of the related details? Thanks. The Header ID field indicates the type of data that is in the following data block. 4.5.2 The current Header ID mappings defined by PKWARE are: 0x0001Zip64 extended information extra field 0x0007AV Info 0x0008Reserved for extended language encoding data (PFS) (see APPENDIX D) 0x0009OS/2 0x000aNTFS 0x000cOpenVMS 0x000dUNIX 0x000eReserved for file stream and fork descriptors 0x000fPatch Descriptor 0x0014PKCS#7 Store for X.509 Certificates 0x0015X.509 Certificate ID and Signature for individual file 0x0016X.509 Certificate ID for Central Directory 0x0017Strong Encryption Header 0x0018Record Management Controls 0x0019PKCS#7 Encryption Recipient Certificate List 0x0020Reserved for Timestamp record 0x0021Policy Decryption Key Record 0x0022Smartcrypt Key Provider Record 0x0023Smartcrypt Policy Key Data Record 0x0065IBM S/390 (Z390), AS/400 (I400) attributes - uncompressed 0x0066Reserved for IBM S/390 (Z390), AS/400 (I400) attributes - compressed 0x4690POSZIP 4690 (reserved) - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544752797
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v3]
On Fri, 29 Mar 2024 17:50:00 GMT, Liam Miller-Cushon wrote:
>> src/java.base/share/native/libjli/parse_manifest.c line 505:
>>
>>> 503: Byte *base = p + CENHDR + CENNAM(p);
>>> 504: jlong offset = 0;
>>> 505: while (offset < cenext) {
>>
>> Any reason why a loop is need here?
>
> There can be multiple extra fields, and the zip64 extended information may
> not be the first one
Could you please point to the related spec for the other `extra field`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544739399
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v3]
On Wed, 27 Mar 2024 17:43:41 GMT, Liam Miller-Cushon wrote:
>> This change fixes a zip64 bug in the launcher that is prevent it from
>> reading the manifest of jars where the 'relative offset of local header'
>> field in the central directory entry is >4GB. As described in APPNOTE.TXT
>> 4.5.3, the offset is too large to be stored in the central directory it is
>> stored in a 'Zip64 Extended Information Extra Field'.
>
> Liam Miller-Cushon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Make cendsk an unsigned short
src/java.base/share/native/libjli/parse_manifest.c line 505:
> 503: Byte *base = p + CENHDR + CENNAM(p);
> 504: jlong offset = 0;
> 505: while (offset < cenext) {
Any reason why a loop is need here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544726976
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v3]
On Wed, 27 Mar 2024 17:43:41 GMT, Liam Miller-Cushon wrote:
>> This change fixes a zip64 bug in the launcher that is prevent it from
>> reading the manifest of jars where the 'relative offset of local header'
>> field in the central directory entry is >4GB. As described in APPNOTE.TXT
>> 4.5.3, the offset is too large to be stored in the central directory it is
>> stored in a 'Zip64 Extended Information Extra Field'.
>
> Liam Miller-Cushon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Make cendsk an unsigned short
src/java.base/share/native/libjli/parse_manifest.c line 503:
> 501: || cenoff == ZIP64_MAGICVAL)
> 502: && cenext > 0) {
> 503: Byte *base = p + CENHDR + CENNAM(p);
How about adding a comment describing the start of then extended fields
calculation?
// The start of the extended fields = cen_header_start + cen_header_fixed_fix +
file_name_length
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544725300
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB [v3]
On Wed, 27 Mar 2024 16:03:09 GMT, Liam Miller-Cushon wrote: >> src/java.base/share/native/libjli/manifest_info.h line 59: >> >>> 57: #define ZIP64_EXTID 1 // Extra field Zip64 header ID >>> 58: >>> 59: #define ZIP64_EXTMAXLEN 36 // Maximum Zip64 extra field length >> >> The fields described in APPNOTE-6.3.9.TXT 4.5.3 are total 32 bytes. Any >> other additional fields in the Zip64 extended information? >> >> >> Value Size Description >> - --- >> (ZIP64) 0x0001 2 bytesTag for this "extra" block type >> Size 2 bytesSize of this "extra" block >> Original >> Size 8 bytesOriginal uncompressed file size >> Compressed >> Size 8 bytesSize of compressed data >> Relative Header >> Offset 8 bytesOffset of local header record >> Disk Start >> Number 4 bytesNumber of the disk on which >> this file starts > > Thanks for the catch, I had missed that the disk start number is 4 bytes and > not 8. I pushed a commit. I also removed some unused references to the disk > number, which is only being used to validate the size of the zip64 extended > info. Ok. Could you please also fix the above `36`? Please change the start of `//` (comment) to be aligned with the earlier comment lines. >> src/java.base/share/native/libjli/manifest_info.h line 146: >> >>> 144: * Macros for getting Extensible Data Fields >>> 145: */ >>> 146: #define ZIPEXT_HDR(b) SH(b, 0) /* Header ID */ >> >> How about naming the macros as ZIP64EXT_HDR and ZIP64EXT_SIZ? > > My thinking was that all extensible data fields start with a header with an > id and a length, and these macros are used to iterate through the extra > fields to find the zip64 extended information extra field, so these macros > aren't zip64-specific. Seem reasonable, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544719690 PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1544718064
Re: RFR: 8328995: launcher can't open jar files where the offset of the manifest is >4GB
On Mon, 25 Mar 2024 21:37:03 GMT, Liam Miller-Cushon wrote: > This change fixes a zip64 bug in the launcher that is prevent it from reading > the manifest of jars where the 'relative offset of local header' field in the > central directory entry is >4GB. As described in APPNOTE.TXT 4.5.3, the > offset is too large to be stored in the central directory it is stored in a > 'Zip64 Extended Information Extra Field'. src/java.base/share/native/libjli/manifest_info.h line 59: > 57: #define ZIP64_EXTID 1 // Extra field Zip64 header ID > 58: > 59: #define ZIP64_EXTMAXLEN 36 // Maximum Zip64 extra field length The fields described in APPNOTE-6.3.9.TXT 4.5.3 are total 32 bytes. Any other additional fields in the Zip64 extended information? Value Size Description - --- (ZIP64) 0x0001 2 bytesTag for this "extra" block type Size 2 bytesSize of this "extra" block Original Size 8 bytesOriginal uncompressed file size Compressed Size 8 bytesSize of compressed data Relative Header Offset 8 bytesOffset of local header record Disk Start Number 4 bytesNumber of the disk on which this file starts src/java.base/share/native/libjli/manifest_info.h line 146: > 144: * Macros for getting Extensible Data Fields > 145: */ > 146: #define ZIPEXT_HDR(b) SH(b, 0) /* Header ID */ How about naming the macros as ZIP64EXT_HDR and ZIP64EXT_SIZ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1540404585 PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1540405121
Integrated: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
On Tue, 27 Feb 2024 03:11:37 GMT, Jiangli Zhou wrote: > Please help review this trivial change. This was branched from > https://github.com/openjdk/jdk/pull/18013, based on discussion with > @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks This pull request has now been integrated. Changeset: 81b065a9 Author: Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/81b065a95d670ef357c36239d8c408cd72a5c48c Stats: 20 lines in 2 files changed: 0 ins; 13 del; 7 mod 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c Reviewed-by: djelinski, rriggs - PR: https://git.openjdk.org/jdk/pull/18019
Re: RFR: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
On Tue, 27 Feb 2024 07:29:46 GMT, Daniel Jeliński wrote: >> Please help review this trivial change. This was branched from >> https://github.com/openjdk/jdk/pull/18013, based on discussion with >> @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks > > LGTM @djelinski @RogerRiggs Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18019#issuecomment-1966903722
RFR: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
Please help review this trivial change. This was branched from https://github.com/openjdk/jdk/pull/18013, based on discussion with @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks - Commit messages: - 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c Changes: https://git.openjdk.org/jdk/pull/18019/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18019&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326714 Stats: 20 lines in 2 files changed: 0 ins; 13 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/18019.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18019/head:pull/18019 PR: https://git.openjdk.org/jdk/pull/18019
Integrated: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
On Mon, 26 Feb 2024 20:20:31 GMT, Jiangli Zhou wrote: > Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. This pull request has now been integrated. Changeset: 0901dede Author: Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/0901dedefe16afa3f7222723b3fec7a22d9df675 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/18013
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Tue, 27 Feb 2024 00:34:49 GMT, Serguei Spitsyn wrote: >> Jiangli Zhou has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address plummercj's comment and make forkedChildProcess static. >> - Revert src/java.base/unix/native/libjava/childproc.c and >> src/java.base/unix/native/libjava/childproc.h. Will handle those with >> JDK-8326714. > > Marked as reviewed by sspitsyn (Reviewer). Thanks for the review, @sspitsyn. - PR Comment: https://git.openjdk.org/jdk/pull/18013#issuecomment-1965631861
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 23:50:11 GMT, Chris Plummer wrote: > Looks good. Thanks for the quick review, @plummercj. - PR Comment: https://git.openjdk.org/jdk/pull/18013#issuecomment-1965539618
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 20:37:45 GMT, Chris Plummer wrote: >> Jiangli Zhou has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address plummercj's comment and make forkedChildProcess static. >> - Revert src/java.base/unix/native/libjava/childproc.c and >> src/java.base/unix/native/libjava/childproc.h. Will handle those with >> JDK-8326714. > > src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 65: > >> 63: // by this function. This function returns 0 on failure >> 64: // and 1 on success. >> 65: static int > > I think you should also make forkedChildProcess static. It was added at the > same time. Updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503414683
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function [v2]
> Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. Jiangli Zhou has updated the pull request incrementally with two additional commits since the last revision: - Address plummercj's comment and make forkedChildProcess static. - Revert src/java.base/unix/native/libjava/childproc.c and src/java.base/unix/native/libjava/childproc.h. Will handle those with JDK-8326714. - Changes: - all: https://git.openjdk.org/jdk/pull/18013/files - new: https://git.openjdk.org/jdk/pull/18013/files/bb9e2791..3816d315 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=00-01 Stats: 3 lines in 3 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18013/head:pull/18013 PR: https://git.openjdk.org/jdk/pull/18013
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 22:15:00 GMT, Jiangli Zhou wrote: >> src/java.base/unix/native/libjava/childproc.h line 134: >> >>> 132: int closeSafely(int fd); >>> 133: int isAsciiDigit(char c); >>> 134: int closeDescriptors(void); >> >> It seems that most of the APIs in this file should be static. I don't think >> you should selectively deal with just one of them because of the conflict. >> Since this ends up being a more involved change, and is in a different >> component than the jdwp change, it should probably have a separate PR. > > @plummercj thanks for looking into this! Sounds good to make the additional > local functions static in these files. Perhaps we can use two different FRs. > I'll make the current one to handle the ones in > src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. Filed https://bugs.openjdk.org/browse/JDK-8326714. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503414392
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
On Mon, 26 Feb 2024 20:40:52 GMT, Chris Plummer wrote: >> Please help review this trivial fix for resolving `ld: error: duplicate >> symbol: closeDescriptors` when static linking with both libjdwp and libjava, >> thanks. > > src/java.base/unix/native/libjava/childproc.h line 134: > >> 132: int closeSafely(int fd); >> 133: int isAsciiDigit(char c); >> 134: int closeDescriptors(void); > > It seems that most of the APIs in this file should be static. I don't think > you should selectively deal with just one of them because of the conflict. > Since this ends up being a more involved change, and is in a different > component than the jdwp change, it should probably have a separate PR. @plummercj thanks for looking into this! Sounds good to make the additional local functions static in these files. Perhaps we can use two different FRs. I'll make the current one to handle the ones in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503379848
RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
Please help review this trivial fix for resolving `ld: error: duplicate symbol: closeDescriptors` when static linking with both libjdwp and libjava, thanks. - Commit messages: - Make closeDescriptors() as static function in src/java.base/unix/native/libjava/childproc.c and src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. Changes: https://git.openjdk.org/jdk/pull/18013/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326433 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18013/head:pull/18013 PR: https://git.openjdk.org/jdk/pull/18013
Integrated: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi
On Tue, 3 Oct 2023 02:19:00 GMT, Jiangli Zhou wrote: > Please help review this trivial change that adds missing > DEF_STATIC_JNI_OnLoad for librmi. > > Thanks This pull request has now been integrated. Changeset: ae796a4e Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/ae796a4e1000afb836c1b0a65edf39ab9d2e7ce2 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8316923: Add DEF_STATIC_JNI_OnLoad for librmi Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/16020
Re: RFR: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi [v2]
On Tue, 3 Oct 2023 06:08:20 GMT, Alan Bateman wrote: > Surprised this one didn't have DEF_STATIC_JNI_OnLoad already. Change looks > okay, can you update the copyright date before integrating. Thanks for the review! Updated copyright year as you suggested. - PR Comment: https://git.openjdk.org/jdk/pull/16020#issuecomment-1745218266
Re: RFR: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi [v2]
> Please help review this trivial change that adds missing > DEF_STATIC_JNI_OnLoad for librmi. > > Thanks Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Update copyright year as suggested by @AlanBateman. - Changes: - all: https://git.openjdk.org/jdk/pull/16020/files - new: https://git.openjdk.org/jdk/pull/16020/files/f51209fb..bcd5dfd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16020&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16020&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16020.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16020/head:pull/16020 PR: https://git.openjdk.org/jdk/pull/16020
RFR: 8316923: Add DEF_STATIC_JNI_OnLoad for librmi
Please help review this trivial change that adds missing DEF_STATIC_JNI_OnLoad for librmi. Thanks - Commit messages: - 8316923: Add DEF_STATIC_JNI_OnLoad for librmi Changes: https://git.openjdk.org/jdk/pull/16020/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16020&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8316923 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16020.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16020/head:pull/16020 PR: https://git.openjdk.org/jdk/pull/16020
Re: [jdk21] RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Wed, 21 Jun 2023 17:09:58 GMT, Kevin Rushforth wrote: > Since this Enhancement was rejected for JDK 21, this PR should be closed. Closing without integration accordingly, thanks. - PR Comment: https://git.openjdk.org/jdk21/pull/26#issuecomment-1601273074
[jdk21] Withdrawn: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Fri, 16 Jun 2023 20:36:07 GMT, Jiangli Zhou wrote: > 8307858: [REDO] JDK-8307194 Add make target for optionally building a > complete set of all JDK and hotspot libjvm static libraries This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk21/pull/26
Withdrawn: 8303796: Optionally build fully statically linked JDK image
On Fri, 28 Apr 2023 01:03:28 GMT, Jiangli Zhou wrote: > Initial implementation for supporting building a fully statically linked > (with a desired set of JDK native libraries and libjvm) Java launcher > executable, which is named as 'javastatic'. > > In this PR, the support is only added for the linux platform. Both gcc and > clang can be supported. For current demo/testing purpose, the bin/javastatic > is statically linked with awt headless and other common JDK native libraries. > The current PR doesn't fully handle creating the bundle for a static JDK > image, which can be supported later. > > To build the statically linked executable: > > 1. Configure the JDK build with --with-static-java=yes > 2. Build static-java-image, e.g. 'make jdk-image static-java-image' > > The 'javastatic' binary created by the static-java-image target is not > runnable. The runtime issues will be handled separately. > > Following is a summary of the changes in this PR: > > - Add make/autoconf/static-java.m4 for defining STATIC_JAVA_SETUP. Add > STATIC_JAVA_SETUP to make/autoconf/configure.ac. > > - Changes in make/Main.gmk > - Add HOTSPOT_VARIANT_STATIC_LIBS_TARGETS and > DeclareHotspotStaticLibsRecipe for building libjvm static library. > - Add static-java-image for creating the fully statically linked standard > Java launcher binary, bin/javastatic. The build process also places libjvm.a > into the 'static-libs' image lib/ directory. > > - Add make/StaticLink.gmk, which contains the main support for creating the > fully statically linked Java launcher binary. > > - Setup LDFLAGS_CXX_STATIC_JDK based on $TOOLCHAIN_TYPE in > make/autoconf/flags-ldflags.m4. > > - Always use bundled libraries for zlib, freetype, etc for static build > support. The related changes are in make/autoconf/lib-bundled.m4 and > make/autoconf/lib-freetype.m4. Building the bundled zlib, freetype and etc > libraries ensures those libraries are included in the JDK binary bundle. It > decouples the assumptions/requirements of the static Java image build process > from the assumptions/requirements of the JDK build process. A post process > that builds the static Java image can use those bundled libraries provided by > JDK binary if needed. > > - When building a fully statically linked java launcher executable, the > --whole-archive linker option is used for the JDK/VM static libraries to make > sure it links every object (.o) file provided by those static libraries. As a > result, we need to remove any duplicate object files from the different > JDK/VM static libraries. To do that, STATIC_LIB_EXCLUDE_OBJS is added and > used in make/common/Nativ... This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/13709
Re: [jdk21] RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Tue, 20 Jun 2023 17:25:16 GMT, Erik Joelsson wrote: > The changes look ok. Thanks. I'll wait for approval on https://bugs.openjdk.org/browse/JDK-8307858 as well. - PR Comment: https://git.openjdk.org/jdk21/pull/26#issuecomment-1599256118
Re: [jdk21] RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Tue, 20 Jun 2023 17:24:03 GMT, Erik Joelsson wrote: > > @erikj - You did a round of Mach5 testing on the JDK22 version of this fix. > > Do you have plans to redo that testing for the JDK21 backport? > > I have done testing of the JDK 21 version of the patch and it's running > cleanly. Thanks for testing, @erikj79! - PR Comment: https://git.openjdk.org/jdk21/pull/26#issuecomment-1599255419
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Fri, 16 Jun 2023 22:08:19 GMT, Daniel D. Daugherty wrote: > GHA is failing on windows; is this related to this PR or something else? The windows build failures occur with other PRs as well, e.g. https://github.com/openjdk/jdk21/pull/24/checks?check_run_id=14317258603. They should be unrelated. make: *** [jtdiff.gmk:36: D:/a/jdk21/jdk21/jtreg/src/build/classes.com.sun.javatest.diff.ok] Error 126 Error: Process completed with exit code 2. I noticed these presubmit failures with `jdk` started since last week or so. - PR Comment: https://git.openjdk.org/jdk21/pull/26#issuecomment-1595395385
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Fri, 16 Jun 2023 20:52:13 GMT, Kevin Rushforth wrote: > As a P4 enhancement, this doesn't meet the criteria for integration into JDK > 21 during [Rampdown Phase > 1](https://mail.openjdk.org/pipermail/jdk-dev/2023-June/007911.html). You > could request late approval to get this enhancement in, via the process > described in [JEP 3](https://openjdk.org/jeps/3). Thanks for the pointer, @kevinrushforth. Will follow the process. - PR Comment: https://git.openjdk.org/jdk21/pull/26#issuecomment-1595318981
