Re: RFR: 8265702: ZGC on macOS/aarch64 [v2]
On Thu, 22 Apr 2021 06:43:45 GMT, Per Liden wrote: >> This patch enables ZGC on macOS/aarch64. It does three things: >> 1) Enables building of ZGC on this platform. >> 2) Adds `os::processor_id()`, which for now always returns 0. >> 3) Fixes a WX issue in `OptoRuntime::handle_exception_C()`, where the >> stackwater mark might unnecessarily process a frame when the thread is in >> WXExec mode. In this case, the we're not touching any oops, so we don't need >> to process any frames. >> >> Testing: Passes the same tests as macOS/x86_64 (with the exception of >> pre-existing issues unrelated to ZGC). > > Per Liden has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment to #else Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/3609
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Mar 2021 17:46:41 GMT, Andrew Haley wrote: > > A list of the bugs that our internal testing revealed so far: > > Are any of these blockers for integration? Some of them are to do with things > like features that aren't yet supported, and we can't fix what we can't see. I don't personally think any of these issues are blockers. It's a great effort as it is and very much appreciated. Anything else can be fixed as a followup. There might be some legal requirements (i.e. JCK) that I'm not in position to comment on, however, so someone else might need to chime in here. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Mar 2021 11:05:20 GMT, Anton Kozlov wrote: >> For platform files that were copied from other ports to this port, if the >> file wasn't >> changed I presume the copyright years are left alone. If the file required >> changes >> for this port, I expect the year to be updated to 2021. How are you >> verifying that >> these copyright years are being properly managed on the new files? >> >> For the new W^X helpers, e.g., WXWriteFromExecSetter, a short comment >> explaining why one was landed in a particular place would help reviewers. >> Also see my comment about creating a new ThreadToNativeWithWXExecFromVM >> helper. >> >> I'm stopping my review with all the src/hotspot files done for now. > >> For platform files that were copied from other ports to this port, if the >> file wasn't >> changed I presume the copyright years are left alone. If the file required >> changes >> for this port, I expect the year to be updated to 2021. How are you >> verifying that >> these copyright years are being properly managed on the new files? > > There are no exact copies, based on > git -c diff.renameLimit=1000 diff --find-copies-harder -C75% > --name-status upstream/master... > > So every file changed in the branch potentially needs the copyright update. > All file diffs are not trivial, IMHO. > > I'll run the copyright update after we fix a few remaining issues with the > PR, to avoid updating copyright and changing/reverting the actual content. A list of the bugs that our internal testing revealed so far: https://bugs.openjdk.java.net/browse/JDK-8262952 https://bugs.openjdk.java.net/browse/JDK-8262894 https://bugs.openjdk.java.net/browse/JDK-8262895 https://bugs.openjdk.java.net/browse/JDK-8262896 https://bugs.openjdk.java.net/browse/JDK-8262897 https://bugs.openjdk.java.net/browse/JDK-8262898 https://bugs.openjdk.java.net/browse/JDK-8262899 https://bugs.openjdk.java.net/browse/JDK-8262900 https://bugs.openjdk.java.net/browse/JDK-8262901 https://bugs.openjdk.java.net/browse/JDK-8262903 https://bugs.openjdk.java.net/browse/JDK-8262904 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v23]
On Tue, 2 Mar 2021 23:21:28 GMT, David Holmes wrote: > Note that `thread` can be NULL here if the signal handler is running in a > non-attached thread. If we then perform: > `ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) : _thread(thread ? > thread : Thread::current()),` > we call Thread::current() on a non-attached thread and that will assert/crash > if we get NULL. Either avoid using WX when the thread is NULL, or else change > to use Thread::current_or_null_safe() and ensure all uses have a NULL check. https://bugs.openjdk.java.net/browse/JDK-8262903 tracks this issue. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]
On Wed, 17 Feb 2021 12:36:10 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 88 commits: > > - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos > - Re-do safefetch.hpp > - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into > jdk-macos > - stubRoutines.inline.hpp -> safefetch.hpp > - Update copyrights > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Extract SafeFetch32/N to stubRoutines.inline.hpp > - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp" > >This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0. > - Merge pull request #9 from VladimirKempik/pull/2200 > >Removed unused variables > - ... and 78 more: > https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]
On Mon, 15 Feb 2021 06:24:13 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 20 additional > commits since the last revision: > > - Lanai PR#191 - 8261705 - jdv > - Lanai PR#190 - 8261706 - jdv > - Lanai PR#189 - 8261712 - avu > - Lanai PR#187 - 8261704 - jdv > - Lanai PR#186 - 8261638 - avu > - Lanai PR#185 - 8261632 - jdv > - Lanai PR#184 - 8261620 - aghaisas > - Lanai PR#182 - 8261547 - psadhukhan > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#181 - 8261143 - aghaisas > - ... and 10 more: > https://git.openjdk.java.net/jdk/compare/827ab648...7b0b0dc4 Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Mon, 15 Feb 2021 18:30:51 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed > a startup time regression with the Metal rendering pipeline, so I dug a bit > and here is what I found using Xcode startup profiler: > > Here is the OpenGL pipeline: > src="https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png";> > > > Here is the Metal pipeline: > src="https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png";> > > If I read the results correctly, there is a weird 330ms time gap in the case > of the Metal pipeline where nothing is done and it looks like the culprit is > `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable` > > The OpenGL pipeline gets blocked only for 83ms in > `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo` > > I would advice that we take a closer look at why > 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so > long, and optimize it, or cache the results, so the next VM instance can skip > it if needed (assuming the call doesn't end up initializing the native Metal > or something like that), still worth taking a look. > > I would also recommend that you adopt Xcode and its Instruments profiler > tools for future work. Please let me know if you need help in this area. > > I only scratched the surface here and I think that there is plenty of > profiling that can be done to investigate the startup time regression further. > > Thanks @gerard-ziemski for taking a detailed look at this. > > We do have an open bug to address this. Please refer > > [JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825). > > Hi Gerard, expecting a process and parsing its output is definitely not ideal > and that's why there's this open bug. > One thing that is under consideration is to equate >= 10.14 with Metal is > available since as of 10.14 macOS won't install if a system does not support > metal. We have no compelling reason to support metal on earlier releases > anyway .. those are either already unsupported or barely supported and OGL > will always be available there. I did not know that there already was a bug covering this issue. Your idea seems reasonable. More than just focusing on this immediate issue, however, I was hoping to raise the point of us starting adopting profiling tools to analyze our code (memory utilization, leaks, cpu/gpu profiling etc). A new feature that brings 10% benefit, but uses 50% more resources for example would probably not be a good investment. And to figure that we need to relay on some good tools and Xcode provides some. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:55:47 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#181 - 8261143 - aghaisas >> - Lanai PR#180 - 8261546 - jdv > > Changes requested by gziemski (Committer). I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed a startup time regression with the Metal rendering pipeline, so I dug a bit and here is what I found using Xcode startup profiler: Here is the OpenGL pipeline: https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png";> Here is the Metal pipeline: https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png";> If I read the results correctly, there is a weird 330ms time gap in the case of the Metal pipeline where nothing is done and it looks like the culprit is `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable` The OpenGL pipeline gets blocked only for 83ms in `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo` I would advice that we take a closer look at why 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so long, and optimize it, or cache the results, so the next VM instance can skip it if needed (assuming the call doesn't end up initializing the native Metal or something like that), still worth taking a look. I would also recommend that you adopt Xcode and its Instruments profiler tools for future work. Please let me know if you need help in this area. I only scratched the surface here and I think that there is plenty of profiling that can be done to investigate the startup time regression further. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:58:36 GMT, Gerard Ziemski wrote: >> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: >> >>> 110: sourceSize:MTLSizeMake(self.buffer.width, >>> self.buffer.height, 1) >>> 111: toTexture:mtlDrawable.texture destinationSlice:0 >>> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >>> 112: [blitEncoder endEncoding]; >> >> There is an issue with this code. Running Java2D.jar in Xcode asserts here >> with this message: >> >> 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation >> Enabled >> 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API >> Validation Enabled >> -[MTLDebugBlitCommandEncoder >> internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: >> failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= >> height(400).' >> (lldb) >> >> and forcing the execution past it results in the java process crashing. >> >> A solution would be to clip the src size like this: >> >> NSUInteger src_x = self.leftInset*self.contentsScale; >> NSUInteger src_y = self.topInset*self.contentsScale; >> NSUInteger src_w = self.buffer.width-src_x; >> NSUInteger src_h = self.buffer.height-src_y; >> >> [blitEncoder >> copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 >> sourceOrigin:MTLOriginMake(src_x, src_y, 0) >> sourceSize:MTLSizeMake(src_w, src_h, 1) >> toTexture:mtlDrawable.texture destinationSlice:0 >> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >> [blitEncoder endEncoding]; > > I guess you will only see this if `Metal API Validation Enabled` Which actually begs a question of whether we tested with `Metal API Validation Enabled` ? - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 20:55:35 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#181 - 8261143 - aghaisas >> - Lanai PR#180 - 8261546 - jdv > > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: > >> 110: sourceSize:MTLSizeMake(self.buffer.width, >> self.buffer.height, 1) >> 111: toTexture:mtlDrawable.texture destinationSlice:0 >> destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; >> 112: [blitEncoder endEncoding]; > > There is an issue with this code. Running Java2D.jar in Xcode asserts here > with this message: > > 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation > Enabled > 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API > Validation Enabled > -[MTLDebugBlitCommandEncoder > internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: > failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= > height(400).' > (lldb) > > and forcing the execution past it results in the java process crashing. > > A solution would be to clip the src size like this: > > NSUInteger src_x = self.leftInset*self.contentsScale; > NSUInteger src_y = self.topInset*self.contentsScale; > NSUInteger src_w = self.buffer.width-src_x; > NSUInteger src_h = self.buffer.height-src_y; > > [blitEncoder > copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 > sourceOrigin:MTLOriginMake((jint)src_x, (jint)src_y, 0) > sourceSize:MTLSizeMake(src_w, src_h, 1) > toTexture:mtlDrawable.texture destinationSlice:0 > destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; > [blitEncoder endEncoding]; I guess you will only see this if `Metal API Validation Enabled` - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 11:51:57 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas has updated the pull request incrementally with two additional > commits since the last revision: > > - Lanai PR#181 - 8261143 - aghaisas > - Lanai PR#180 - 8261546 - jdv Changes requested by gziemski (Committer). src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 112: > 110: sourceSize:MTLSizeMake(self.buffer.width, > self.buffer.height, 1) > 111: toTexture:mtlDrawable.texture destinationSlice:0 > destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; > 112: [blitEncoder endEncoding]; There is an issue with this code. Running Java2D.jar in Xcode asserts here with this message: 2021-02-11 14:11:45.710457-0600 java[49971:9486360] Metal API Validation Enabled 2021-02-11 14:11:46.038720-0600 system_profiler[49975:9486885] Metal API Validation Enabled -[MTLDebugBlitCommandEncoder internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:474: failed assertion `(sourceOrigin.y + sourceSize.height)(444) must be <= height(400).' (lldb) and forcing the execution past it results in the java process crashing. A solution would be to clip the src size like this: NSUInteger src_x = self.leftInset*self.contentsScale; NSUInteger src_y = self.topInset*self.contentsScale; NSUInteger src_w = self.buffer.width-src_x; NSUInteger src_h = self.buffer.height-src_y; [blitEncoder copyFromTexture:self.buffer sourceSlice:0 sourceLevel:0 sourceOrigin:MTLOriginMake((jint)src_x, (jint)src_y, 0) sourceSize:MTLSizeMake(src_w, src_h, 1) toTexture:mtlDrawable.texture destinationSlice:0 destinationLevel:0 destinationOrigin:MTLOriginMake(0, 0, 0)]; [blitEncoder endEncoding]; - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Wed, 10 Feb 2021 21:45:37 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Lanai PR#177 - 8261430 - aghaisas >> - Lanai PR#176 - 8261399 - jdv > > Marked as reviewed by gziemski (Committer). According to Xcode Instruments leak profile, there are 2 minor memory leaks in the Metal rendering pipeline: `#1 Malloc 80 Bytes 1 0x7fde0d4247b0 80 Byteslibjava.dylib getStringUTF8` 0 libsystem_malloc.dylib malloc_zone_malloc 1 libsystem_malloc.dylib malloc 2 libjava.dylib getStringUTF8 /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 3 libjava.dylib JNU_GetStringPlatformChars /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 4 libawt_lwawt.dylib Java_sun_java2d_metal_MTLGraphicsConfig_getMTLConfigInfo /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:151 5 0x11ab08d48 6 0x11ab0250d `#2 Malloc 80 Bytes 1 0x7fde0d4325a0 80 Byteslibjava.dylib getStringUTF8` 0 libsystem_malloc.dylib malloc_zone_malloc 1 libsystem_malloc.dylib malloc 2 libjava.dylib getStringUTF8 /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888 3 libjava.dylib JNU_GetStringPlatformChars /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917 4 libawt_lwawt.dylib Java_sun_java2d_metal_MTLGraphicsConfig_tryLoadMetalLibrary /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:120 5 0x11ab08d48 6 0x11ab024c8 Those can be handled as a followup issues though if you like, it's only 160 bytes total. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]
On Tue, 9 Feb 2021 12:29:52 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas has updated the pull request incrementally with two additional > commits since the last revision: > > - Lanai PR#177 - 8261430 - aghaisas > - Lanai PR#176 - 8261399 - jdv Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Wed, 10 Feb 2021 21:40:46 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > I tried to code review the native implementation files, but Metal APIs is > brand new to me and it's been a long while since I worked with graphics API, > so I can't be of much help really. > > The code I've looked at looked clean and nothing caught my eye. But it's a > partial code review at best. > > Good job! A small thing caught my eye when I was comparing OpenGL and Metal pipelines running J2Demo.jar It looks to me like OpenGL rendering pipeline uses brighter colors. I prefer the more subdued colors that Metal is rendering, but not sure if this is something you want to investigate further. I tried to capture what I see below: OpenGL: https://user-images.githubusercontent.com/63425797/107575744-dc0ca180-6bb5-11eb-9eb3-5cff415eb8a3.png";> Metal (the green color has a yellowish tint): https://user-images.githubusercontent.com/63425797/107575716-cf884900-6bb5-11eb-8ae8-14212ec94e87.png";> Performance wise I did not see much difference either way. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 23:07:39 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Lanai PR#175 - 8261304 - aghaisas > > Changes requested by gziemski (Committer). I tried to code review the native implementation file, but Metal APIs is brand new to me and it's been a long while since I worked with graphics API, so I can't be of much help really. The code I've looked at looked clean and nothing caught my eye. But it's a partial code review at best. Good job! - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas wrote:
>> **Description :**
>> This is the implementation of [JEP 382 : New macOS Rendering
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
>> It implements a Java 2D internal rendering pipeline for macOS using the
>> Apple Metal API.
>> The entire work on this was done under [OpenJDK Project -
>> Lanai](http://openjdk.java.net/projects/lanai/)
>>
>> We iterated through several Early Access (EA) builds and have reached a
>> stage where it is ready to be integrated to openjdk/jdk. The latest EA build
>> is available at - https://jdk.java.net/lanai/
>>
>> A new option -Dsun.java2d.metal=true | True needs to be used to use this
>> pipeline.
>>
>> **Testing :**
>> This implementation has been tested with the tests present at - [Test Plan
>> for JEP 382: New macOS Rendering
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396)
>>
>> **Note to reviewers :**
>> 1) Default rendering pipeline on macOS has not been changed by this PR.
>> OpenGL still stays as the default rendering pipeline and Metal rendering
>> pipeline is optional to choose.
>>
>> 2) To apply and test this PR -
>> To enable the metal pipeline you must specify on command line
>> -Dsun.java2d.metal=true (No message will be printed in this case) or
>> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is
>> enabled gets printed)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Lanai PR#175 - 8261304 - aghaisas
Changes requested by gziemski (Committer).
src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 61:
> 59:
> 60: public abstract class MTLSurfaceData extends SurfaceData
> 61: implements AccelSurface {
`MTLSurfaceData` and `OGLSurfaceData` seem to share lots of the same code,
isn't there a way to refactor the common code out?
There are other files that structually look identical, except for the names of
classes they use, ex `MTLMaskBlit.java` and `OGLMaskBlit.java`.
-
PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 12:28:07 GMT, Ajit Ghaisas wrote:
>> **Description :**
>> This is the implementation of [JEP 382 : New macOS Rendering
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
>> It implements a Java 2D internal rendering pipeline for macOS using the
>> Apple Metal API.
>> The entire work on this was done under [OpenJDK Project -
>> Lanai](http://openjdk.java.net/projects/lanai/)
>>
>> We iterated through several Early Access (EA) builds and have reached a
>> stage where it is ready to be integrated to openjdk/jdk. The latest EA build
>> is available at - https://jdk.java.net/lanai/
>>
>> A new option -Dsun.java2d.metal=true | True needs to be used to use this
>> pipeline.
>>
>> **Testing :**
>> This implementation has been tested with the tests present at - [Test Plan
>> for JEP 382: New macOS Rendering
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396)
>>
>> **Note to reviewers :**
>> 1) Default rendering pipeline on macOS has not been changed by this PR.
>> OpenGL still stays as the default rendering pipeline and Metal rendering
>> pipeline is optional to choose.
>>
>> 2) To apply and test this PR -
>> To enable the metal pipeline you must specify on command line
>> -Dsun.java2d.metal=true (No message will be printed in this case) or
>> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is
>> enabled gets printed)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Lanai PR#175 - 8261304 - aghaisas
> > General comment - I am not sure I like the `MTL` prefix acronym in names
> > (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`).
> > I think you tried to match the `CGL`, but that is a real acronym that
> > stands for "Core Graphics Layer" (I think).
> > `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I
> > suppose, but also just `Metal` would work just fine.
>
> `MTL` is the abbreviation that Apple uses for Metal in all of their APIs. The
> only potential issue I might see with this prefix is in the native code where
> there could be name collisions between Java2D's names and Apple's names.
> Since we haven't run into such a collision, I don't think this needs to
> change, and wouldn't necessarily affect the Java class names anyway. If we
> were to consider it, `METAL` seems better than `ML` (which is too short to be
> descriptive and might suggest machine learning).
If Apple itself uses `MTL` then we are good.
src/java.desktop/macosx/classes/sun/awt/CGraphicsConfig.java line 35:
> 33:
> 34: import sun.java2d.SurfaceData;
> 35: import sun.java2d.opengl.CGLLayer;
Not needed import anymore?
src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 113:
> 111: // This indicates fallback to other rendering pipeline also
> failed.
> 112: // Should never reach here
> 113: throw new InternalError("Error - unable to initialize any
> rendering pipeline.");
There is no software based renderer to fall back here?
src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 66:
> 64:propString.equals("f") ||
> 65:propString.equals("False") ||
> 66:propString.equals("F"))
Shouldn't `1` and `0` be also allowed here?
src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 100:
> 98: oglState = PropertyState.ENABLED; // Enable
> default pipeline
> 99: metalState = PropertyState.DISABLED; // Disable
> non-default pipeline
> 100: }
This matches JEP 382 specification, but even when both GL is `false` and Metal
is `false` we still get GL? There is no software based pipeline anymore?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 131:
> 129: @Native
> 130: static final int CAPS_EXT_GRAD_SHADER = (FIRST_PRIVATE_CAP <<
> 3);
> 131: /** Indicates the presence of the GL_ARB_texture_rectangle
> extension. */
Reference to `GL_ARB_texture_rectangle` extension in Metal pipeline?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLContext.java line 134:
> 132: @Native
> 133: static final int CAPS_EXT_TEXRECT = (FIRST_PRIVATE_CAP <<
> 4);
> 134: /** Indicates the presence of the GL_NV_texture_barrier
> extension. */
Reference to `GL_NV_texture_barrier` extension in Metal pipeline?
src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 97:
> 95: public static void disposeGraphicsConfig(long pConfigInfo) {
> 96: MTLRenderQueue rq = getInstance();
> 97: rq.lock();
Is it allowed to have multiple `MTLRenderQueue` instances?
If not, then I see this pattern everywhere:
MTLRenderQueue rq = getInstance();
rq.lock();
{
...
}
rq.unlock();
why not just do:
MTLRenderQueue.
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Mon, 8 Feb 2021 17:15:25 GMT, Gerard Ziemski wrote: >> The file in `RenderPerfTest` should have a GPLv2 license header (no >> Classpath). I filed >> [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also >> highlighted a couple examples below. > > General comment - I am not sure I like the `MTL` prefix acronym in names (ex. > `sun.java2d.metal.MTLVolatileSurfaceManager`). > > I think you tried to match the `CGL`, but that is a real acronym that stands > for "Core Graphics Layer" (I think). > > `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I > suppose, but also just `Metal` would work just fine. I'm in the process of reviewing this feature, but there is lots of code to go through - please wait for my review. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]
On Sat, 6 Feb 2021 00:53:08 GMT, Kevin Rushforth wrote: >> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Lanai PR#175 - 8261304 - aghaisas > > The file in `RenderPerfTest` should have a GPLv2 license header (no > Classpath). I filed > [JDK-8261273](https://bugs.openjdk.java.net/browse/JDK-8261273) and also > highlighted a couple examples below. General comment - I am not sure I like the `MTL` prefix acronym in names (ex. `sun.java2d.metal.MTLVolatileSurfaceManager`). I think you tried to match the `CGL`, but that is a real acronym that stands for "Core Graphics Layer" (I think). `MTL` on the other hand is no acronym. I can see `ML` for "Metal Layer" I suppose, but also just `Metal` would work just fine. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Fri, 5 Feb 2021 12:26:27 GMT, Anton Kozlov wrote: >> Marked as reviewed by ihse (Reviewer). > >> I haven't got a MacOS AArch64 system right now. Is it possible to >> enable W^X in Linux in order to kick the tyres? > > I've just got rid of asserts that fired on Linux sometime :) As for W^X like > on macOS, I vaguely remember working with a Linux system with one-way > transition W->X, probably provided by SELinux. But I don't think it allowed > per-thread W^X state. > _Mailing list message from [daniel.daugherty at > oracle.com](mailto:daniel.daughe...@oracle.com) on > [security-dev](mailto:security-...@openjdk.java.net):_ > > On 2/5/21 4:51 AM, Magnus Ihse Bursie wrote: > > @magicus - I'm good with both of these answers. I personally like 'macosx'. > > Dan It's no longer `macosx`, it's just `macos` now - see https://en.wikipedia.org/wiki/MacOS - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 22:09:58 GMT, Daniel D. Daugherty wrote:
>> Anton Kozlov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> support macos_aarch64 in hsdis
>
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 363:
>
>> 361: address pc = os::Posix::ucontext_get_pc(uc);
>> 362:
>> 363: if (pc != addr && uc->context_esr == 0x924F) { //TODO: figure
>> out what this value means
>
> Is this TODO going to be resolved by this port?
Where did this come from - some snippet/example/tech note code? Maybe other
people can help figure it out if we provide more info.
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 374:
>
>> 372:
>> 373: last_addr = (address) -1;
>> 374: } else if (pc == addr && uc->context_esr == 0x820f) { //TODO:
>> figure out what this value means
>
> Is this TODO going to be resolved by this port?
Where did this come from - some snippet/example/tech note code? Maybe other
people can help figure it out if we provide more info.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
> - Add comments to WX transitions
>
>+ minor change of placements
> - Use macro conditionals instead of empty functions
> - Add W^X to tests
> - Do not require known W^X state
> - Revert w^x in gtests
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 403:
> 401: }
> 402:
> 403: return false; // Mute compiler
Is this comment needed?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 420:
> 418: size_t os::Posix::_compiler_thread_min_stack_allowed = 72 * K;
> 419: size_t os::Posix::_java_thread_min_stack_allowed = 72 * K;
> 420: size_t os::Posix::_vm_internal_thread_min_stack_allowed = 72 * K;
Those are slightly larger than their x86_64 counter parts. Are they
conservative/aggressive values? How did we arrive at those?
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 652:
> 650:
> 651: void os::setup_fpu() {
> 652: }
Is there really nothing to do here, or does still need to be implemented? A
clarification comment here would help/.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request incrementally with six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos > - Add comments to WX transitions > >+ minor change of placements > - Use macro conditionals instead of empty functions > - Add W^X to tests > - Do not require known W^X state > - Revert w^x in gtests I reviewed bsd_aarch64.cpp digging bit deeper and left some comments. - Changes requested by gziemski (Committer). PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
> - Add comments to WX transitions
>
>+ minor change of placements
> - Use macro conditionals instead of empty functions
> - Add W^X to tests
> - Do not require known W^X state
> - Revert w^x in gtests
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 297:
> 295: stub = SharedRuntime::handle_unsafe_access(thread, next_pc);
> 296: }
> 297: } else if (sig == SIGILL && nativeInstruction_at(pc)->is_stop()) {
Can we add a comment here describing what this case means?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request incrementally with six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos > - Add comments to WX transitions > >+ minor change of placements > - Use macro conditionals instead of empty functions > - Add W^X to tests > - Do not require known W^X state > - Revert w^x in gtests src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 302: > 300: const uint64_t *detail_msg_ptr > 301: = (uint64_t*)(pc + NativeInstruction::instruction_size); > 302: const char *detail_msg = (const char *)*detail_msg_ptr; Where is `detail_msg` used? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request incrementally with six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos > - Add comments to WX transitions > >+ minor change of placements > - Use macro conditionals instead of empty functions > - Add W^X to tests > - Do not require known W^X state > - Revert w^x in gtests src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 237: > 235: os::Posix::ucontext_set_pc(uc, > StubRoutines::continuation_for_safefetch_fault(pc)); > 236: return true; > 237: } Isn't this case already handled by `JVM_HANDLE_XXX_SIGNAL()` ? Why do we need it here again? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
> - Add comments to WX transitions
>
>+ minor change of placements
> - Use macro conditionals instead of empty functions
> - Add W^X to tests
> - Do not require known W^X state
> - Revert w^x in gtests
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 198:
> 196:
> 197: NOINLINE frame os::current_frame() {
> 198: intptr_t *fp = *(intptr_t **)__builtin_frame_address(0);
In the bsd_x86 counter part we initialize `fp` to `_get_previous_fp()` - do we
need to implement it on aarch64 as well (and using address 0 is just a temp
workaround) or is it doing the right thing here?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
> - Add comments to WX transitions
>
>+ minor change of placements
> - Use macro conditionals instead of empty functions
> - Add W^X to tests
> - Do not require known W^X state
> - Revert w^x in gtests
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 291:
> 289: bool is_unsafe_arraycopy = (thread->doing_unsafe_access() &&
> UnsafeCopyMemory::contains_pc(pc));
> 290: if ((nm != NULL && nm->has_unsafe_access()) ||
> is_unsafe_arraycopy) {
> 291: address next_pc = pc + NativeCall::instruction_size;
Replace
address next_pc = pc + NativeCall::instruction_size;
with
address next_pc = Assembler::locate_next_instruction(pc);
there is at least one other place that needs it.
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 322:
> 320: #ifdef __APPLE__
> 321: } else if (sig == SIGFPE && info->si_code == FPE_NOOP) {
> 322: Unimplemented();
Is there a follow up issue for this?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
> - Add comments to WX transitions
>
>+ minor change of placements
> - Use macro conditionals instead of empty functions
> - Add W^X to tests
> - Do not require known W^X state
> - Revert w^x in gtests
src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 194:
> 192: // may get turned off by -fomit-frame-pointer.
> 193: frame os::get_sender_for_C_frame(frame* fr) {
> 194: return frame(fr->link(), fr->link(), fr->sender_pc());
Why is it
return frame(fr->link(), fr->link(), fr->sender_pc());
and not
return frame(fr->sender_sp(), fr->link(), fr->sender_pc());
like in the bsd-x86 counter part?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 23:13:12 GMT, Bernhard Urban-Forster wrote: >> No idea how to insert spaces and make text align :-( > > using ` ```c ` > https://docs.github.com/en/github/writing-on-github/creating-and-highlighting-code-blocks > > I was wrong about `SIGFPE` / `EXC_MASK_ARITHMETIC`, it's used on i386, x86_64: > https://github.com/openjdk/jdk/blob/2be60e37e0e433141b2e3d3e32f8e638a4888e3a/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp#L467-L524 > and aarch64: > https://github.com/AntonKozlov/jdk/blob/80827176cbc5f0dd26003cf234a8076f3f557928/src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp#L309-L323 > (What happened with the formatting here, ugh?) > > Your suggestion sounds good otherwise. @AntonKozlov, do you mind to integrate > that? So it should be: #if defined(__APPLE__) // lldb (gdb) installs both standard BSD signal handlers, and mach exception // handlers. By replacing the existing task exception handler, we disable lldb's mach // exception handling, while leaving the standard BSD signal handlers functional. // // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking // EXC_MASK_ARITHMETIC needed by all architectures for div by 0 checking // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization kern_return_t kr; kr = task_set_exception_ports(mach_task_self(), EXC_MASK_BAD_ACCESS | EXC_MASK_ARITHMETIC AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION), MACH_PORT_NULL, EXCEPTION_STATE_IDENTITY, MACHINE_THREAD_STATE); assert(kr == KERN_SUCCESS, "could not set mach task signal handler"); #endif - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 22:44:18 GMT, Gerard Ziemski wrote: >> Thanks for your questions Gerard. >> >>> Part of the comment said This work-around is not necessary for 10.5+, as >>> CrashReporter no longer intercedes on caught fatal signals. >> >> That comment can probably be deleted since minversion is anyway 10.9 (and >> soon 10.12 https://github.com/openjdk/jdk/pull/2268 ). >> >>> Do you know if this also apply to lldb or is it gdb only specific? How do >>> you run gdb on macOS nowadays anyhow? >> >> `lldb` is shipped with Xcode, `gdb` isn't. You would need to build and sign >> it yourself, I haven't tried that in a while. So, we should update that >> comment to talk about `lldb` 🙂 >> >>> a) why we need `EXC_MASK_ARITHMETIC` ? >> >> I _believe_ this dates back to i386. As far as I can tell this isn't needed >> for x86_64 or aarch64. I guess we can remove it, the worst case is that it >> makes the debugging experience of the runtime a little bit worse. OTOH it >> doesn't hurt either to have it here. >> >>> b) we hit signal SIGSEGV in debugger even with the code in place, any way >>> to avoid that? >> >> The equivalent for `handle SIGSEGV nostop noprint` (gdb) in lldb is `process >> handle -n false -p true -s false SIGSEGV`. >> >>> c) does `BSD aarch6` need only `EXC_MASK_BAD_INSTRUCTION` or does it need >>> `EXC_MASK_BAD_ACCESS` as well? >> >> aarch64 needs `EXC_MASK_BAD_ACCESS` at least for implicit null checking, >> there might be other cases. >> >>> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to >>> apply to `aarch64`? >> >> Maybe. I don't see any value in it though, except making the code more >> complicated to read 🙂 > > I don't like the idea of using masks on architectures that do not require > them. How about something like this? > > `#if defined(__APPLE__)` > ` // lldb (gdb) installs both standard BSD signal handlers, and mach > exception` > ` // handlers. By replacing the existing task exception handler, we disable > lldb's mach` > ` // exception handling, while leaving the standard BSD signal handlers > functional.` > ` //` > ` // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking` > ` // EXC_MASK_ARITHMETIC needed by i386` > ` // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization` > ` kern_return_t kr;` > ` kr = task_set_exception_ports(mach_task_self(),` > `EXC_MASK_BAD_ACCESS` > `NOT_LP64(| EXC_MASK_ARITHMETIC)` > `AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),` > `MACH_PORT_NULL,` > `EXCEPTION_STATE_IDENTITY,` > `MACHINE_THREAD_STATE);` > ` ` > ` assert(kr == KERN_SUCCESS, "could not set mach task signal handler");` > `#endif` > > If I just knew why i386 needs `EXC_MASK_ARITHMETIC` and add that to the > comment I would be personally happy with that chunk of code. No idea how to insert spaces and make text align :-( - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 22:17:02 GMT, Bernhard Urban-Forster wrote: >> To answer my own question, it seems that code is still needed on `x86_64` >> for `lldb` with `EXC_MASK_BAD_ACCESS` or we keep tripping over >> `EXC_BAD_ACCESS` >> >> Remaining questions: >> >> a) why we need `EXC_MASK_ARITHMETIC` ? >> b) we hit `signal SIGSEGV` in debugger even with the code in place, any way >> to avoid that? >> c) does `BSD aarch64` need only `EXC_MASK_BAD_INSTRUCTION` or does it need >> `EXC_MASK_BAD_ACCESS` as well? >> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to >> apply to `aarch64`? > > Thanks for your questions Gerard. > >> Part of the comment said This work-around is not necessary for 10.5+, as >> CrashReporter no longer intercedes on caught fatal signals. > > That comment can probably be deleted since minversion is anyway 10.9 (and > soon 10.12 https://github.com/openjdk/jdk/pull/2268 ). > >> Do you know if this also apply to lldb or is it gdb only specific? How do >> you run gdb on macOS nowadays anyhow? > > `lldb` is shipped with Xcode, `gdb` isn't. You would need to build and sign > it yourself, I haven't tried that in a while. So, we should update that > comment to talk about `lldb` 🙂 > >> a) why we need `EXC_MASK_ARITHMETIC` ? > > I _believe_ this dates back to i386. As far as I can tell this isn't needed > for x86_64 or aarch64. I guess we can remove it, the worst case is that it > makes the debugging experience of the runtime a little bit worse. OTOH it > doesn't hurt either to have it here. > >> b) we hit signal SIGSEGV in debugger even with the code in place, any way to >> avoid that? > > The equivalent for `handle SIGSEGV nostop noprint` (gdb) in lldb is `process > handle -n false -p true -s false SIGSEGV`. > >> c) does `BSD aarch6` need only `EXC_MASK_BAD_INSTRUCTION` or does it need >> `EXC_MASK_BAD_ACCESS` as well? > > aarch64 needs `EXC_MASK_BAD_ACCESS` at least for implicit null checking, > there might be other cases. > >> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to >> apply to `aarch64`? > > Maybe. I don't see any value in it though, except making the code more > complicated to read 🙂 I don't like the idea of using masks on architectures that do not require them. How about something like this? `#if defined(__APPLE__)` ` // lldb (gdb) installs both standard BSD signal handlers, and mach exception` ` // handlers. By replacing the existing task exception handler, we disable lldb's mach` ` // exception handling, while leaving the standard BSD signal handlers functional.` ` //` ` // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking` ` // EXC_MASK_ARITHMETIC needed by i386` ` // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization` ` kern_return_t kr;` ` kr = task_set_exception_ports(mach_task_self(),` `EXC_MASK_BAD_ACCESS` `NOT_LP64(| EXC_MASK_ARITHMETIC)` `AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),` `MACH_PORT_NULL,` `EXCEPTION_STATE_IDENTITY,` `MACHINE_THREAD_STATE);` ` ` ` assert(kr == KERN_SUCCESS, "could not set mach task signal handler");` `#endif` If I just knew why i386 needs `EXC_MASK_ARITHMETIC` and add that to the comment I would be personally happy with that chunk of code. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 20:04:18 GMT, Gerard Ziemski wrote: >> See comment above about `gdb`, the same applies to `lldb` today. The AArch64 >> backend uses `SIGILL` (~= `EXC_MASK_BAD_INSTRUCTION`) to initiate a >> deoptimization. Without this change you cannot continue debugging once you >> the debuggee receives `SIGILL`. This wasn't needed before as x86 doesn't use >> `SIGILL`. > > Part of the comment said `This work-around is not necessary for 10.5+, as > CrashReporter no longer intercedes on caught fatal signals.` so I thought it > was no longer needed, but it sounds like the part about `gdb` still applies > then. > > We should update the comment to just say the `gdb` relevant part perhaps (and > evaluate which of the EXC_MASK_BAD_ACCESS | EXC_MASK_BAD_INSTRUCTION | > EXC_MASK_ARITHMETIC) we actually need for gdb: > > `// gdb installs both standard BSD signal handlers, and mach exception` > `// handlers. By replacing the existing task exception handler, we disable > gdb's mach` > `// exception handling, while leaving the standard BSD signal handlers > functional.` > > Do you know if this also apply to `lldb` or is it `gdb` only specific? How do > you run `gdb` on macOS nowadays anyhow? To answer my own question, it seems that code is still needed on `x86_64` for `lldb` with `EXC_MASK_BAD_ACCESS` or we keep tripping over `EXC_BAD_ACCESS` Remaining questions: a) why we need `EXC_MASK_ARITHMETIC` ? b) we hit `signal SIGSEGV` in debugger even with the code in place, any way to avoid that? c) does `BSD aarch64` need only `EXC_MASK_BAD_INSTRUCTION` or does it need `EXC_MASK_BAD_ACCESS` as well? d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to apply to `aarch64`? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 19:23:16 GMT, Bernhard Urban-Forster wrote: >> src/hotspot/os/posix/signals_posix.cpp line 1297: >> >>> 1295: kern_return_t kr; >>> 1296: kr = task_set_exception_ports(mach_task_self(), >>> 1297: EXC_MASK_BAD_ACCESS | >>> EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC, >> >> Could someone elaborate on why we need to add `EXC_MASK_BAD_INSTRUCTION` to >> the mask here? > > See comment above about `gdb`, the same applies to `lldb` today. The AArch64 > backend uses `SIGILL` (~= `EXC_MASK_BAD_INSTRUCTION`) to initiate a > deoptimization. Without this change you cannot continue debugging once you > the debuggee receives `SIGILL`. This wasn't needed before as x86 doesn't use > `SIGILL`. Part of the comment said `This work-around is not necessary for 10.5+, as CrashReporter no longer intercedes on caught fatal signals.` so I thought it was no longer needed, but it sounds like the part about `gdb` still applies then. We should update the comment to just say the `gdb` relevant part perhaps (and evaluate which of the EXC_MASK_BAD_ACCESS | EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC) we actually need for gdb: `// gdb installs both standard BSD signal handlers, and mach exception` `// handlers. By replacing the existing task exception handler, we disable gdb's mach` `// exception handling, while leaving the standard BSD signal handlers functional.` Do you know if this also apply to `lldb` or is it `gdb` only specific? How do you run `gdb` on macOS nowadays anyhow? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 18:52:29 GMT, Gerard Ziemski wrote: >> Anton Kozlov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> support macos_aarch64 in hsdis > > Changes requested by gziemski (Committer). There were bunch of assembly code that I couldn't review. I also shallow reviewed the brand new files that were copies of the existing BSD x86_64 files - I hope I can get back to those when I figure out how to build/run these changes. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 11:59:08 GMT, Anton Kozlov wrote:
>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>>
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and
>> windows/aarch64.
>>
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X),
>> required on macOS/AArch64 platform. It's implemented with
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a
>> thread, so W^X mode change relates to the java thread state change (for java
>> threads). In most cases, JVM executes in write-only mode, except when
>> calling a generated stub like SafeFetch, which requires a temporary switch
>> to execute-only mode. The same execute-only mode is enabled when a java
>> thread executes in java or native states. This approach of managing W^X mode
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> support macos_aarch64 in hsdis
Changes requested by gziemski (Committer).
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 390:
> 388: store_and_inc(_to, from_obj, NativeStack::intSpace);
> 389:
> 390: _num_int_args++;
`pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the `if
else` but other methods use 2 of them inside `if else` branches.
We should be consistent.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 404:
> 402: } else {
> 403: store_and_inc(_to, from_obj, NativeStack::longSpace);
> 404: _num_int_args++;
`pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the `if
else` but other methods use it 2 of them inside `if else`
We should be consistent.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 418:
> 416: } else {
> 417: store_and_inc(_to, (*from_addr == 0) ? (intptr_t)NULL : (intptr_t)
> from_addr, wordSize);
> 418: _num_int_args++;
`pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the `if
else` but other methods use it 2 of them inside `if else`
We should be consistent.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 433:
> 431: store_and_inc(_to, from_obj, NativeStack::floatSpace);
> 432:
> 433: _num_fp_args++;
`pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the `if
else` but other methods use it 2 of them inside `if else`
We should be consistent.
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 448:
> 446: } else {
> 447: store_and_inc(_to, from_obj, NativeStack::doubleSpace);
> 448: _num_fp_args++;
`pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the `if
else` but other methods use it 2 of them inside `if else`
We should be consistent.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5271:
> 5269: //
> 5270: void MacroAssembler::get_thread(Register dst) {
> 5271: RegSet saved_regs = RegSet::range(r0, r1) +
> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
The comment needs to be updated, since on BSD we also seem to clobber r2,r17 ?
src/hotspot/os/posix/signals_posix.cpp line 1297:
> 1295: kern_return_t kr;
> 1296: kr = task_set_exception_ports(mach_task_self(),
> 1297: EXC_MASK_BAD_ACCESS |
> EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC,
Could someone elaborate on why we need to add `EXC_MASK_BAD_INSTRUCTION` to the
mask here?
src/hotspot/cpu/aarch64/vm_version_aarch64.hpp line 93:
> 91: CPU_MARVELL = 'V',
> 92: CPU_INTEL = 'i',
> 93: CPU_APPLE = 'a',
The `ARM Architecture Reference Manual ARMv8, for ARMv8-A architecture profile`
has 8538 pages, can we be more specific and point to the particular section of
the document where the CPU codes are defined?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Mon, 1 Feb 2021 20:10:58 GMT, Ioi Lam wrote: >> - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which >> allowed the same JDK library to use different version of HotSpot. However, >> HSX is no longer supported so this API should be removed. >> - Implementations of APIs such as JVM_DTraceActivate, were removed in >> [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their >> declarations should be removed from jvm.h > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed macos build Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Mon, 1 Feb 2021 20:54:42 GMT, Ioi Lam wrote:
>> src/java.base/share/native/libjava/check_version.c line 33:
>>
>>> 31: DEF_JNI_OnLoad(JavaVM *vm, void *reserved)
>>> 32: {
>>> 33: return JNI_VERSION_1_2;
>>
>> This leaves an entire file with one trivial function implementation. Can we
>> remove the file and implement `DEF_JNI_OnLoad()` in `jni_util.h` (or some
>> other existing suitable file) ?
>
> I am not sure if jni_utils.c is the right file (it defines the `JNU_XXX`
> functions that are used by other shared libraries).
>
> There are other .c files that have trivial `DEF_JNI_OnLoad` functions (e.g.,
> java.base/share/native/libnio/nio_util.c).
>
> @AlanBateman do you have any suggestions?
I'm fine with the way it is, just thought we might want to consider cleaning up
a bit more, since it's a cleanup task itself.
-
PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Mon, 1 Feb 2021 20:10:58 GMT, Ioi Lam wrote:
>> - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which
>> allowed the same JDK library to use different version of HotSpot. However,
>> HSX is no longer supported so this API should be removed.
>> - Implementations of APIs such as JVM_DTraceActivate, were removed in
>> [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their
>> declarations should be removed from jvm.h
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> fixed macos build
Changes requested by gziemski (Committer).
src/java.base/share/native/libjava/check_version.c line 33:
> 31: DEF_JNI_OnLoad(JavaVM *vm, void *reserved)
> 32: {
> 33: return JNI_VERSION_1_2;
This leaves an entire file with one trivial function implementation. Can we
remove the file and implement `DEF_JNI_OnLoad()` in `jni_util.h` (or some
other existing suitable file) ?
-
PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Mon, 1 Feb 2021 18:31:23 GMT, Gerard Ziemski wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8260616: Removing remaining JNF dependencies in the java.desktop modul > > Marked as reviewed by gziemski (Committer). The changes look good to me, but please see my comment from my review about documenting `NormalizedPathNSStringFromJavaString()` API. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 19:53:32 GMT, Phil Race wrote: >> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83: >> >>> 81: stringWithFileSystemRepresentation:chs length:len]; >>> 82: return result; >>> 83: } >> >> Why are we doing: >> >> `java_string -> chars -> NSString -> chars -> [NSFileManager >> stringWithFileSystemRepresentation]` >> >> ? >> >> Why not just get the raw characters form JNI and feed them directly into >> `[NSFileManager stringWithFileSystemRepresentation]`, ie: >> >> `java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]` >> >> and skip the `JavaStringToNSString` step altogether? > > I tried to explain the odd-ness of this in the comments preceding the > function definition. > Near as I could figure out that intermediate call is needed to do the > decomposition since the NSFileManager won't do that. > But this is not exactly my area of expertise and there may be a better way. > Who is an expert on the intricacies of the macOS file system naming ? How about a comment like: /* Returns an NSString in decomposed UTF16 format that is compatible with HFS's expectation of the UTF16 format for file system paths. Example string: "/Users/Amélie/" Java's UTF16 string is "/ U s e r s / A m \351 l i e /" macOS UTF16 string suitable for HFS is "/ U s e r s / A m e \314 \201 l i e /" There is no direct API that takes in NSString UTF16 encoded by Java and produces NSString UTF16 for HFS, so we first need to decompose it into chars (suitable for low level C file APIs), and only then create NSString representation of this decomposition back into UTF16 string. */ ? I borrowed the API description from Apple's original `JNFNormalizedNSStringForPath` API, but added an example and different description. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 17:24:05 GMT, Phil Race wrote: >> This completes the desktop module JNF removal >> >> * remove -framework JavaNativeFoundation from make files >> >> * remove #import from all >> source files. If needed add import of JNIUtilities.h to get jni.h >> definitions - better anyway since then it gets the current JDK ones not the >> ones from the O/S >> >> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString >> with JavaStringToNSString >> >> * replace JNFNormalizedNSStringForPath with >> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with >> NormalizedPathJavaStringFromNSString >> >> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI >> >> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the >> vast majority already did this) >> >> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject >> perform* methods. >> >> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and >> use where needed. >> >> * Remove the single usage of JNFPerformEnvBlock >> >> * replace JNFJavaToNSNumber in single A11Y file with local replacement >> >> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with >> local replacement >> >> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m >> >> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8260616: Removing remaining JNF dependencies in the java.desktop modul Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 16:57:09 GMT, Gerard Ziemski wrote: >> This completes the desktop module JNF removal >> >> * remove -framework JavaNativeFoundation from make files >> >> * remove #import from all >> source files. If needed add import of JNIUtilities.h to get jni.h >> definitions - better anyway since then it gets the current JDK ones not the >> ones from the O/S >> >> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString >> with JavaStringToNSString >> >> * replace JNFNormalizedNSStringForPath with >> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with >> NormalizedPathJavaStringFromNSString >> >> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI >> >> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the >> vast majority already did this) >> >> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject >> perform* methods. >> >> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and >> use where needed. >> >> * Remove the single usage of JNFPerformEnvBlock >> >> * replace JNFJavaToNSNumber in single A11Y file with local replacement >> >> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with >> local replacement >> >> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m >> >> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) > > Changes requested by gziemski (Committer). Lots of small changes you had to handle here. Thank you for doing it! - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 00:30:21 GMT, Phil Race wrote:
> This completes the desktop module JNF removal
>
> * remove -framework JavaNativeFoundation from make files
>
> * remove #import from all
> source files. If needed add import of JNIUtilities.h to get jni.h definitions
> - better anyway since then it gets the current JDK ones not the ones from the
> O/S
>
> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString
> with JavaStringToNSString
>
> * replace JNFNormalizedNSStringForPath with
> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with
> NormalizedPathJavaStringFromNSString
>
> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI
>
> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast
> majority already did this)
>
> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject
> perform* methods.
>
> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and
> use where needed.
>
> * Remove the single usage of JNFPerformEnvBlock
>
> * replace JNFJavaToNSNumber in single A11Y file with local replacement
>
> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with
> local replacement
>
> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m
>
> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION)
Changes requested by gziemski (Committer).
src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 608:
> 606: {
> 607: // Get string to draw and the length
> 608: const jchar *unichars = JNFGetStringUTF16UniChars(env, str);
According to `JNFString.h`, the `JNFGetStringUTF16UniChars()` API:
/*
* Gets UTF16 unichars from a Java string, and checks for errors and raises a
JNFException if
* the unichars cannot be obtained from Java.
*/
raises a (JNFException) if it can't get the chars, but now we simply return if
`GetStringChars()` can not return the chars.
Seems like we handle this case differently in the new code now - is this change
desired and correct?
src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 601:
> 599: jchar unichars[len];
> 600: (*env)->GetStringRegion(env, str, 0, len, unichars);
> 601: CHECK_EXCEPTION();
Are `JNF_CHECK_AND_RETHROW_EXCEPTION` and `CHECK_EXCEPTION` equivalent?
`JNF_CHECK_AND_RETHROW_EXCEPTION` seems to throw exception, but
`CHECK_EXCEPTION` does not?
src/java.desktop/macosx/native/libosxui/ScreenMenu.m line 165:
> 163: */
> 164: static jlong NSTimeIntervalToJavaMilliseconds(NSTimeInterval interval) {
> 165: NSTimeInterval interval1970 = interval + NSTimeIntervalSince1970;
Is it required for the APIs using the value from this function to expect the
time calculated since Jan 1st 1970?
src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83:
> 81: stringWithFileSystemRepresentation:chs length:len];
> 82: return result;
> 83: }
Why are we doing:
`java_string -> chars -> NSString -> chars -> [NSFileManager
stringWithFileSystemRepresentation]`
?
Why not just get the raw characters form JNI and feed them directly into
`[NSFileManager stringWithFileSystemRepresentation]`, ie:
`java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
and skip the `JavaStringToNSString` step altogether?
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC [v5]
On Thu, 21 Jan 2021 05:24:09 GMT, David Holmes wrote: >> We are now confident that we have build-time and runtime support for >> clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX >> platforms - see bug report for some more details on different OS. >> Consequently we can simplify a lot of the code in this area and move common >> code to os_posix. >> >> As of glibc 2.17 the necessary functions are in glibc rather than librt, but >> we (Oracle at least) aren't yet in position to set our minimum Linux version >> to support that. We still have supported platforms at glibc 2.12. So to >> address that we link librt at build time on Linux. This seems to work find >> for older and more modern Linuxes and also works for the Apline Linux with >> Musl variant. >> >> The changes are in layered commits: >> >> Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true >> and removed. >> Step 2: make supports_monotonic_clock always true and so remove checking in >> OS code >> Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME) >> Step 4: Move shared time functions to os_posix.cpp >> Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17 >> >> Testing: tiers 1-3 for functional testing >> built and checked (-Xlog:os) on Linux with glibc 2.12 and 2.17, >> macOS 10.13.6 and 10.15.7 > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Remove the always true os::supports_monotonic_clock() Marked as reviewed by gziemski (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2090
Re: RFR: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC [v3]
On Tue, 19 Jan 2021 01:53:03 GMT, David Holmes wrote:
>> We are now confident that we have build-time and runtime support for
>> clock_gettime and CLOCK_MONOTONIC on all our OpenJDK supported POSIX
>> platforms - see bug report for some more details on different OS.
>> Consequently we can simplify a lot of the code in this area and move common
>> code to os_posix.
>>
>> As of glibc 2.17 the necessary functions are in glibc rather than librt, but
>> we (Oracle at least) aren't yet in position to set our minimum Linux version
>> to support that. We still have supported platforms at glibc 2.12. So to
>> address that we link librt at build time on Linux. This seems to work find
>> for older and more modern Linuxes and also works for the Apline Linux with
>> Musl variant.
>>
>> The changes are in layered commits:
>>
>> Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true
>> and removed.
>> Step 2: make supports_monotonic_clock always true and so remove checking in
>> OS code
>> Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
>> Step 4: Move shared time functions to os_posix.cpp
>> Step 5: Alway link librt on Linux so we don't rely on glibc > 2.17
>>
>> Testing: tiers 1-3 for functional testing
>> built and checked (-Xlog:os) on Linux with glibc 2.12 and 2.17,
>> macOS 10.13.6 and 10.15.7
>
> David Holmes 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 eight additional
> commits since the last revision:
>
> - Merge branch 'master' into 8246112-mono
> - Restrict librt linking to JVM - per Magnus's request
> - Merge branch 'master' into 8246112-mono
> - Alway link librt on Linux so we don't rely on glibc > 2.12
> - Step 4: Move shared time functions to os_posix.cpp
> - Step 3: Replace gettimeofday by clock_gettime(CLOCK_REALTIME)
> - Step 2: make supports_monotonic_clock always true and so remove checking
> it (in OS code)
> - 8246112: Remove build-time and run-time checks for clock_gettime and
> CLOCK_MONOTONIC
>
>Step 1: Remove build time checks. SUPPORTS_CLOCK_MONOTONIC is assumed true
> and removed.
Changes requested by gziemski (Committer).
src/hotspot/os/bsd/os_bsd.inline.hpp line 2:
> 1: /*
> 2: * Copyright (c) 1999, 2030, Oracle and/or its affiliates. All rights
> reserved.
Year 2030 ?
src/hotspot/os/bsd/os_bsd.cpp line 828:
> 826: }
> 827:
> 828: void os::javaTimeNanos_info(jvmtiTimerInfo *info_ptr) {
How come we need `os::javaTimeNanos_info` here if there is already one in
`os_posix.cpp`?
src/hotspot/os/posix/os_posix.hpp line 95:
> 93: static voiducontext_set_pc(ucontext_t* ctx, address pc);
> 94:
> 95: static bool supports_monotonic_clock();
Why do we need this API at all now?
thread.cpp uses it here:
assert(!os::supports_monotonic_clock(),
"unexpected time moving backwards detected in
JavaThread::sleep()");
so we can just remove this usage?
src/hotspot/os/posix/os_posix.cpp line 1161:
> 1159:
> 1160: void os::Posix::init_2(void) {
> 1161: log_info(os)("Use of CLOCK_MONOTONIC is supported");
We are keeping this output to avoid breaking whomever might be looking for this
text?
-
PR: https://git.openjdk.java.net/jdk/pull/2090
Re: RFR: 8205197: Never default to using libc++ on Linux
Thank you for fixing this. > On Jun 18, 2018, at 7:36 PM, Martin Buchholz wrote: > > 8205197: Never default to using libc++ on Linux > http://cr.openjdk.java.net/~martin/webrevs/jdk/stlib-default/ > https://bugs.openjdk.java.net/browse/JDK-8205197 >
Re: RFR: JDK-8196724: Change macosx deployment target to 10.9
hi Erik, Thanks for doing this. I like how you are using a narrow mechanism to turn off only those warnings that come up due to deprecated APIs. Just a quick verification question (not very familiar with the makefiles), in line like this: DISABLED_WARNINGS_clang := deprecated-declarations I assume we turn "deprecated-declarations” into “-Wdeprecated-declarations” flag that then gets passed to the compiler? cheers > On Apr 4, 2018, at 1:30 PM, Erik Joelsson wrote: > > This patch changes the values for the macosx version min and max settings > from 10.7 to 10.9. It also changes the stdlib from libstdc++ to libc++ > (explicitly for Hotspot and implicitly everywhere else). This change is > necessary to keep up with newer toolchain versions on Macosx where using the > old and no longer maintained libstdc++ has been deprecated. This is done in > preparation for bumping the preferred Xcode version used for builds at Oracle. > > The switch has been tested for both Hotspot and client. > > The switch triggered some new deprecation warnings which have been silenced > and followup bugs have been filed on the concerned team. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8196724 > > Webrev: http://cr.openjdk.java.net/~erikj/8196724/webrev.01/index.html > > /Erik >
Re: RFR 8173715: Remove Flat Profiler
> On Aug 22, 2017, at 9:10 PM, David Holmes wrote: > > On 22/08/2017 11:36 PM, Gerard Ziemski wrote: >> Thank you for the review. >>> On Aug 21, 2017, at 8:01 AM, Erik Joelsson wrote: >>> >>> Build changes look good. >>> >>> Don't forget to regenerate configure and push both the closed and open >>> generated-configure script with this change. >> I’m not 100% sure which files we’re talking about, can you point out the >> files you just referred to please? > > When you change a .m4 file you have to regenerate the generated-configure.sh > scripts. With a full forest (open and closed) you would do: > > #> (cd common/autoconf/ && sh autogen.sh) > > which will generate open and closed generated-configure.sh files, which will > have to committed and pushed with the change. You'll need to do this on a > linux system (most likely) with autoconf 2.69 available. Thank you for the explanation - I did this on my Mac. cheers
Re: RFR 8173715: Remove Flat Profiler
Thank you for the review. > On Aug 21, 2017, at 8:01 AM, Erik Joelsson wrote: > > Build changes look good. > > Don't forget to regenerate configure and push both the closed and open > generated-configure script with this change. I’m not 100% sure which files we’re talking about, can you point out the files you just referred to please? > > /Erik > > > On 2017-08-18 23:09, Gerard Ziemski wrote: >> hi all, >> >> The FlatProfiler (i.e. -Xprof) has been deprecated in jdk9, and now it’s the >> time to remove the code and obsolete the -Xprof flag. >> >> issue: https://bugs.openjdk.java.net/browse/JDK-8173715 >> jdk repo webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_repo/ >> hotspot webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_hotspot/ >> >> Specific files to review: >> >> http://cr.openjdk.java.net/~gziemski/8173715_rev2_repo/common/autoconf/hotspot.m4.udiff.html >> http://cr.openjdk.java.net/~gziemski/8173715_rev2_hotspot/make/lib/JvmFeatures.gmk.udiff.html >> >
RFR 8173715: Remove Flat Profiler
hi all, The FlatProfiler (i.e. -Xprof) has been deprecated in jdk9, and now it’s the time to remove the code and obsolete the -Xprof flag. issue: https://bugs.openjdk.java.net/browse/JDK-8173715 jdk repo webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_repo/ hotspot webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_hotspot/ Specific files to review: http://cr.openjdk.java.net/~gziemski/8173715_rev2_repo/common/autoconf/hotspot.m4.udiff.html http://cr.openjdk.java.net/~gziemski/8173715_rev2_hotspot/make/lib/JvmFeatures.gmk.udiff.html
Re: RFR: 8086005: Define __STDC_xxx_MACROS config macros globally via build system
hi Kim, I’m withdrawing my objection. Making this mechanism cover ldflags is beyond the scope of this fix and deserves its own feature request. cheers > On Jun 8, 2017, at 4:21 PM, Gerard Ziemski wrote: > > hi Kim, > > My understanding is that to enable c++11, for example, we need to do 2 things > (at least on Mac OS X): > > #1 For the compilation phase we need to add “-std=c++11 -stdlib=libc++”, > where “-std=c++11” selects the language model, and “-stdlib=libc++” selects > the corresponding headers. > > #2 For the linking phase we need to add "-stdlib=libc++” to select the > corresponding c++ standard lib. > > Ie, we need to set both cflags and ldflags, but you are only allowing to add > to JVM_CFLAGS. Without the ability to also modify JVM_LDFLAGS, this fix, as > is, is not complete on Mac OS X. > > Unless I’m mistaken, please correct me if I’m wrong, can we include modifying > JVM_LDFLAGS in this fix as well? > > > cheers > > >> On Jun 7, 2017, at 6:51 PM, Kim Barrett wrote: >> >> Please review this change to the build of hotspot to globally define >> the __STDC_xxx_MACROS macros via the command line, rather than >> via #defines scattered through several header files. >> >> CR: >> https://bugs.openjdk.java.net/browse/JDK-8086005 >> >> Webrev: >> http://cr.openjdk.java.net/~kbarrett/8086005/hs.00/ >> http://cr.openjdk.java.net/~kbarrett/8086005/hotspot.00/ >> >> Testing: >> JPRT >> >
Re: RFR: 8086005: Define __STDC_xxx_MACROS config macros globally via build system
hi Kim, My understanding is that to enable c++11, for example, we need to do 2 things (at least on Mac OS X): #1 For the compilation phase we need to add “-std=c++11 -stdlib=libc++”, where “-std=c++11” selects the language model, and “-stdlib=libc++” selects the corresponding headers. #2 For the linking phase we need to add "-stdlib=libc++” to select the corresponding c++ standard lib. Ie, we need to set both cflags and ldflags, but you are only allowing to add to JVM_CFLAGS. Without the ability to also modify JVM_LDFLAGS, this fix, as is, is not complete on Mac OS X. Unless I’m mistaken, please correct me if I’m wrong, can we include modifying JVM_LDFLAGS in this fix as well? cheers > On Jun 7, 2017, at 6:51 PM, Kim Barrett wrote: > > Please review this change to the build of hotspot to globally define > the __STDC_xxx_MACROS macros via the command line, rather than > via #defines scattered through several header files. > > CR: > https://bugs.openjdk.java.net/browse/JDK-8086005 > > Webrev: > http://cr.openjdk.java.net/~kbarrett/8086005/hs.00/ > http://cr.openjdk.java.net/~kbarrett/8086005/hotspot.00/ > > Testing: > JPRT >
Re: RFR(M) 8069540: Remove universal binaries support from hotspot build
Thank you for the review! > On Feb 1, 2016, at 6:12 AM, Magnus Ihse Bursie > wrote: > > On 2016-01-29 18:04, Erik Joelsson wrote: >> (adding build-dev) >> >> Looks good enough to me. > > Looks good to me too. > > /Magnus > >> >> /Erik >> >> On 2016-01-29 17:51, Gerard Ziemski wrote: >>> Hi all (and especially the makefiles experts), >>> >>> This fix removes support for building hotspot universal libraries on Mac OS >>> X and simplifies the makefiles. >>> >>> We are still building Mac OS X hotspot libraries as universal libraries, >>> but with only one architecture (x86_64). The rest of JDK is built as plain >>> single architecture libraries and there is no longer any need for this >>> complexity (since we haven't supported 32bit platform on Mac OS X for a >>> while now) >>> >>> Bug link: https://bugs.openjdk.java.net/browse/JDK-8069540 >>> Webrev: http://cr.openjdk.java.net/~gziemski/8069540_jdk_rev2/ >>> Webrev: http://cr.openjdk.java.net/~gziemski/8069540_hotspot_rev2/ >>> >>> Testing: JPRT + RBT on all platforms. >>> >>> >>> cheers >> >
Re: RFR(M) 8069540: Remove universal binaries support from hotspot build
Thank you for the review! > On Jan 29, 2016, at 11:04 AM, Erik Joelsson wrote: > > (adding build-dev) > > Looks good enough to me. > > /Erik > > On 2016-01-29 17:51, Gerard Ziemski wrote: >> Hi all (and especially the makefiles experts), >> >> This fix removes support for building hotspot universal libraries on Mac OS >> X and simplifies the makefiles. >> >> We are still building Mac OS X hotspot libraries as universal libraries, but >> with only one architecture (x86_64). The rest of JDK is built as plain >> single architecture libraries and there is no longer any need for this >> complexity (since we haven't supported 32bit platform on Mac OS X for a >> while now) >> >> Bug link: https://bugs.openjdk.java.net/browse/JDK-8069540 >> Webrev: http://cr.openjdk.java.net/~gziemski/8069540_jdk_rev2/ >> Webrev: http://cr.openjdk.java.net/~gziemski/8069540_hotspot_rev2/ >> >> Testing: JPRT + RBT on all platforms. >> >> >> cheers >
Re: RFR (XS) 8033946 - Hotspot build should ignore "ide" folder
hi Bernhard, I will have to look into how I can make it available to the open source code community. cheers On 9/4/2014 4:16 PM, Bernhard Urban wrote: Hi Gerard, On Thu, Sep 4, 2014 at 6:45 PM, Gerard Ziemski mailto:gerard.ziem...@oracle.com>> wrote: For those interested, I have an Xcode project for JDK8 and JDK9 that I am personally actively supporting and using, which is hosted at https://orahub.oraclecorp.com/gerard.ziemski/xcode that is meant to be put in "jdk/hotspot/ide" folder. I would be interested into this Xcode project, but I can't access the URL. Can you make it publicly accessible? Thanks, Bernhard
Re: RFR (XS) 8033946 - Hotspot build should ignore "ide" folder
hi Karen, The Xcode project is just to build hotspot libs, which it then combines with all the other JDK libs and jars built beforehand using command line, to create a complete JDK build that can be ran using Xcode for live debugging hotspot. I have made a deliberate decision, which may have not been the right one, to put the project inside the hotspot folder, so that one can have multiple jdks and Xcode projects at the same time and tightly coupled. A long term vision here is that eventually we may consider providing officially supported projects for modern IDE that build hotspot to the developer community - a "jdk/hotspot/ide" location seemed like a logical choice for storing project that is supposed to go along with hotspot. cheers On 9/4/2014 12:21 PM, Karen Kinnear wrote: Gerard, I'm a bit confused - if you have an ide to build the entire jdk - what happens? I was a bit surprised to see a hotspot specific change? Also - can't you store your favorite IDE project outside of the repository? thanks, Karen On Sep 4, 2014, at 12:45 PM, Gerard Ziemski wrote: hi all, Please review a very small fix that makes hotspot build ignore "ide" folder, which is where local users can store their own favorite IDE projects. For those interested, I have an Xcode project for JDK8 and JDK9 that I am personally actively supporting and using, which is hosted at https://orahub.oraclecorp.com/gerard.ziemski/xcode that is meant to be put in "jdk/hotspot/ide" folder. Summary of fix: Exclude "ide" folder from the makefile that searches for hotspot src files, or otherwise make bails out complaining that it does not know how to handle Xcode project files. Testing: Passes local build on Mac OS X References: bug: https://bugs.openjdk.java.net/browse/JDK-8033946 webrev: http://cr.openjdk.java.net/~gziemski/8033946_rev0/ Thank you!
RFR (XS) 8033946 - Hotspot build should ignore "ide" folder
hi all, Please review a very small fix that makes hotspot build ignore "ide" folder, which is where local users can store their own favorite IDE projects. For those interested, I have an Xcode project for JDK8 and JDK9 that I am personally actively supporting and using, which is hosted at https://orahub.oraclecorp.com/gerard.ziemski/xcode that is meant to be put in "jdk/hotspot/ide" folder. Summary of fix: Exclude "ide" folder from the makefile that searches for hotspot src files, or otherwise make bails out complaining that it does not know how to handle Xcode project files. Testing: Passes local build on Mac OS X References: bug: https://bugs.openjdk.java.net/browse/JDK-8033946 webrev: http://cr.openjdk.java.net/~gziemski/8033946_rev0/ Thank you!
Does anyone have an Xcode project to build Hotspot?
hi guys, I hope this is the right place to ask this question, if not please suggest where else I could ask it. Does anyone here have an Xcode project to build Hotspot? For reference, I have filed https://jbs.oracle.com/bugs/browse/JDK-8007737 to track this issue. cheers
