Re: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v23]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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:

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module

[Gerard Ziemski]

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: [OpenJDK 2D-Dev] RFR: JDK-8196724: Change macosx deployment target to 10.9

[Gerard Ziemski]

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
>