Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Anton Kozlov]

> 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 two additional 
commits since the last revision:

 - Refactor CDS disabling
 - Redo builsys support for aarch64-darwin

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2200/files
  - new: https://git.openjdk.java.net/jdk/pull/2200/files/50b55f66..0c2cb0a3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=01-02

  Stats: 36 lines in 3 files changed: 12 ins; 15 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2200.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 19:38:16 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 two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573:

> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \
> 572: WARNINGS_AS_ERRORS_xlc := false, \
> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \

I see this added here and in another location. What is deprecated ? You really 
need to explain these sorts of things.
I've built JDK using Xcode 12.3 and not had any need for this.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Mon, 25 Jan 2021 19:42:41 GMT, Phil Race  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Refactor CDS disabling
>>  - Redo builsys support for aarch64-darwin
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573:
> 
>> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \
>> 572: WARNINGS_AS_ERRORS_xlc := false, \
>> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \
> 
> I see this added here and in another location. What is deprecated ? You 
> really need to explain these sorts of things.
> I've built JDK using Xcode 12.3 and not had any need for this.

Hello
I believe it was a workaround for issues with xcode 12.2 in early beta days.
Those issues were fixed later in upstream jdk, so most likely we need to remove 
these workarounds.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Mon, 25 Jan 2021 20:54:38 GMT, Vladimir Kempik  wrote:

>> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573:
>> 
>>> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \
>>> 572: WARNINGS_AS_ERRORS_xlc := false, \
>>> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \
>> 
>> I see this added here and in another location. What is deprecated ? You 
>> really need to explain these sorts of things.
>> I've built JDK using Xcode 12.3 and not had any need for this.
>
> Hello
> I believe it was a workaround for issues with xcode 12.2 in early beta days.
> Those issues were fixed later in upstream jdk, so most likely we need to 
> remove these workarounds.

It seems these workarounds are still needed:

jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: 
error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 
10.14 [-Werror,-Wdeprecated-declarations]
bitmapFormat: NSAlphaFirstBitmapFormat | 
NSAlphaNonpremultipliedBitmapFormat
  ^~~~
  NSBitmapFormatAlphaFirst

jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: 
error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 10.12 
[-Werror,-Wdeprecated-declarations]
  styleMask: NSBorderlessWindowMask
 ^~
 NSWindowStyleMaskBorderless

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 21:18:59 GMT, Vladimir Kempik  wrote:

>> Hello
>> I believe it was a workaround for issues with xcode 12.2 in early beta days.
>> Those issues were fixed later in upstream jdk, so most likely we need to 
>> remove these workarounds.
>
> It seems these workarounds are still needed:
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: 
> error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 
> 10.14 [-Werror,-Wdeprecated-declarations]
> bitmapFormat: NSAlphaFirstBitmapFormat | 
> NSAlphaNonpremultipliedBitmapFormat
>   ^~~~
>   NSBitmapFormatAlphaFirst
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: 
> error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 
> 10.12 [-Werror,-Wdeprecated-declarations]
>   styleMask: NSBorderlessWindowMask
>  ^~
>  NSWindowStyleMaskBorderless

Are you doing something somewhere to change the target version of macOS or SDK 
? I had no such problem.
I think we currently target a macOS 10.9 and if you are changing that it would 
need discussion.
If you are changing it only for Mac ARM that may make more sense .. 

And these appear to be just API churn by Apple
NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst

https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc

NSBorderlessWindowMask is replaced by NSWindowStyleMask

https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Mon, 25 Jan 2021 22:22:06 GMT, Phil Race  wrote:

>> It seems these workarounds are still needed:
>> 
>> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39:
>>  error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 
>> 10.14 [-Werror,-Wdeprecated-declarations]
>> bitmapFormat: NSAlphaFirstBitmapFormat | 
>> NSAlphaNonpremultipliedBitmapFormat
>>   ^~~~
>>   NSBitmapFormatAlphaFirst
>> 
>> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34:
>>  error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 
>> 10.12 [-Werror,-Wdeprecated-declarations]
>>   styleMask: NSBorderlessWindowMask
>>  ^~
>>  NSWindowStyleMaskBorderless
>
> Are you doing something somewhere to change the target version of macOS or 
> SDK ? I had no such problem.
> I think we currently target a macOS 10.9 and if you are changing that it 
> would need discussion.
> If you are changing it only for Mac ARM that may make more sense .. 
> 
> And these appear to be just API churn by Apple
> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst
> 
> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc
> 
> NSBorderlessWindowMask is replaced by NSWindowStyleMask
> 
> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ

Min_macos version is changed to 11.0 for macos_aarch64

https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 22:25:48 GMT, Vladimir Kempik  wrote:

>> Are you doing something somewhere to change the target version of macOS or 
>> SDK ? I had no such problem.
>> I think we currently target a macOS 10.9 and if you are changing that it 
>> would need discussion.
>> If you are changing it only for Mac ARM that may make more sense .. 
>> 
>> And these appear to be just API churn by Apple
>> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst
>> 
>> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc
>> 
>> NSBorderlessWindowMask is replaced by NSWindowStyleMask
>> 
>> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ
>
> Min_macos version is changed to 11.0 for macos_aarch64
> 
> https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136

1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
2) So maybe rather than the deprecation suppression  you could change both 
constants to the new ones.
Ordinarily I'd say let someone else do that but this looks like a simple 
obvious change and is dwarfed by all the other changes going on for Mac ARM ...

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Mon, 25 Jan 2021 22:42:40 GMT, Phil Race  wrote:

>> Min_macos version is changed to 11.0 for macos_aarch64
>> 
>> https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136
>
> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
> 2) So maybe rather than the deprecation suppression  you could change both 
> constants to the new ones.
> Ordinarily I'd say let someone else do that but this looks like a simple 
> obvious change and is dwarfed by all the other changes going on for Mac ARM 
> ...

that sounds good to me, I am just afraid to break intel mac on older macos 
versions with this change.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Chris Plummer]

On Mon, 25 Jan 2021 19:38:16 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 two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

I looked through the SA changes. Overall looks good except for a couple of 
minor issues noted. For the most part it seems to have been boilerplate 
copy-n-paste from existing ports. If there is anything you want me to take a 
closer look at, let me know.

src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 702:

> 700:   primitiveArray = (*env)->GetLongArrayElements(env, registerArray, 
> NULL);
> 701: 
> 702: #undef REG_INDEX

I'm not so sure why the #undef and subsequent #define of REG_INDEX is needed 
since it seems to just get #define'd back to the same value.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/aarch64/BsdAARCH64ThreadContext.java
 line 2:

> 1: /*
> 2:  * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved.

Update copyright.

-

Marked as reviewed by cjplummer (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Chris Plummer]

On Mon, 25 Jan 2021 19:38:16 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 two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

JDI changes also look good.

-

Marked as reviewed by cjplummer (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 22:47:33 GMT, Vladimir Kempik  wrote:

>> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
>> 2) So maybe rather than the deprecation suppression  you could change both 
>> constants to the new ones.
>> Ordinarily I'd say let someone else do that but this looks like a simple 
>> obvious change and is dwarfed by all the other changes going on for Mac ARM 
>> ...
>
> that sounds good to me, I am just afraid to break intel mac on older macos 
> versions with this change.

That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
conflict with the 10.9 target.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Phil Race]

On Mon, 25 Jan 2021 23:34:04 GMT, Phil Race  wrote:

>> that sounds good to me, I am just afraid to break intel mac on older macos 
>> versions with this change.
>
> That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
> conflict with the 10.9 target.

Maybe you should just file a bug after all for this to be dealt with separately.
Certainly if it is NOT fixed now such a bug needs to be filed.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Mon, 25 Jan 2021 23:35:52 GMT, Phil Race  wrote:

>> That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
>> conflict with the 10.9 target.
>
> Maybe you should just file a bug after all for this to be dealt with 
> separately.
> Certainly if it is NOT fixed now such a bug needs to be filed.

I have created https://bugs.openjdk.java.net/browse/JDK-8260402 which is 
blocked by jep-391 currently

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Stefan Karlsson]

On Mon, 25 Jan 2021 19:38:16 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 two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

> * 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.

I wonder if this is the right choice. I think it would have been good if this 
could have been completely hidden in the transition classes. However, that 
doesn't seem to have been enough and now there are explicit 
Thread::WXWriteFromExecSetter instances where it's not at all obvious why they 
are needed. For example, why was it added here?:
OopStorageParIterPerf::~OopStorageParIterPerf() {
  // missing transition to vm state
  Thread::WXWriteFromExecSetter wx_write;
  _storage.release(_entries, ARRAY_SIZE(_entries));
}
You need to dig down in the OopStorage implementation to find that it's because 
it uses SafeFetch, which has the opposite transition:
inline int SafeFetch32(int* adr, int errValue) {
  assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");
  Thread::WXExecFromWriteSetter wx_exec;
  return StubRoutines::SafeFetch32_stub()(adr, errValue);
}
I think this adds complexity to code that shouldn't have to deal with this. I'm 
fine with having to force devs / code that writes to exec memory to have to 
deal with a bit more complexity, but it seems wrong to let this leak out to 
code that is staying far away from that.

For my understanding, was this choice made because the nmethods instances (and 
maybe other types as well) are placed in the code cache memory segment, which 
is executable? If the code cache only contained the JIT:ed code, and not object 
instances, I think this could more easily have been contained a bit more.

If the proposed solution is going to stay, maybe this could be made a little 
bit nicer by changing the SafeFetch implementation to use a new scoped object 
that doesn't enforce the "from" state to be "write"? Instead it could check if 
the state is "write" and only then flip the state back and forth. I think, this 
would remove the change needed OopStorageParIterPerf, and probably other places 
as well.

src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp line 38:

> 36: #include "runtime/os.hpp"
> 37: #include "runtime/stubRoutines.hpp"
> 38: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

src/hotspot/share/runtime/stubRoutines.inline.hpp line 29:

> 27: 
> 28: #include 
> 29: #include 

Replace < > with " "

src/hotspot/os/aix/os_aix.cpp line 70:

> 68: #include "runtime/statSampler.hpp"
> 69: #include "runtime/stubRoutines.hpp"
> 70: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

-

Changes requested by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On Mon, 25 Jan 2021 19:38:16 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 two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

Changes requested by ihse (Reviewer).

make/autoconf/build-aux/autoconf-config.guess line 1275:

> 1273:   UNAME_PROCESSOR="aarch64"
> 1274:   fi
> 1275: fi ;;

Almost, but not quite, correct. We cannot change the autoconf-config.guess file 
due to license restrictions (the license allows redistribution, not 
modifications). Instead we have the config.guess file which "wraps" 
autoconf-config.guess and makes pre-/post-call modifications to work around 
limitations in the autoconf original file. So you need to check there if you 
are getting incorrect results back and adjust it in that case. See the already 
existing clauses in that file.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Alan Hayward]

On Tue, 26 Jan 2021 09:23:23 GMT, Magnus Ihse Bursie  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Refactor CDS disabling
>>  - Redo builsys support for aarch64-darwin
>
> Changes requested by ihse (Reviewer).

AIUI, the configure line needs passing a prebuilt JavaNativeFoundation framework
ie:
`
--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'
`

Otherwise there will be missing _JNFNative* functions.

Is this the long term plan? Or will eventually the required code be moved into 
JDK and/or the xcode one automatically get picked up by the configure scripts?

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Tue, 26 Jan 2021 12:02:02 GMT, Alan Hayward 
 wrote:

> AIUI, the configure line needs passing a prebuilt JavaNativeFoundation 
> framework
> ie:
> `--with-extra-ldflags='-F 
> /Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`
> 
> Otherwise there will be missing _JNFNative* functions.
> 
> Is this the long term plan? Or will eventually the required code be moved 
> into JDK and/or the xcode one automatically get picked up by the configure 
> scripts?

There is ongoing work by P. Race to eliminate dependence on JNF at all

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Alan Hayward]

On Tue, 26 Jan 2021 12:06:28 GMT, Vladimir Kempik  wrote:

> > AIUI, the configure line needs passing a prebuilt JavaNativeFoundation 
> > framework
> > ie:
> > `--with-extra-ldflags='-F 
> > /Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`
> > Otherwise there will be missing _JNFNative* functions.
> > Is this the long term plan? Or will eventually the required code be moved 
> > into JDK and/or the xcode one automatically get picked up by the configure 
> > scripts?
> 
> There is ongoing work by P. Race to eliminate dependence on JNF at all

Ok, that's great.
In the meantime is it worth adding something to the MacOS section of 
doc/building.* ?
(I pieced together the required line from multiple posts in a mailing list)

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On 2021-01-26 13:09, Vladimir Kempik wrote:

On Tue, 26 Jan 2021 12:02:02 GMT, Alan Hayward 
 wrote:


AIUI, the configure line needs passing a prebuilt JavaNativeFoundation framework
ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`

Otherwise there will be missing _JNFNative* functions.

Is this the long term plan? Or will eventually the required code be moved into 
JDK and/or the xcode one automatically get picked up by the configure scripts?

There is ongoing work by P. Race to eliminate dependence on JNF at all
How far has that work come? Otherwise the logic should be added to 
configure to look for this framework automatically, and provide a way to 
override it/set it if not found.


I don't think it's OK to publish a new port that cannot build 
out-of-the-box without hacks like this.


/Magnus


-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[erik . joelsson]

On 2021-01-26 04:44, Magnus Ihse Bursie wrote:

On 2021-01-26 13:09, Vladimir Kempik wrote:
On Tue, 26 Jan 2021 12:02:02 GMT, Alan Hayward 
 wrote:


AIUI, the configure line needs passing a prebuilt 
JavaNativeFoundation framework

ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`


Otherwise there will be missing _JNFNative* functions.

Is this the long term plan? Or will eventually the required code be 
moved into JDK and/or the xcode one automatically get picked up by 
the configure scripts?

There is ongoing work by P. Race to eliminate dependence on JNF at all
How far has that work come? Otherwise the logic should be added to 
configure to look for this framework automatically, and provide a way 
to override it/set it if not found.


I don't think it's OK to publish a new port that cannot build 
out-of-the-box without hacks like this.


My understanding is that Apple chose to not provide JNF for aarch64, so 
if you want to build OpenJDK, you first need to build JNF yourself (it's 
available in github). Phil is working on removing this dependency 
completely, which will solve this issue [1].


In the meantime, I don't think we should rely on finding JNF in 
unsupported locations inside Xcode. Could we wait with integrating this 
port until it can be built without such hacks? If not, then adding 
something in the documentation on how to get a working JNF would at 
least be needed.


/Erik

[1] https://bugs.openjdk.java.net/browse/JDK-8257852

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Vladimir Kempik]

On Tue, 26 Jan 2021 09:23:18 GMT, Magnus Ihse Bursie  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Refactor CDS disabling
>>  - Redo builsys support for aarch64-darwin
>
> make/autoconf/build-aux/autoconf-config.guess line 1275:
> 
>> 1273:  UNAME_PROCESSOR="aarch64"
>> 1274:  fi
>> 1275:fi ;;
> 
> Almost, but not quite, correct. We cannot change the autoconf-config.guess 
> file due to license restrictions (the license allows redistribution, not 
> modifications). Instead we have the config.guess file which "wraps" 
> autoconf-config.guess and makes pre-/post-call modifications to work around 
> limitations in the autoconf original file. So you need to check there if you 
> are getting incorrect results back and adjust it in that case. See the 
> already existing clauses in that file.

Hello
I have updated PR and moved this logic to make/autoconf/build-aux/config.guess
It's pretty similar to i386 -> x86_64 fix-up on macos_intel

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[David Holmes]

On Tue, 26 Jan 2021 12:34:11 GMT, Alan Hayward 
 wrote:

>>> AIUI, the configure line needs passing a prebuilt JavaNativeFoundation 
>>> framework
>>> ie:
>>> `--with-extra-ldflags='-F 
>>> /Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`
>>> 
>>> Otherwise there will be missing _JNFNative* functions.
>>> 
>>> Is this the long term plan? Or will eventually the required code be moved 
>>> into JDK and/or the xcode one automatically get picked up by the configure 
>>> scripts?
>> 
>> There is ongoing work by P. Race to eliminate dependence on JNF at all
>
>> > AIUI, the configure line needs passing a prebuilt JavaNativeFoundation 
>> > framework
>> > ie:
>> > `--with-extra-ldflags='-F 
>> > /Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`
>> > Otherwise there will be missing _JNFNative* functions.
>> > Is this the long term plan? Or will eventually the required code be moved 
>> > into JDK and/or the xcode one automatically get picked up by the configure 
>> > scripts?
>> 
>> There is ongoing work by P. Race to eliminate dependence on JNF at all
> 
> Ok, that's great.
> In the meantime is it worth adding something to the MacOS section of 
> doc/building.* ?
> (I pieced together the required line from multiple posts in a mailing list)

Hi Anton,

Just to add weight to comments already made by @coleenp and @stefank , I also 
find the W^X coding support to be too intrusive and polluting of the shared 
code. I would much rather see this support pushed down as far as possible, to 
minimise the impact and to use ifdef'd code for macos/Aarch64 (via 
MACOS_AARCH64_ONLY macro) rather than providing empty methods.

Thanks,
David

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On Tue, 26 Jan 2021 22:04:48 GMT, Vladimir Kempik  wrote:

>> make/autoconf/build-aux/autoconf-config.guess line 1275:
>> 
>>> 1273: UNAME_PROCESSOR="aarch64"
>>> 1274: fi
>>> 1275:   fi ;;
>> 
>> Almost, but not quite, correct. We cannot change the autoconf-config.guess 
>> file due to license restrictions (the license allows redistribution, not 
>> modifications). Instead we have the config.guess file which "wraps" 
>> autoconf-config.guess and makes pre-/post-call modifications to work around 
>> limitations in the autoconf original file. So you need to check there if you 
>> are getting incorrect results back and adjust it in that case. See the 
>> already existing clauses in that file.
>
> Hello
> I have updated PR and moved this logic to make/autoconf/build-aux/config.guess
> It's pretty similar to i386 -> x86_64 fix-up on macos_intel

Thanks. That looks better.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[David Holmes]

I don't know why the Skara tools decided to associate my comment with 
Alan Hayward's comment as they are not at all related. :(


David

On 27/01/2021 4:50 pm, David Holmes wrote:

On Tue, 26 Jan 2021 12:34:11 GMT, Alan Hayward 
 wrote:


AIUI, the configure line needs passing a prebuilt JavaNativeFoundation framework
ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`

Otherwise there will be missing _JNFNative* functions.

Is this the long term plan? Or will eventually the required code be moved into 
JDK and/or the xcode one automatically get picked up by the configure scripts?


There is ongoing work by P. Race to eliminate dependence on JNF at all



AIUI, the configure line needs passing a prebuilt JavaNativeFoundation framework
ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`
Otherwise there will be missing _JNFNative* functions.
Is this the long term plan? Or will eventually the required code be moved into 
JDK and/or the xcode one automatically get picked up by the configure scripts?


There is ongoing work by P. Race to eliminate dependence on JNF at all


Ok, that's great.
In the meantime is it worth adding something to the MacOS section of 
doc/building.* ?
(I pieced together the required line from multiple posts in a mailing list)


Hi Anton,

Just to add weight to comments already made by @coleenp and @stefank , I also 
find the W^X coding support to be too intrusive and polluting of the shared 
code. I would much rather see this support pushed down as far as possible, to 
minimise the impact and to use ifdef'd code for macos/Aarch64 (via 
MACOS_AARCH64_ONLY macro) rather than providing empty methods.

Thanks,
David

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Andrew Haley]

On 1/26/21 6:42 PM, erik.joels...@oracle.com wrote:
> My understanding is that Apple chose to not provide JNF for aarch64, so
> if you want to build OpenJDK, you first need to build JNF yourself (it's
> available in github). Phil is working on removing this dependency
> completely, which will solve this issue [1].
>
> In the meantime, I don't think we should rely on finding JNF in
> unsupported locations inside Xcode. Could we wait with integrating this
> port until it can be built without such hacks?

That sounds right to me.

In the meantime, there is some cleanup work to be done in mainline
on slow_signature_handler, which will potentially make the AArch64
back end merge much simpler.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Anton Kozlov]

On Mon, 25 Jan 2021 22:48:50 GMT, Chris Plummer  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Refactor CDS disabling
>>  - Redo builsys support for aarch64-darwin
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 702:
> 
>> 700:   primitiveArray = (*env)->GetLongArrayElements(env, registerArray, 
>> NULL);
>> 701: 
>> 702: #undef REG_INDEX
> 
> I'm not so sure why the #undef and subsequent #define of REG_INDEX is needed 
> since it seems to just get #define'd back to the same value.

We've merged two implementations of SA, this change slipped in. I've cleaned 
this up. Thanks for noticing!

-

PR: https://git.openjdk.java.net/jdk/pull/2200