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

[Anton Kozlov]

On Tue, 26 Jan 2021 12:50:22 GMT, Anton Kozlov  wrote:

>> Yes, that's why I thought it should be added to the classes 
>> ThreadInVMfromNative, etc like:
>> class ThreadInVMfromNative : public ThreadStateTransition {
>>   ResetNoHandleMark __rnhm;
>> 
>> We can look at it with cleaning up the thread transitions RFE or as a 
>> follow-on.  If every line of ThreadInVMfromNative has to have one of these   
>> Thread::WXWriteVerifier __wx_write; people are going to miss them when 
>> adding the former.
>
> Not every ThreadInVMfromNative needs this, for example JIT goes to Native 
> state without changing of W^X state. But from some experience of maintaining 
> this patch, I actually had a duty to add missing W^X transitions after assert 
> failures. A possible solution is actually to make W^X transition a part of 
> ThreadInVMfromNative (and similar), but controlled by an optional constructor 
> parameter with possible values "do exec->write", "verify write"...;. So in a 
> common case ThreadInVMfromNative would implicitly do the transition and still 
> would allow something uncommon like verification of the state for the JIT. I 
> have to think about this.

I've dropped this transition here and in similar places after state tracking 
always available. As a benefit, there are few places really using the setter 
and all of them are tied to VM_ENTRY macro or similar one. I expect we don't 
need to do W^X management near every java thread state change.

-

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

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

[Anton Kozlov]

On Tue, 26 Jan 2021 12:01:30 GMT, Coleen Phillimore  wrote:

>> I assume a WXVerifier class that tracks W^X mode in debug mode and does 
>> nothing in release mode. I've considered to do this, it's relates to small 
>> inefficiencies, while this patch brings zero overhead (in release) for a 
>> platform that does not need W^X. 
>> * We don't need thread instance in release to call 
>> `os::current_thread_enable_wx`. Having WXVerifier a part of the Thread will 
>> require calling `Thread::current()` first and we could only hope for 
>> compiler to optimize this out, not sure if it will happen at all. In some 
>> contexts the Thread instance is available, in some it's not. 
>> * An instance of the empty class (as WXVerifier will be in the release) will 
>> occupy non-zero space in the Thread instance.
>> 
>> If such costs are negligible, I can do as suggested.
>
> I really just want the minimal number of lines of code and hooks in 
> thread.hpp.  You can still access it through the thread, just like these 
> lines currently.  Look at HandshakeState as an example.

Please take a look at the recent changes.

Changes in thread.hpp were reduced:
https://github.com/openjdk/jdk/pull/2200/files#diff-abdc409967d04172ecc20e3747aa55a79e755584d570b57c4d58902a9813d188

thread.inline.hpp provides definitions of accessors (non-trivial):
https://github.com/openjdk/jdk/pull/2200/files#diff-3a29f7f952bf2bd936f49e97cb3b86a7324851133e879c142dec724455310b50

And new threadWXSetters.hpp defines RAII context setter:
https://github.com/openjdk/jdk/pull/2200/files#diff-6424782ec43941031282f079e89adaa76d341ce340a3b78b0e9657358ec16278

-

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

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

[Bernhard Urban-Forster]

On Mon, 25 Jan 2021 17:43:35 GMT, Phil Race  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193:
> 
>> 191:* crbug.com/549610 */
>> 192:   // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h
>> 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10
> 
> I need a robust explanation of these changes.
> They are not mentioned, nor are they in upstream harfbuzz.
> Likely these should be pushed to upstream first if there is a reason for them.

I'm afraid it's one of those dirty changes that we did to make progress, but 
forgot to revisit later. Without the guard you would get:
* For target support_native_java.desktop_libharfbuzz_hb-coretext.o:

  if ( != nullptr && CTGetCoreTextVersion() < 0x0007) {
   ^

uint32_t CTGetCoreTextVersion( void ) CT_DEPRECATED("Use -[NSProcessInfo 
operatingSystemVersion]", macos(10.5, 11.0), ios(3.2, 14.0), watchos(2.0, 7.0), 
tvos(9.0, 14.0));
 ^

  if ( != nullptr && CTGetCoreTextVersion() < 0x0007) {
  ^

uint32_t CTGetCoreTextVersion( void ) CT_DEPRECATED("Use -[NSProcessInfo 
operatingSystemVersion]", macos(10.5, 11.0), ios(3.2, 14.0), watchos(2.0, 7.0), 
tvos(9.0, 14.0));
 ^
2 errors generated.
For now, it's probably better to go with what Anton did in the latest commit 
https://github.com/openjdk/jdk/pull/2200/commits/b2b396fca679fbc7ee78fb5bc80191bc79e1c490
Eventually upstream will do something about it I assume? How often does it get 
synced with upstream? Maybe worth to file an issue.

-

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

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

[Anton Kozlov]

On Tue, 26 Jan 2021 16:35:54 GMT, Bernhard Urban-Forster  
wrote:

>> @lewurm This and other harfbuzz changes came from MS, could you please 
>> comment here ?
>
> This looks like a merge fix mistake: 
> https://github.com/openjdk/jdk/commit/051357ef977ecab77fa9b2b1e61f94f288e716f9#diff-e3815f37244d076e27feef0c778fb78a4e5691b47db9c92abcb28bb2a9c2865e

I've reverted this change and disabled some warnings for libharfbuzz instead. I 
should be blamed, yes. Now this is consistent with other changes covered by 
JDK-8260402

-

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

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

[Bernhard Urban-Forster]

On Tue, 26 Jan 2021 16:07:19 GMT, Vladimir Kempik  wrote:

>> src/java.desktop/share/native/libharfbuzz/hb-common.h line 113:
>> 
>>> 111: 
>>> 112: #define HB_TAG(c1,c2,c3,c4) 
>>> ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF)))
>>> 113: #define HB_UNTAG(tag)   (char)(((tag)>>24)&0xFF), 
>>> (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF)
>> 
>> I need a robust explanation of these changes.
>> They are not mentioned, nor are they in upstream harfbuzz.
>> Likely these should be pushed to upstream first if there is a reason for 
>> them.
>
> @lewurm This and other harfbuzz changes came from MS, could you please 
> comment here ?

This looks like a merge fix mistake: 
https://github.com/openjdk/jdk/commit/051357ef977ecab77fa9b2b1e61f94f288e716f9#diff-e3815f37244d076e27feef0c778fb78a4e5691b47db9c92abcb28bb2a9c2865e

-

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

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

[Vladimir Kempik]

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

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/java.desktop/share/native/libharfbuzz/hb-common.h line 113:
> 
>> 111: 
>> 112: #define HB_TAG(c1,c2,c3,c4) 
>> ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF)))
>> 113: #define HB_UNTAG(tag)   (char)(((tag)>>24)&0xFF), 
>> (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF)
> 
> I need a robust explanation of these changes.
> They are not mentioned, nor are they in upstream harfbuzz.
> Likely these should be pushed to upstream first if there is a reason for them.

@lewurm This and other harfbuzz changes came from MS, could you please comment 
here ?

-

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

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

[Anton Kozlov]

On Mon, 25 Jan 2021 19:12:17 GMT, Coleen Phillimore  wrote:

>> I've tried to do something like this initially. The idea was to use Write in 
>> VM state and Exec in Java and Native states. However, for example, JIT runs 
>> in the Native state and needs Write access. So instead, W^X is managed on 
>> entry and exit from VM code, in macros like JRT_ENTRY. Unfortunately, not 
>> every JVM entry function is defined with an appropriate macro (at least for 
>> now), so I had to add explicit W^X state management along with the explicit 
>> java thread state, like here.
>
> Yes, that's why I thought it should be added to the classes 
> ThreadInVMfromNative, etc like:
> class ThreadInVMfromNative : public ThreadStateTransition {
>   ResetNoHandleMark __rnhm;
> 
> We can look at it with cleaning up the thread transitions RFE or as a 
> follow-on.  If every line of ThreadInVMfromNative has to have one of these   
> Thread::WXWriteVerifier __wx_write; people are going to miss them when 
> adding the former.

Not every ThreadInVMfromNative needs this, for example JIT goes to Native state 
without changing of W^X state. But from some experience of maintaining this 
patch, I actually had a duty to add missing W^X transitions after assert 
failures. A possible solution is actually to make W^X transition a part of 
ThreadInVMfromNative (and similar), but controlled by an optional constructor 
parameter with possible values "do exec->write", "verify write"...;. So in a 
common case ThreadInVMfromNative would implicitly do the transition and still 
would allow something uncommon like verification of the state for the JIT. I 
have to think about this.

-

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

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

[Coleen Phillimore]

On Tue, 26 Jan 2021 11:31:18 GMT, Anton Kozlov  wrote:

>> This could be a follow-up RFE.
>
> I assume a WXVerifier class that tracks W^X mode in debug mode and does 
> nothing in release mode. I've considered to do this, it's relates to small 
> inefficiencies, while this patch brings zero overhead (in release) for a 
> platform that does not need W^X. 
> * We don't need thread instance in release to call 
> `os::current_thread_enable_wx`. Having WXVerifier a part of the Thread will 
> require calling `Thread::current()` first and we could only hope for compiler 
> to optimize this out, not sure if it will happen at all. In some contexts the 
> Thread instance is available, in some it's not. 
> * An instance of the empty class (as WXVerifier will be in the release) will 
> occupy non-zero space in the Thread instance.
> 
> If such costs are negligible, I can do as suggested.

I really just want the minimal number of lines of code and hooks in thread.hpp. 
 You can still access it through the thread, just like these lines currently.  
Look at HandshakeState as an example.

-

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

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

[Anton Kozlov]

On Mon, 25 Jan 2021 14:40:42 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/runtime/thread.hpp line 915:
>> 
>>> 913:   verify_wx_state(WXExec);
>>> 914: }
>>> 915:   };
>> 
>> Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and 
>> just add the class instance as a field in thread.hpp?
>
> This could be a follow-up RFE.

I assume a WXVerifier class that tracks W^X mode in debug mode and does nothing 
in release mode. I've considered to do this, it's relates to small 
inefficiencies, while this patch brings zero overhead (in release) for a 
platform that does not need W^X. 
* We don't need thread instance in release to call 
`os::current_thread_enable_wx`. Having WXVerifier a part of the Thread will 
require calling `Thread::current()` first and we could only hope for compiler 
to optimize this out, not sure if it will happen at all. In some contexts the 
Thread instance is available, in some it's not. 
* An instance of the empty class (as WXVerifier will be in the release) will 
occupy non-zero space in the Thread instance.

If such costs are negligible, I can do as suggested.

-

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

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

[Andrew Haley]

On 1/25/21 5:45 PM, Anton Kozlov wrote:
> I see how this can be done, from looking at C example with `__thread`, which 
> involves 
> poorly documented relocations and private libc interface invocation.
> 
> But now I also wonder how much benefit would we have from this optimization.

OK, so perhaps it's not worth doing. Withdrawn.

-- 
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 [v2]

[Coleen Phillimore]

On Mon, 25 Jan 2021 15:01:25 GMT, Anton Kozlov  wrote:

>> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:
>> 
>>> 85:   JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
>>> 86:   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
>>> 87:   Thread::WXWriteFromExecSetter wx_write;
>> 
>> Is this on every transition to VM from Native?  Would it be better to add to 
>> ThreadInVMfromNative like the ResetNoHandleMark is?
>
> I've tried to do something like this initially. The idea was to use Write in 
> VM state and Exec in Java and Native states. However, for example, JIT runs 
> in the Native state and needs Write access. So instead, W^X is managed on 
> entry and exit from VM code, in macros like JRT_ENTRY. Unfortunately, not 
> every JVM entry function is defined with an appropriate macro (at least for 
> now), so I had to add explicit W^X state management along with the explicit 
> java thread state, like here.

Yes, that's why I thought it should be added to the classes 
ThreadInVMfromNative, etc like:
class ThreadInVMfromNative : public ThreadStateTransition {
  ResetNoHandleMark __rnhm;

We can look at it with cleaning up the thread transitions RFE or as a 
follow-on.  If every line of ThreadInVMfromNative has to have one of these   
Thread::WXWriteVerifier __wx_write; people are going to miss them when 
adding the former.

-

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

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

[Anton Kozlov]

On Mon, 25 Jan 2021 09:52:00 GMT, Andrew Haley  wrote:

>> Hello
>> Why is it not nice ?
>> linux_aarch64 uses some linux specific tls function 
>> _ZN10JavaThread25aarch64_get_thread_helperEv from 
>> hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.s
>> which clobbers only r0 and r1
>> macos_aarch64 has no such tls code and uses generic C-call to 
>> Thread::current();
>> Hence we  are saving possibly clobbered regs here.
>
> Surely if you did as Linux does you wouldn't need to clobber all those 
> registers.

I see how this can be done, from looking at C example with `__thread`, which 
involves 
poorly documented relocations and private libc interface invocation.

But now I also wonder how much benefit would we have from this optimization.
`get_thread` is called from few places and all of them are guarded by `#ifdef 
ASSERT`. The release build is still fine after I removed 
MacroAssembler::get_thread definition (as a verification).

Callers of get_thread:
* StubAssembler::call_RT
* Runtime1::generate_patching
* StubGenerator::generate_call_stub
* StubGenerator::generate_catch_exception

All of them are heavy-weight functions, nonoptimal get_thread is unlikely to 
slow them down even in fastdebug.

-

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

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

[Phil Race]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

Changes requested by prr (Reviewer).

src/java.desktop/share/native/libharfbuzz/hb-common.h line 113:

> 111: 
> 112: #define HB_TAG(c1,c2,c3,c4) 
> ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF)))
> 113: #define HB_UNTAG(tag)   (char)(((tag)>>24)&0xFF), 
> (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF)

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193:

> 191:* crbug.com/549610 */
> 192:   // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h
> 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

-

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

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

[Andrew Haley]

On Sun, 24 Jan 2021 15:50:01 GMT, Anton Kozlov  wrote:

>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 86:
>> 
>>> 84: 
>>> 85:   switch (_num_int_args) {
>>> 86:   case 0:
>> 
>> I don't think you need such a large switch statement. I think this can be 
>> expressed as
>> if (num_int_args <= 6) {
>> ldr(as_Register(num_int_args + r1.encoding()), src);
>> ... etc.
>
> I like the suggestion. For the defense, new functions were made this way 
> intentionally, to match existing `pass_int`, `pass_long`,.. I take your 
> comment as a blessing to fix all of them. But I feel that refactoring of 
> existing code should go in a separate commit. Especially after `pass_int` 
> used `ldr/str` instead of `ldrw/strw`, which looks wrong. 
> https://github.com/openjdk/jdk/pull/2200/files#diff-1ff58ce70aeea7e9842d34e8d8fd9c94dd91182999d455618b2a171efd8f742cL87-R122

This is new code, and it should not repeat the mistakes of the past. There's no 
need to refactor the similar existing code as part of this patch.

-

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

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

[Bernhard Urban-Forster]

On Mon, 25 Jan 2021 13:30:55 GMT, Vladimir Kempik  wrote:

>> make/modules/jdk.hotspot.agent/Lib.gmk line 34:
>> 
>>> 32: 
>>> 33: else ifeq ($(call isTargetOs, macosx), true)
>>> 34:   SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \
>> 
>> Is this really proper for macos-x64? I thought we used -Damd64 as a marker 
>> for all macos-x64 C/C++ files. (Most files get their flags from the common 
>> setup in configure, but SA is a different beast due to historical reasons).
>
> @luhenry , could you please check this comment, I think SA-agent was MS's job 
> here.

The target is identified by the header file now:
https://github.com/openjdk/jdk/pull/2200/files#diff-51442e74eeef2163f0f0643df0ae995083f666367e25fba2b527a9a5bc8743a6R35-R41

Do you think there is any problem with this approach?

-

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

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

[Anton Kozlov]

On Mon, 25 Jan 2021 14:36:35 GMT, Coleen Phillimore  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:
> 
>> 85:   JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
>> 86:   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
>> 87:   Thread::WXWriteFromExecSetter wx_write;
> 
> Is this on every transition to VM from Native?  Would it be better to add to 
> ThreadInVMfromNative like the ResetNoHandleMark is?

I've tried to do something like this initially. The idea was to use Write in VM 
state and Exec in Java and Native states. However, for example, JIT runs in the 
Native state and needs Write access. So instead, W^X is managed on entry and 
exit from VM code, in macros like JRT_ENTRY. Unfortunately, not every JVM entry 
function is defined with an appropriate macro (at least for now), so I had to 
add explicit W^X state management along with the explicit java thread state, 
like here.

-

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

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

[Coleen Phillimore]

On Mon, 25 Jan 2021 14:40:09 GMT, Coleen Phillimore  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/share/runtime/thread.hpp line 915:
> 
>> 913:   verify_wx_state(WXExec);
>> 914: }
>> 915:   };
> 
> Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and 
> just add the class instance as a field in thread.hpp?

This could be a follow-up RFE.

-

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

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

[Coleen Phillimore]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

src/hotspot/share/runtime/thread.hpp line 915:

> 913:   verify_wx_state(WXExec);
> 914: }
> 915:   };

Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and just 
add the class instance as a field in thread.hpp?

-

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

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

[Coleen Phillimore]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:

> 85:   JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
> 86:   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
> 87:   Thread::WXWriteFromExecSetter wx_write;

Is this on every transition to VM from Native?  Would it be better to add to 
ThreadInVMfromNative like the ResetNoHandleMark is?

-

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

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

[Vladimir Kempik]

On Mon, 25 Jan 2021 14:03:40 GMT, Per Liden  wrote:

> In `make/autoconf/jvm-features.m4` I notice that you haven't enabled ZGC for 
> macos/aarch64. Is that just an oversight, or is there a reason for that?

because it does not work
processor_id has no "official docs"-friendly implementation, only a hacky one 
from ios world.

Also ZGC needs additional W^X work.
I believe zgc supports needs to be done in a separate commit.

-

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

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

[Per Liden]

On Mon, 25 Jan 2021 13:23:27 GMT, Magnus Ihse Bursie  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> Changes requested by ihse (Reviewer).

In `make/autoconf/jvm-features.m4` I notice that you haven't enabled ZGC for 
macos/aarch64. Is that just an oversight, or is there a reason for that?

-

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

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

[Vladimir Kempik]

On Mon, 25 Jan 2021 13:18:34 GMT, Magnus Ihse Bursie  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> make/common/NativeCompilation.gmk line 1180:
> 
>> 1178: # silently fail otherwise.
>> 1179: ifneq ($(CODESIGN), )
>> 1180:  $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" 
>> --timestamp --options runtime \
> 
> What does -f do, and is it needed for macos-x64 as well?

-f  - replace signature if it's present, if not  - just sign as usual
macos-aarch64 binaries always have adhoc signature, so need to replace it.

> make/modules/jdk.hotspot.agent/Lib.gmk line 34:
> 
>> 32: 
>> 33: else ifeq ($(call isTargetOs, macosx), true)
>> 34:   SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \
> 
> Is this really proper for macos-x64? I thought we used -Damd64 as a marker 
> for all macos-x64 C/C++ files. (Most files get their flags from the common 
> setup in configure, but SA is a different beast due to historical reasons).

@luhenry , could you please check this comment, I think SA-agent was MS's job 
here.

-

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

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

[Magnus Ihse Bursie]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

Changes requested by ihse (Reviewer).

make/autoconf/jvm-features.m4 line 269:

> 267: if test "x$OPENJDK_TARGET_OS" != xaix && \
> 268: !( test "x$OPENJDK_TARGET_OS" = "xmacosx" && \
> 269: test "x$OPENJDK_TARGET_CPU" = "xaarch64" ) ; then

This test is making my head spin. Can't you just invert it to this structure:

if OS=aix || OS+CPU = mac+aarch64; then
  no
else
 yes
fi

make/autoconf/platform.m4 line 75:

> 73: ;;
> 74:   esac
> 75:   ;;

This is solved in the wrong place. This code should just use the result from 
`config.guess/config.sub`. These files are imported from the autoconf project. 
Unfortunately, they have changed the license to one incompatible with OpenJDK, 
so we are stuck with an old version. Instead, we have created a "bugfix 
wrapper" that calls the original `autoconf-config.guess/sub` and fixes up the 
result, with stuff like this.

make/autoconf/platform.m4 line 273:

> 271:   # Convert the autoconf OS/CPU value to our own data, into the 
> VAR_OS/CPU/LIBC variables.
> 272:   PLATFORM_EXTRACT_VARS_FROM_OS($build_os)
> 273:   PLATFORM_EXTRACT_VARS_FROM_CPU($build_cpu, $build_os)

Fixing the comment above means this change, and the one below, can be reverted.

make/common/NativeCompilation.gmk line 1180:

> 1178: # silently fail otherwise.
> 1179: ifneq ($(CODESIGN), )
> 1180:   $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" 
> --timestamp --options runtime \

What does -f do, and is it needed for macos-x64 as well?

make/modules/jdk.hotspot.agent/Lib.gmk line 34:

> 32: 
> 33: else ifeq ($(call isTargetOs, macosx), true)
> 34:   SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \

Is this really proper for macos-x64? I thought we used -Damd64 as a marker for 
all macos-x64 C/C++ files. (Most files get their flags from the common setup in 
configure, but SA is a different beast due to historical reasons).

-

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

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

[Andrew Haley]

On Sun, 24 Jan 2021 16:29:31 GMT, Vladimir Kempik  wrote:

>> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5272:
>> 
>>> 5270: void MacroAssembler::get_thread(Register dst) {
>>> 5271:   RegSet saved_regs = RegSet::range(r0, r1) + 
>>> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
>>> 5272:   push(saved_regs, sp);
>> 
>> This isn't very nice.
>
> Hello
> Why is it not nice ?
> linux_aarch64 uses some linux specific tls function 
> _ZN10JavaThread25aarch64_get_thread_helperEv from 
> hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.s
> which clobbers only r0 and r1
> macos_aarch64 has no such tls code and uses generic C-call to 
> Thread::current();
> Hence we  are saving possibly clobbered regs here.

Surely if you did as Linux does you wouldn't need to clobber all those 
registers.

-

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

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

[Andrew Haley]

On Sun, 24 Jan 2021 16:10:44 GMT, Anton Kozlov  wrote:

>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 394:
>> 
>>> 392: 
>>> 393: class SlowSignatureHandler
>>> 394:   : public NativeSignatureIterator {
>> 
>> SlowSignatureHandler is turning into a maintenance nightmare. This isn't the 
>> fault of this commit so much as some nasty cut-and-paste programming in the 
>> code you're editing. It might well be better to rewrite this whole thing to 
>> use a table-driven approach, with a table that contains the increment and 
>> the size of each native type: then we'd have only a single routine which 
>> could pass data of any type, and each OS needs only supply a table of 
>> constants.
>
> Would you like me to do something about it now? The problem is that the 
> functions of SlowSignatureHandler are subtly different, so it will be 
> multiple tables, not sure how many. Such change is another candidate for a 
> separate code enhancement, which I would like not to mix into the JEP 
> implementation (it's already not a small change :)). But if you think it 
> would be a good thing, please let me know. In another case, I'll add this to 
> a queue of follow-up enhancements.

I'm not sure it can wait. This change turns already-messy code into something 
significantly messier, to the extent that it's not really good enough to go 
into mainline.

-

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

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

[Vladimir Kempik]

On Sat, 23 Jan 2021 11:43:31 GMT, Andrew Haley  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5272:
> 
>> 5270: void MacroAssembler::get_thread(Register dst) {
>> 5271:   RegSet saved_regs = RegSet::range(r0, r1) + 
>> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
>> 5272:   push(saved_regs, sp);
> 
> This isn't very nice.

Hello
Why is it not nice ?
linux_aarch64 uses some linux specific tls function 
_ZN10JavaThread25aarch64_get_thread_helperEv from 
hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.s
which clobbers only r0 and r1
macos_aarch64 has no such tls code and uses generic C-call to Thread::current();
Hence we  are saving possibly clobbered regs here.

-

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

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

[Anton Kozlov]

On Sat, 23 Jan 2021 11:42:48 GMT, Andrew Haley  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 394:
> 
>> 392: 
>> 393: class SlowSignatureHandler
>> 394:   : public NativeSignatureIterator {
> 
> SlowSignatureHandler is turning into a maintenance nightmare. This isn't the 
> fault of this commit so much as some nasty cut-and-paste programming in the 
> code you're editing. It might well be better to rewrite this whole thing to 
> use a table-driven approach, with a table that contains the increment and the 
> size of each native type: then we'd have only a single routine which could 
> pass data of any type, and each OS needs only supply a table of constants.

Would you like me to do something about it now? The problem is that the 
functions of SlowSignatureHandler are subtly different, so it will be multiple 
tables, not sure how many. Such change is another candidate for a separate code 
enhancement, which I would like not to mix into the JEP implementation (it's 
already not a small change :)). But if you think it would be a good thing, 
please let me know. In another case, I'll add this to a queue of follow-up 
enhancements.

-

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

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

[Anton Kozlov]

On Sat, 23 Jan 2021 11:10:17 GMT, Andrew Haley  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 86:
> 
>> 84: 
>> 85:   switch (_num_int_args) {
>> 86:   case 0:
> 
> I don't think you need such a large switch statement. I think this can be 
> expressed as
> if (num_int_args <= 6) {
> ldr(as_Register(num_int_args + r1.encoding()), src);
> ... etc.

I like the suggestion. For the defense, new functions were made this way 
intentionally, to match existing `pass_int`, `pass_long`,.. I take your comment 
as a blessing to fix all of them. But I feel that refactoring of existing code 
should go in a separate commit. Especially after `pass_int` used `ldr/str` 
instead of `ldrw/strw`, which looks wrong. 
https://github.com/openjdk/jdk/pull/2200/files#diff-1ff58ce70aeea7e9842d34e8d8fd9c94dd91182999d455618b2a171efd8f742cL87-R122

-

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

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

[Anton Kozlov]

On Fri, 22 Jan 2021 20:18:51 GMT, Erik Joelsson  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> make/autoconf/flags-cflags.m4 line 169:
> 
>> 167:   WARNINGS_ENABLE_ALL="-Wall -Wextra -Wformat=2 
>> $WARNINGS_ENABLE_ADDITIONAL"
>> 168: 
>> 169:   DISABLED_WARNINGS="unknown-warning-option unused-parameter unused 
>> format-nonliteral"
> 
> Globally disabling a warning needs some motivation. Why is this needed? Does 
> it really need to be applied everywhere or is there a specific binary or 
> source file where this is triggered?

It seems to be a leftover from the past. I tried to revert the line and the 
build went fine. Thanks for noticing!

-

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

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

[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:

 - Address feedback for signature generators
 - Enable -Wformat-nonliteral back

-

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

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2200=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2200=00-01

  Stats: 204 lines in 2 files changed: 20 ins; 138 del; 46 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