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

2021-03-12 Thread Vladimir Kempik
On Tue, 2 Mar 2021 08:12:10 GMT, Anton Kozlov  wrote:

>> I wasn't able to replicate JDK-8020753 and JDK-8186286. So will remove these 
>> workaround
>> @gerard-ziemski, 8020753 was originally your fix, do you know if it still 
>> needed on intel-mac ?
>
> The x86_bsd still carries the workaround 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp#L745.
>  It's worth having macos ports to be aligned by features. I would propose to 
> have this workaround for now, and decide on it later for macos/x86 and 
> macos/aarch64 at once. Sorry for chiming in so late.

Hello Anton.
Please keep JDK-8020753 away from this port, as JDK-8020753 was very 
intel-specific workaround and macos11.0 on aarch64 doesn't show any signs of 
original bug.

-

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


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

2021-03-05 Thread Richard Reingruber
On Fri, 5 Mar 2021 11:11:44 GMT, Alan Hayward 
 wrote:

>>> I was building this PR on a new machine, and I now get the following error:
>>> 
>>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:258:31:
>>> >  error: cast to smaller integer type 'MIDIClientRef' (aka 'unsigned int') 
>>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
>>> > static MIDIClientRef client = (MIDIClientRef) NULL;
>>> > ^~~~
>>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:259:29:
>>> >  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') 
>>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
>>> > static MIDIPortRef inPort = (MIDIPortRef) NULL;
>>> > ^~
>>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:260:30:
>>> >  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') 
>>> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
>>> > static MIDIPortRef outPort = (MIDIPortRef) NULL;
>>> > ^~
>>> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:466:32:
>>> >  error: cast to smaller integer type 'MIDIEndpointRef' (aka 'unsigned 
>>> > int') from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
>>> > MIDIEndpointRef endpoint = (MIDIEndpointRef) NULL;
>>> > ^~
>>> > 4 errors generated.
>>> 
>>> As far as I can tell the only difference between the two systems is the 
>>> xcode version:
>>> 
>>> New system (failing)
>>> % xcodebuild -version
>>> Xcode 12.5
>>> Build version 12E5244e
>>> 
>>> Old system (working)
>>> % xcodebuild -version
>>> Xcode 12.4
>>> Build version 12D4e
>>> 
>>> Looks like the newer version of Xcode is being a little stricter with 
>>> casting?
>>> Replacing the NULL with 0 seems to fix the issue.
>> 
>> Hello
>> there is one issue with the info you provided, it's from Xcode12.5 beta.
>> And beta license agreement forbids sharing output of beta version of 
>> compiler&co
>> So we can't say we have issue with newer xcode beta until that beta went 
>> public & released.
>> Fixing issues you found now will mean someone have violated xcode beta 
>> license agreement and made these issues public.
>
>> Hello
>> there is one issue with the info you provided, it's from Xcode12.5 beta.
>> And beta license agreement forbids sharing output of beta version of 
>> compiler&co
>> So we can't say we have issue with newer xcode beta until that beta went 
>> public & released.
>> Fixing issues you found now will mean someone have violated xcode beta 
>> license agreement and made these issues public.
> 
> Ok, I wasn't aware of that. I'll downgrade back to the non-beta version.

Hi,

@VladimirKempik reported failure of 
CompressedClassPointers.largeHeapAbove32GTest() with 
[JDK-8262895](https://bugs.openjdk.java.net/browse/JDK-8262895) on 
macos_aarch64.

He also helped analyzing the issue (thanks!).

Apparently the vm has difficulties mapping the heap of 31GB at one of the 
preferred addresses given in
[`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477).
 This causes the test failure indirectly.

IMO it could be worth the effort to find out why the heap cannot be mapped at 
the preferred addresses. Reviewers should decide on it. I wouldn't be able to 
do it myself though as I don't have access to a macos_aarch64 system.

Alternatively I'd suggest to exlude macos_aarch64 from the subtest 
largeHeapAbove32GTest.

Best regards,
Richard.

--

 Details of the analysis

In the following trace we see the vm trying to allocate the heap at addresses 
given in 
[`get_attach_addresses_for_disjoint_mode()`](https://github.com/openjdk/jdk/blob/8d3de4b1bdb5dc13bb7724596dc2123ba05bbb81/src/hotspot/share/memory/virtualspace.cpp#L477)
 without success:

images/jdk/bin/java  -XX:+UnlockDiagnosticVMOptions 
-XX:+UnlockExperimentalVMOptions -Xmx31g -XX:-UseAOT 
-Xlog:gc+metaspace=trace,cds=trace,heap+gc+exit=info,gc+heap+coops=trace 
-Xshare:off -XX:+VerifyBeforeGC -XX:HeapSearchSteps=40 -version
OpenJDK 64-Bit Server VM warning: Shared spaces are not supported in this VM
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0010 
heap of size 0x7c100
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0018 
heap of size 0x7c100
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0020 
heap of size 0x7c100
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0040 
heap of size 0x7c100
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0050 
heap of size 0x7c100
[0.005s][trace][gc,heap,coops] Trying to allocate at address 0x0008 
heap of size 0x7c10

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

2021-03-05 Thread Alan Hayward
On Thu, 4 Mar 2021 18:19:33 GMT, Vladimir Kempik  wrote:

> Hello
> there is one issue with the info you provided, it's from Xcode12.5 beta.
> And beta license agreement forbids sharing output of beta version of 
> compiler&co
> So we can't say we have issue with newer xcode beta until that beta went 
> public & released.
> Fixing issues you found now will mean someone have violated xcode beta 
> license agreement and made these issues public.

Ok, I wasn't aware of that. I'll downgrade back to the non-beta version.

-

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


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

2021-03-04 Thread Vladimir Kempik
On Thu, 4 Mar 2021 17:36:22 GMT, Alan Hayward 
 wrote:

> I was building this PR on a new machine, and I now get the following error:
> 
> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:258:31:
> >  error: cast to smaller integer type 'MIDIClientRef' (aka 'unsigned int') 
> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> > static MIDIClientRef client = (MIDIClientRef) NULL;
> > ^~~~
> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:259:29:
> >  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') 
> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> > static MIDIPortRef inPort = (MIDIPortRef) NULL;
> > ^~
> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:260:30:
> >  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') 
> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> > static MIDIPortRef outPort = (MIDIPortRef) NULL;
> > ^~
> > /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:466:32:
> >  error: cast to smaller integer type 'MIDIEndpointRef' (aka 'unsigned int') 
> > from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> > MIDIEndpointRef endpoint = (MIDIEndpointRef) NULL;
> > ^~
> > 4 errors generated.
> 
> As far as I can tell the only difference between the two systems is the xcode 
> version:
> 
> New system (failing)
> % xcodebuild -version
> Xcode 12.5
> Build version 12E5244e
> 
> Old system (working)
> % xcodebuild -version
> Xcode 12.4
> Build version 12D4e
> 
> Looks like the newer version of Xcode is being a little stricter with casting?
> Replacing the NULL with 0 seems to fix the issue.

Hello
there is one issue with the info you provided, it's from Xcode12.5 beta.
And beta license agreement forbids sharing output of beta version of compiler&co
So we can't say we have issue with newer xcode beta until that beta went public 
& released.
Fixing issues you found now will mean someone have violated xcode beta license 
agreement and made these issues public.

-

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


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

2021-03-04 Thread Alan Hayward
On Thu, 4 Mar 2021 15:27:25 GMT, Gerard Ziemski  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.
>
>> > 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.

I was building this PR on a new machine, and I now get the following error:

> /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:258:31:
>  error: cast to smaller integer type 'MIDIClientRef' (aka 'unsigned int') 
> from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> static MIDIClientRef client = (MIDIClientRef) NULL;
>   ^~~~
> /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:259:29:
>  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') from 
> 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> static MIDIPortRef inPort = (MIDIPortRef) NULL;
> ^~
> /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:260:30:
>  error: cast to smaller integer type 'MIDIPortRef' (aka 'unsigned int') from 
> 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> static MIDIPortRef outPort = (MIDIPortRef) NULL;
>  ^~
> /Users/alahay01/java/gerrit_jdk/src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiUtils.c:466:32:
>  error: cast to smaller integer type 'MIDIEndpointRef' (aka 'unsigned int') 
> from 'void *' [-Werror,-Wvoid-pointer-to-int-cast]
> MIDIEndpointRef endpoint = (MIDIEndpointRef) NULL;
>^~
> 4 errors generated.

As far as I can tell the only difference between the two systems is the xcode 
version:

New system (failing)
% xcodebuild -version
Xcode 12.5
Build version 12E5244e

Old system (working)
% xcodebuild -version
Xcode 12.4
Build version 12D4e

Looks like the newer version of Xcode is being a little stricter with casting?
Replacing the NULL with 0 seems to fix the issue.

-

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


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

2021-03-04 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-03-03 Thread Anton Kozlov
On Wed, 3 Mar 2021 17:46:41 GMT, Andrew Haley  wrote:

> A list of the bugs that our internal testing revealed so far ...

Thank you! Some of them look like test issues, but a few need more serious 
consideration. I want to resolve 
https://bugs.openjdk.java.net/browse/JDK-8262903 at least, along with a few 
remaining comments.

-

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


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

2021-03-03 Thread Andrew Haley
On Wed, 3 Mar 2021 17:39:28 GMT, Gerard Ziemski  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.

-

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


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

2021-03-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-03-02 Thread Anton Kozlov
On Tue, 2 Feb 2021 23:10:17 GMT, Daniel D. Daugherty  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?

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.

-

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


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

2021-03-02 Thread Anton Kozlov
On Tue, 2 Feb 2021 22:47:04 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/share/prims/nativeEntryPoint.cpp line 45:
> 
>> 43:   guarantee(status == JNI_OK && !env->ExceptionOccurred(),
>> 44: "register jdk.internal.invoke.NativeEntryPoint natives");
>> 45: JNI_END
> 
> I thought that jcheck caught a missing new-line?

It seems it did not 
https://github.com/openjdk/jdk/commit/0fb31dbf3a2adc0f7fb2f9924083908724dcde5a#diff-f39cd3f794a337734adf30863f702725ee04182fee2345b2669e59ebed17a2ccR44.
 Anyway I reverted this change as it is the only change in the file, thanks.

-

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


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

2021-03-02 Thread Anton Kozlov
On Tue, 2 Feb 2021 22:18:43 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/thread_bsd_aarch64.cpp line 43:
> 
>> 41:   assert(Thread::current() == this, "caller must be current thread");
>> 42:   return pd_get_top_frame(fr_addr, ucontext, isInJava);
>> 43: }
> 
> Is AsyncGetCallTrace() being supported by this port?

I assume answer to be yes (I have a little experience with AsyncGetCallTrace). 
This code is identical to one in bsd/x86 and linux/aarch64. After few changes 
in the build system, I got jtreg/serviceability/AsyncGetCallTrace test compiled 
and passed. I've filed JDK-8262839 to enable the test.

-

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


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

2021-03-02 Thread Anton Kozlov
On Fri, 12 Feb 2021 13:32:52 GMT, Vladimir Kempik  wrote:

>> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 486:
>> 
>>> 484: }
>>> 485:   }
>>> 486: }
>> 
>> This appears to be a mix for Mavericks (10.9) and 10.12
>> work arounds. Is this code needed by this project?
>
> I wasn't able to replicate JDK-8020753 and JDK-8186286. So will remove these 
> workaround
> @gerard-ziemski, 8020753 was originally your fix, do you know if it still 
> needed on intel-mac ?

The x86_bsd still carries the workaround 
https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp#L745.
 It's worth having macos ports to be aligned by features. I would propose to 
have this workaround for now, and decide on it later for macos/x86 and 
macos/aarch64 at once. Sorry for chiming in so late.

-

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


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

2021-03-02 Thread Anton Kozlov
On Fri, 12 Feb 2021 12:40:09 GMT, Florian Weimer  wrote:

>> only macos comiplers
>
> The comment is also wrong for glibc: The AArch64 ABI requires a 64 KiB guard 
> region independently of page size, otherwise `-fstack-clash-protection` is 
> not reliable.

Thanks, I deleted the comment. It describes implementation distinguishing 
initial and regular thread 
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/548cb3b7b713#l12.7. I'm not 
sure why the comment was preserved for the bsd_x86, but I don't think it makes 
sense here.

-

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


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

2021-03-01 Thread Anton Kozlov
On Mon, 15 Feb 2021 17:59:54 GMT, Vladimir Kempik  wrote:

>> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 810:
>> 
>>> 808: #ifdef __APPLE__
>>> 809:   // Less-than word types are stored one after another.
>>> 810:   // The code unable to handle this, bailout.
>> 
>> Perhaps:  // The code is unable to handle this so bailout.
>
> Hello, we have updated PR, now this bailout is used only by the code which 
> can handle it (native wrapper generator), for the rest it will cause 
> guarantee failed if this bailout is triggered

I've fixed the spelling. Sorry for it to take so long :(

>> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 195:
>> 
>>> 193: frame os::get_sender_for_C_frame(frame* fr) {
>>> 194:   return frame(fr->link(), fr->link(), fr->sender_pc());
>>> 195: }
>> 
>> Is this file going to be built by GCC or just macOS compilers?
>
> there is no support for compiling java with gcc on macos since about jdk11, 
> only clang.
> considering this and the absence of gcc for macos_m1, the answer is - just 
> macOS compilers.

I've fixed the comment. Now it states `JVM compiled with 
-fno-omit-frame-pointer, so RFP is saved on the stack.`

-

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


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

2021-03-01 Thread Anton Kozlov
On Mon, 1 Mar 2021 10:46:34 GMT, Andrew Haley  wrote:

>> They are defined in 13.2.95. MIDR_EL1, Main ID Register. Apple's code is not 
>> there, but "Arm can assign codes that are not published in this manual. All 
>> values not assigned by Arm are reserved and must not be used.". I assume the 
>> value was obtained by digging around 
>> https://github.com/apple/darwin-xnu/blob/main/osfmk/arm/cpuid.h#L62
>
> Anton, this paragraph looks like an excellent comment.

Thanks, I've added the comment.

-

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


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

2021-03-01 Thread Andrew Haley
On Fri, 5 Feb 2021 17:20:55 GMT, Anton Kozlov  wrote:

>> 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?
>
> They are defined in 13.2.95. MIDR_EL1, Main ID Register. Apple's code is not 
> there, but "Arm can assign codes that are not published in this manual. All 
> values not assigned by Arm are reserved and must not be used.". I assume the 
> value was obtained by digging around 
> https://github.com/apple/darwin-xnu/blob/main/osfmk/arm/cpuid.h#L62

Anton, this paragraph looks like an excellent comment.

-

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


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

2021-02-26 Thread erik . joelsson



On 2021-02-26 06:37, daniel.daughe...@oracle.com wrote:

On 2/26/21 7:55 AM, Vladimir Kempik wrote:
On Tue, 2 Feb 2021 23:07:08 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/java.base/macosx/native/libjli/java_md_macosx.m line 210:


208: if (preferredJVM == NULL) {
209: #if defined(__i386__)
210: preferredJVM = "client";

#if defined(__i386__)
 preferredJVM = "client";
Not your bug, but Oracle/OpenJDK never supported 32-bit __i386__ on 
macOS.

Hello
I thought the openjdk7 supported 32-bit macos at some point in time


The macOS porting project supported 32-bit macOS, but when the code
was integrated into OpenJDK7 I don't believe that 32-bit macOS was
supported. I could be wrong... it was quite a while ago...

AFAIK, OpenJDK never supported 32bit macos, but there are certainly 
leftovers here and there indicating that the original source did at some 
point. In the build system we cleaned that up a long time ago.


/Erik



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

2021-02-26 Thread daniel . daugherty

On 2/26/21 7:55 AM, Vladimir Kempik wrote:

On Tue, 2 Feb 2021 23:07:08 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/java.base/macosx/native/libjli/java_md_macosx.m line 210:


208: if (preferredJVM == NULL) {
209: #if defined(__i386__)
210: preferredJVM = "client";

#if defined(__i386__)
 preferredJVM = "client";
Not your bug, but Oracle/OpenJDK never supported 32-bit __i386__ on macOS.

Hello
I thought the openjdk7 supported 32-bit macos at some point in time


The macOS porting project supported 32-bit macOS, but when the code
was integrated into OpenJDK7 I don't believe that 32-bit macOS was
supported. I could be wrong... it was quite a while ago...

Dan




-

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




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

2021-02-26 Thread Vladimir Kempik
On Tue, 2 Feb 2021 23:07:08 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/java.base/macosx/native/libjli/java_md_macosx.m line 210:
> 
>> 208: if (preferredJVM == NULL) {
>> 209: #if defined(__i386__)
>> 210: preferredJVM = "client";
> 
> #if defined(__i386__)
> preferredJVM = "client";
> Not your bug, but Oracle/OpenJDK never supported 32-bit __i386__ on macOS.

Hello
I thought the openjdk7 supported 32-bit macos at some point in time

-

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


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

2021-02-17 Thread Anton Kozlov
On Tue, 2 Feb 2021 22:42:22 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/share/logging/logStream.hpp line 35:
> 
>> 33: class LogStream : public outputStream {
>> 34:   // see test/hotspot/gtest/logging/test_logStream.cpp
>> 35:   friend class LogStreamTest;
> 
> It's not clear why this change is made for this port.

This was done for previous implementation of W^X, for gtests be able to access 
this test. This not required anymore, this hunk was reverted.

-

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


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

2021-02-17 Thread Anton Kozlov
On Mon, 15 Feb 2021 16:21:53 GMT, Bernhard Urban-Forster  
wrote:

>> This is the version of w^x on-demand switch implemented by microsoft guys. 
>> This is enabled only for debug builds.
>> @lewurm could you comment here please
>
> Those values can be observed in the debugger, but aren't documented or 
> defined in header files.
> 
> This mode was useful for the initial bootstrap of the platform (it helped 
> with missing W^X transitions), but shouldn't be required anymore today. I'm 
> fine with removing it altogether.

OK, I'm going to remove this block. So we'll be able to revert changes in 
globals.hpp https://github.com/openjdk/jdk/pull/2200/files#r568986339

-

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


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

2021-02-17 Thread Anton Kozlov
On Tue, 2 Feb 2021 21:51:56 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/cpu/aarch64/macroAssembler_aarch64.hpp line 31:
> 
>> 29: #include "asm/assembler.inline.hpp"
>> 30: #include "oops/compressedOops.hpp"
>> 31: #include "runtime/vm_version.hpp"
> 
> It's not clear why this include needed to be added.

Line 448 calls `VM_Version::features()`. It seems the declaration is included 
indirectly somehow on the rest of the platforms, through OS specific headers.

-

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


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

2021-02-15 Thread Anton Kozlov
On Wed, 3 Feb 2021 20:08:41 GMT, Anton Kozlov  wrote:

> I'm going to do as much refactoring as needed before this patch under 
> JDK-8261071

The recent merge resolves inconsitency between pass_byte/pass_short and other 
methods.

-

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


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

2021-02-15 Thread Vladimir Kempik
On Tue, 2 Feb 2021 21:52:47 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/cpu/aarch64/sharedRuntime_aarch64.cpp line 810:
> 
>> 808: #ifdef __APPLE__
>> 809:   // Less-than word types are stored one after another.
>> 810:   // The code unable to handle this, bailout.
> 
> Perhaps:  // The code is unable to handle this so bailout.

Hello, we have updated PR, now this bailout is used only by the code which can 
handle it (native wrapper generator), for the rest it will cause guarantee 
failed if this bailout is triggered

-

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


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

2021-02-15 Thread Bernhard Urban-Forster
On Fri, 12 Feb 2021 12:22:09 GMT, Vladimir Kempik  wrote:

>> Where did this come from - some snippet/example/tech note code? Maybe other 
>> people can help figure it out if we provide more info.
>
> This is the version of w^x on-demand switch implemented by microsoft guys. 
> This is enabled only for debug builds.
> @lewurm could you comment here please

Those values can be observed in the debugger, but aren't documented or defined 
in header files.

This mode was useful for the initial bootstrap of the platform (it helped with 
missing W^X transitions), but shouldn't be required anymore today. I'm fine 
with removing it altogether.

-

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


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

2021-02-12 Thread Vladimir Kempik
On Tue, 2 Feb 2021 22:08:14 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 221:
> 
>> 219: assert(sig == info->si_signo, "bad siginfo");
>> 220:   }
>> 221: */
> 
> Should this code be deleted? From here and from where it was copied
> from if it is also commented out there...

Thanks, will fix in bsd_aarch64 soon, as for bsd_x86 I've filled new bug and pr 
- https://github.com/openjdk/jdk/pull/2547

-

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


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

2021-02-12 Thread Anton Kozlov
On Wed, 3 Feb 2021 09:11:50 GMT, Andrew Haley  wrote:

>> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 323:
>> 
>>> 321:   str(zr, Address(rthread, JavaThread::last_Java_pc_offset()));
>>> 322: 
>>> 323:   str(zr, Address(rthread, 
>>> JavaFrameAnchor::saved_fp_address_offset()));
>> 
>> I don't think this switch from `JavaThread::saved_fp_address_offset()`
>> to `JavaFrameAnchor::saved_fp_address_offset()` is correct since
>> `rthread` is still used and is a JavaThread*. The new code will give you:
>> 
>> `rthread` + offset of the `saved_fp_address` field in a JavaFrameAnchor
>> 
>> The old code gave you:
>> 
>> `rthread` + offset of the `saved_fp_address` field in the 
>> JavaFrameAnchor field in the JavaThread
>> 
>> Those are not the same things.
>
> I agree, I don't understand why this change was made.

Wow, this is scary. I don't understand how I've merged JDK-8257882 like this. 
I've reviewed cpu/aarch64 changes again, there is nothing suspicious besides 
this. Thank you very much for catching, fixed.

-

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


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

2021-02-12 Thread Vladimir Kempik
On Tue, 2 Feb 2021 22:14:42 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 486:
> 
>> 484: }
>> 485:   }
>> 486: }
> 
> This appears to be a mix for Mavericks (10.9) and 10.12
> work arounds. Is this code needed by this project?

I wasn't able to replicate JDK-8020753 and JDK-8186286. So will remove these 
workaround
Gerard, 8020753 was originally your fix, do you know if it still needed on 
intel-mac ?

-

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


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

2021-02-12 Thread Florian Weimer
On Fri, 12 Feb 2021 12:22:44 GMT, Vladimir Kempik  wrote:

>> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 435:
>> 
>>> 433: //||\  Java thread created by VM does not 
>>> have glibc
>>> 434: //|glibc guard page| - guard, attached Java thread usually 
>>> has
>>> 435: //||/  1 glibc guard page.
>> 
>> Is this code going to be built by GCC (with glibc) or will only
>> macOS compilers and libraries be used?
>
> only macos comiplers

The comment is also wrong for glibc: The AArch64 ABI requires a 64 KiB guard 
region independently of page size, otherwise `-fstack-clash-protection` is not 
reliable.

-

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


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

2021-02-12 Thread Vladimir Kempik
On Tue, 2 Feb 2021 22:12:07 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 435:
> 
>> 433: //||\  Java thread created by VM does not 
>> have glibc
>> 434: //|glibc guard page| - guard, attached Java thread usually 
>> has
>> 435: //||/  1 glibc guard page.
> 
> Is this code going to be built by GCC (with glibc) or will only
> macOS compilers and libraries be used?

only macos comiplers

-

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


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

2021-02-12 Thread Vladimir Kempik
On Thu, 4 Feb 2021 22:54:42 GMT, Gerard Ziemski  wrote:

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

This is the version of w^x on-demand switch implemented by microsoft guys. This 
is enabled only for debug builds.
@lewurm could you comment here please

-

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


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

2021-02-12 Thread Vladimir Kempik
On Tue, 2 Feb 2021 22:07:15 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 195:
> 
>> 193: frame os::get_sender_for_C_frame(frame* fr) {
>> 194:   return frame(fr->link(), fr->link(), fr->sender_pc());
>> 195: }
> 
> Is this file going to be built by GCC or just macOS compilers?

there is no support for compiling java with gcc on macos since about jdk11, 
only clang.
considering this and the absence of gcc for macos_m1, the answer is - just 
macOS compilers.

-

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


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

2021-02-05 Thread Anton Kozlov
On Tue, 2 Feb 2021 18:35:51 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
>
> 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?

They are defined in 13.2.95. MIDR_EL1, Main ID Register. Apple's code is not 
there, but "Arm can assign codes that are not published in this manual. All 
values not assigned by Arm are reserved and must not be used.". I assume the 
value was obtained by digging around 
https://github.com/apple/darwin-xnu/blob/main/osfmk/arm/cpuid.h#L62

-

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


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

2021-02-05 Thread Anton Kozlov
On Wed, 3 Feb 2021 23:29:30 GMT, Gerard Ziemski  wrote:

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

Thanks! I've updated the PR with this code, with extra indentation of 
`AARCH64_ONLY(...)` line, since this is continuation of the first parameter.

I'll fix the formatting in os_bsd_arch64.cpp along other changes to 
`bsd_aarch64` directory

-

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


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

2021-02-05 Thread daniel . daugherty

On 2/5/21 4:51 AM, Magnus Ihse Bursie wrote:

On Tue, 2 Feb 2021 21:20:52 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

make/autoconf/flags.m4 line 140:


138: else
139:   MACOSX_VERSION_MIN=10.12.0
140: fi

Not something that needs to be addressed here, but these changes
illustrate that our collective use of macOSX/MACOSX/MacOSX names
are tied to the fact that the macOS major version number was at 10
for a very long time.

@magicus - Do we have an RFE to rename MACOSX or are we sticking
with it and evolving our interpretation of the 'X' from '10' to */splat/asterik?

@dcubed-ojdk There is no RFE to renaming "macosx" to "macos". I'm not sure it should be done. We 
can't follow all marketing trends (Apple recently renamed iOS to iPadOS for the iPad; we can't keep adapting to such 
schemes). Personally, I like the new name without the "x", but we had already spent some time trying to find 
and fix all (or at least, most) instances of "osx" in the code, that I don't really think it's worth the 
effort.

If you can drill up enough enthusiasm for such a project, and get any 
objections down to minimum, I can help implementing it. But I won't be 
spearheading it.


make/common/NativeCompilation.gmk line 1178:


1176: endif
1177: # This only works if the openjdk_codesign identity is 
present on the system. Let
1178: # silently fail otherwise.

Might want to add a comment here:
 #  The '-f' option will replace an existing signature if one exists.

We're not really in the habit of adding comments for various command line options. Normally, you 
can check these with "man" if you are uncertain. If they do something surprising, sure, 
but here it's more of a "it's needed on aarch64 to work at all", so I don't think a 
comment will be anything but added clutter.

-

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


@magicus - I'm good with both of these answers. I personally like 'macosx'.

Dan


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

2021-02-05 Thread Magnus Ihse Bursie
On Tue, 2 Feb 2021 21:20:52 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
>
> make/autoconf/flags.m4 line 140:
> 
>> 138: else
>> 139:   MACOSX_VERSION_MIN=10.12.0
>> 140: fi
> 
> Not something that needs to be addressed here, but these changes
> illustrate that our collective use of macOSX/MACOSX/MacOSX names
> are tied to the fact that the macOS major version number was at 10
> for a very long time.
> 
> @magicus - Do we have an RFE to rename MACOSX or are we sticking
> with it and evolving our interpretation of the 'X' from '10' to 
> */splat/asterik?

@dcubed-ojdk There is no RFE to renaming "macosx" to "macos". I'm not sure it 
should be done. We can't follow all marketing trends (Apple recently renamed 
iOS to iPadOS for the iPad; we can't keep adapting to such schemes). 
Personally, I like the new name without the "x", but we had already spent some 
time trying to find and fix all (or at least, most) instances of "osx" in the 
code, that I don't really think it's worth the effort. 

If you can drill up enough enthusiasm for such a project, and get any 
objections down to minimum, I can help implementing it. But I won't be 
spearheading it.

> make/common/NativeCompilation.gmk line 1178:
> 
>> 1176: endif
>> 1177: # This only works if the openjdk_codesign identity is 
>> present on the system. Let
>> 1178: # silently fail otherwise.
> 
> Might want to add a comment here:
> #  The '-f' option will replace an existing signature if one exists.

We're not really in the habit of adding comments for various command line 
options. Normally, you can check these with "man" if you are uncertain. If they 
do something surprising, sure, but here it's more of a "it's needed on aarch64 
to work at all", so I don't think a comment will be anything but added clutter.

-

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


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

2021-02-04 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-04 Thread Vladimir Kempik
On Thu, 4 Feb 2021 15:13:49 GMT, Anton Kozlov  wrote:

>> Out of curiosity, have you looked at the performance of the W^X state 
>> transition? In particular I'm wondering if the cost is effectively 
>> negligible so doing it unconditionally on JVM entry is a no-brainer and just 
>> easier/cleaner than the alternatives, or if there are reasons to look at 
>> only doing the transition if/when needed (perhaps do it lazily and revert 
>> back to X when leaving the JVM?).
>
>> Out of curiosity, have you looked at the performance of the W^X state 
>> transition?
> 
> Earlier it was possible to disable W^X protection (unfortunately, I don't 
> know a way to do this now). We compared Renaissance results with W^X 
> transitions like the proposed one vs. no transitions with the protection 
> disabled once. Results were identical for a small and large number of 
> iterations.
> 
> From the other hand, I've used 
> https://github.com/AntonKozlov/macos-aarch64-transition-bench to estimate the 
> cost of the transition.
> I re-did measurements with the current implementation and on consumer 
> hardware:
> 
> testJNIthrpt   25   277997000.151 Β±   4095685.956  ops/s
> testJniNanoTimethrpt   2517851098.010 Β±119489.599  ops/s
> testNanoTime   thrpt   2578007491.762 Β±628455.971  ops/s
> testNothingthrpt   25  1724298829.088 Β± 100537565.068  ops/s
> testTwoStateAndWX  thrpt   2521958839.057 Β±210490.755  ops/s
> testWX thrpt   2523299813.266 Β±149837.302  ops/s
> 
> There is an overhead, but it does not look like blocking the first 
> implementation. I'm not refusing future optimizations like enabling W only 
> when necessary. But for now, I don't have a robust and maintainable solution 
> for this, sorry.

> _Mailing list message from [erik.joelsson at 
> oracle.com](mailto:erik.joels...@oracle.com) on 
> [2d-dev](mailto:2d-...@openjdk.java.net):_
> 
> 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

This doesn't seem to be an issue anymore, After P.Race have finished with 
JDK-8257852, Macarm port can be build without extra ld flags. J2Demo works fine 
as example.
This can be checked if one merges pull/2200 branch into his local copy of 
master branch.

-

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


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

2021-02-04 Thread Anton Kozlov
On Wed, 3 Feb 2021 20:08:28 GMT, Mikael Vidstedt  wrote:

> Out of curiosity, have you looked at the performance of the W^X state 
> transition?

Earlier it was possible to disable W^X protection (unfortunately, I don't know 
a way to do this now). We compared Renaissance results with W^X transitions 
like the proposed one vs. no transitions with the protection disabled once. 
Results were identical for a small and large number of iterations.

>From the other hand, I've used 
>https://github.com/AntonKozlov/macos-aarch64-transition-bench to estimate the 
>cost of the transition.
I re-did measurements with the current implementation and on consumer hardware:

testJNIthrpt   25   277997000.151 Β±   4095685.956  ops/s
testJniNanoTimethrpt   2517851098.010 Β±119489.599  ops/s
testNanoTime   thrpt   2578007491.762 Β±628455.971  ops/s
testNothingthrpt   25  1724298829.088 Β± 100537565.068  ops/s
testTwoStateAndWX  thrpt   2521958839.057 Β±210490.755  ops/s
testWX thrpt   2523299813.266 Β±149837.302  ops/s

There is an overhead, but it does not look like blocking the first 
implementation. I'm not refusing future optimizations like enabling W only when 
necessary. But for now, I don't have a robust and maintainable solution for 
this, sorry.

-

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


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

2021-02-04 Thread Vladimir Kempik
On Thu, 4 Feb 2021 14:27:53 GMT, Andrew Haley  wrote:

> > > You read my mind, Andrew. Unless, of course, it's optimized to leverage 
> > > the fact that it's thread-specific..
> > 
> > 
> > it's thread-specific
> > https://developer.apple.com/documentation/apple_silicon/porting_just-in-time_compilers_to_apple_silicon
> > > Because pthread_jit_write_protect_np changes only the current thread’s 
> > > permissions, avoid accessing the same memory region from multiple 
> > > threads. Giving multiple threads access to the same memory region opens 
> > > up a potential attack vector, in which one thread has write access and 
> > > another has executable access to the same region.
> 
> Umm, so how does patching work? We don't even know if other threads are 
> executing the code we need to patch.

I thought java can handle that scenario in usual (non W^X systems) just fine, 
so we just believe jvm did everything right and it's safe to rewrite some code 
at specific moment.

-

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


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

2021-02-04 Thread Andrew Haley
On Thu, 4 Feb 2021 09:49:17 GMT, Vladimir Kempik  wrote:

> > You read my mind, Andrew. Unless, of course, it's optimized to leverage the 
> > fact that it's thread-specific..
> 
> it's thread-specific
> 
> https://developer.apple.com/documentation/apple_silicon/porting_just-in-time_compilers_to_apple_silicon
> 
> > Because pthread_jit_write_protect_np changes only the current thread’s 
> > permissions, avoid accessing the same memory region from multiple threads. 
> > Giving multiple threads access to the same memory region opens up a 
> > potential attack vector, in which one thread has write access and another 
> > has executable access to the same region.

Umm, so how does patching work? We don't even know if other threads are 
executing the code we need to patch.

-

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


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

2021-02-04 Thread Anton Kozlov
On Tue, 2 Feb 2021 22:56:55 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/share/runtime/stubRoutines.inline.hpp line 1:
> 
>> 1: /*
> 
> NOW I understand the reason for switching from include to inline-include.
> Is there a reason that this change is part of this project and not extracted
> into a separate RFE. That would reduce the number of files touched by
> this project.

Makes sense, thanks. I'll do this as JDK-8261075.

-

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


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

2021-02-04 Thread Vladimir Kempik
On Thu, 4 Feb 2021 08:26:35 GMT, Mikael Vidstedt  wrote:

> You read my mind, Andrew. Unless, of course, it's optimized to leverage the 
> fact that it's thread-specific..

it's thread-specific

https://developer.apple.com/documentation/apple_silicon/porting_just-in-time_compilers_to_apple_silicon

>Because pthread_jit_write_protect_np changes only the current thread’s 
>permissions, avoid accessing the same memory region from multiple threads. 
>Giving multiple threads access to the same memory region opens up a potential 
>attack vector, in which one thread has write access and another has executable 
>access to the same region.

-

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


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

2021-02-04 Thread Mikael Vidstedt
On Wed, 3 Feb 2021 20:08:28 GMT, Mikael Vidstedt  wrote:

>>> I wonder if this is the right choice
>>> ...
>>> ```
>>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>>> ...
>>> ```
>>> 
>> 
>> The transition in OopStorageParIterPerf was made for gtest setup to execute 
>> in WXWrite context. For tests themselves, defining macro set WXWrite.
>> 
>> I've simplified the scheme and now we switch to WXWrite once at the gtest 
>> launcher. So this transition was dropped.
>> 
>> I've also refreshed my memory and tried to switch to WXWrite as close as 
>> possible to each place where we'll be writing executable memory. There are a 
>> lot of such places! As you correctly noted, code cache contains objects, not 
>> plain data. For example, CodeCache memory management structures, 
>> CompiledMethod, ...  are there, so we need more WXWrite switches than we 
>> have in the current approach. I had a comparable amount of them just to run 
>> -version, but certainly not enough to run tier1 tests.
>> 
>> Following your advice, I don't require a known "from" state anymore. So a 
>> few W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
>> function, which expects to be called from the native code. I had to switch 
>> to WXExec just only to satisfy the expectations. After the update, we don't 
>> need this anymore.
>> 
>> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
>> functions are not marked as entries for some reason, although they are 
>> called directly from e.g. interpreter. I added W^X management to such 
>> functions.
>> 
>> Thank you!
>
> Out of curiosity, have you looked at the performance of the W^X state 
> transition? In particular I'm wondering if the cost is effectively negligible 
> so doing it unconditionally on JVM entry is a no-brainer and just 
> easier/cleaner than the alternatives, or if there are reasons to look at only 
> doing the transition if/when needed (perhaps do it lazily and revert back to 
> X when leaving the JVM?).

You read my mind, Andrew. Unless, of course, it's optimized to leverage the 
fact that it's thread-specific..

-

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


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

2021-02-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-03 Thread Bernhard Urban-Forster
On Wed, 3 Feb 2021 22:48:33 GMT, Gerard Ziemski  wrote:

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

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?

-

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


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

2021-02-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-03 Thread Bernhard Urban-Forster
On Wed, 3 Feb 2021 20:29:48 GMT, Gerard Ziemski  wrote:

>> 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`?

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 πŸ™‚

-

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


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

2021-02-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-03 Thread Anton Kozlov
On Wed, 3 Feb 2021 09:14:24 GMT, Andrew Haley  wrote:

>> 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 
>> ?
>
> Surely this should be 
> 
>  saved_regs = RegSet::range(r0, r1) BSD_ONLY(+ RegSet::range(r2, r17)) + lr - 
> dst;```
> 
> Shouldn't it?

Interesting, I wonder why it has built successfully on Linux. I'm going to fix 
this under as JDK-8261072

-

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


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

2021-02-03 Thread Anton Kozlov
On Wed, 3 Feb 2021 19:57:24 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.
>
> Thank you all for your comments regarding W^X implementation. I've made a 
> change that reduces the footprint of the implementation, also addressing most 
> of the comments. I'll revisit them individually to make sure nothing is 
> forgotten.
> 
> The basic principle has not changed: when we execute JVM code (owned by 
> libjvm.so, starting from JVM entry function), we switch to Write state. When 
> we leave JVM to execute generated or JNI code, we switch to Executable state. 
> I would like to highlight that JVM code does not mean the VM state of the 
> java thread. After @stefank's suggestion, I could also drop a few W^X state 
> switches, so now it should be more clear that switches are tied to JVM entry 
> functions.

> I wonder if this is the right choice
> ...
> ```
> OopStorageParIterPerf::~OopStorageParIterPerf() {
> ...
> ```
> 

The transition in OopStorageParIterPerf was made for gtest setup to execute in 
WXWrite context. For tests themselves, defining macro set WXWrite.

I've simplified the scheme and now we switch to WXWrite once at the gtest 
launcher. So this transition was dropped.

I've also refreshed my memory and tried to switch to WXWrite as close as 
possible to each place where we'll be writing executable memory. There are a 
lot of such places! As you correctly noted, code cache contains objects, not 
plain data. For example, CodeCache memory management structures, 
CompiledMethod, ...  are there, so we need more WXWrite switches than we have 
in the current approach. I had a comparable amount of them just to run 
-version, but certainly not enough to run tier1 tests.

Following your advice, I don't require a known "from" state anymore. So a few 
W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
function, which expects to be called from the native code. I had to switch to 
WXExec just only to satisfy the expectations. After the update, we don't need 
this anymore.

W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
functions are not marked as entries for some reason, although they are called 
directly from e.g. interpreter. I added W^X management to such functions.

-

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


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

2021-02-03 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-03 Thread Anton Kozlov
On Tue, 2 Feb 2021 23:03:45 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/share/runtime/thread.cpp line 3991:
> 
>> 3989:   JavaThread* thread = JavaThread::current();
>> 3990:   ThreadToNativeFromVM ttn(thread);
>> 3991:   Thread::WXExecFromWriteSetter wx_exec;
> 
> I saw somewhere in this review a comment about why this new
> WXExecFromWriteSetter helper isn't folded into ThreadToNativeFromVM
> and I understand that not every current ThreadToNativeFromVM needs
> the new helper. If the vast majority of the ThreadToNativeFromVM
> locations need WXExecFromWriteSetter, then perhaps those locations
> that currently have a ThreadToNativeFromVM followed by the new
> WXExecFromWriteSetter should use a new helper:
> 
> ThreadToNativeWithWXExecFromVM
> 
> so we'll see changes from:
> 
> ThreadToNativeFromVM -> ThreadToNativeWithWXExecFromVM
> 
> where we need them and hopefully a short comment can be added
> at the same time to explain the need for WXExec. This will allow us
> to easily distinguish ThreadToNativeFromVM locations that DO NOT
> need WXExec from those that DO need it.

With a small overhead for tracking the current W^X state, I avoided 
WXExecFromWriteSetter near ThreadToNativeFromVM at all. New 
ThreadWXEnable(WXExec) is used only to call a generated function. More common 
ThreadWXEnable(WXWrite) is tied to JVM entry functions. I added comments for 
functions that are not clear to be entries, although they are. Thank you for 
the suggestion!

-

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


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

2021-02-03 Thread Anton Kozlov
On Tue, 2 Feb 2021 18:00:06 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
>
> 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.

Agree. I'm going to do as much refactoring as needed before this patch under 
JDK-8261071

-

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


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

2021-02-03 Thread Mikael Vidstedt
On Wed, 3 Feb 2021 20:05:29 GMT, Anton Kozlov  wrote:

>> Thank you all for your comments regarding W^X implementation. I've made a 
>> change that reduces the footprint of the implementation, also addressing 
>> most of the comments. I'll revisit them individually to make sure nothing is 
>> forgotten.
>> 
>> The basic principle has not changed: when we execute JVM code (owned by 
>> libjvm.so, starting from JVM entry function), we switch to Write state. When 
>> we leave JVM to execute generated or JNI code, we switch to Executable 
>> state. I would like to highlight that JVM code does not mean the VM state of 
>> the java thread. After @stefank's suggestion, I could also drop a few W^X 
>> state switches, so now it should be more clear that switches are tied to JVM 
>> entry functions.
>
>> I wonder if this is the right choice
>> ...
>> ```
>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>> ...
>> ```
>> 
> 
> The transition in OopStorageParIterPerf was made for gtest setup to execute 
> in WXWrite context. For tests themselves, defining macro set WXWrite.
> 
> I've simplified the scheme and now we switch to WXWrite once at the gtest 
> launcher. So this transition was dropped.
> 
> I've also refreshed my memory and tried to switch to WXWrite as close as 
> possible to each place where we'll be writing executable memory. There are a 
> lot of such places! As you correctly noted, code cache contains objects, not 
> plain data. For example, CodeCache memory management structures, 
> CompiledMethod, ...  are there, so we need more WXWrite switches than we have 
> in the current approach. I had a comparable amount of them just to run 
> -version, but certainly not enough to run tier1 tests.
> 
> Following your advice, I don't require a known "from" state anymore. So a few 
> W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
> function, which expects to be called from the native code. I had to switch to 
> WXExec just only to satisfy the expectations. After the update, we don't need 
> this anymore.
> 
> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
> functions are not marked as entries for some reason, although they are called 
> directly from e.g. interpreter. I added W^X management to such functions.

Out of curiosity, have you looked at the performance of the W^X state 
transition? In particular I'm wondering if the cost is effectively negligible 
so doing it unconditionally on JVM entry is a no-brainer and just 
easier/cleaner than the alternatives, or if there are reasons to look at only 
doing the transition if/when needed (perhaps do it lazily and revert back to X 
when leaving the JVM?).

-

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


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

2021-02-03 Thread Anton Kozlov
On Tue, 2 Feb 2021 23:10:17 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
>
> 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.

Thank you all for your comments regarding W^X implementation. I've made a 
change that reduces the footprint of the implementation, also addressing most 
of the comments. I'll revisit them individually to make sure nothing is 
forgotten.

The basic principle has not changed: when we execute JVM code (owned by 
libjvm.so, starting from JVM entry function), we switch to Write state. When we 
leave JVM to execute generated or JNI code, we switch to Executable state. I 
would like to highlight that JVM code does not mean the VM state of the java 
thread. After @stefank's suggestion, I could also drop a few W^X state 
switches, so now it should be more clear that switches are tied to JVM entry 
functions.

-

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


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

2021-02-03 Thread Andrew Haley
On Tue, 2 Feb 2021 21:49:36 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/cpu/aarch64/macroAssembler_aarch64.cpp line 323:
> 
>> 321:   str(zr, Address(rthread, JavaThread::last_Java_pc_offset()));
>> 322: 
>> 323:   str(zr, Address(rthread, JavaFrameAnchor::saved_fp_address_offset()));
> 
> I don't think this switch from `JavaThread::saved_fp_address_offset()`
> to `JavaFrameAnchor::saved_fp_address_offset()` is correct since
> `rthread` is still used and is a JavaThread*. The new code will give you:
> 
> `rthread` + offset of the `saved_fp_address` field in a JavaFrameAnchor
> 
> The old code gave you:
> 
> `rthread` + offset of the `saved_fp_address` field in the JavaFrameAnchor 
> field in the JavaThread
> 
> Those are not the same things.

I agree, I don't understand why this change was made.

> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.hpp line 45:
> 
>> 43:   // Atomically copy 64 bits of data
>> 44:   static void atomic_copy64(const volatile void *src, volatile void 
>> *dst) {
>> 45: *(jlong *) dst = *(const jlong *) src;
> 
> Is this construct actually atomic on aarch64?

Yes.

> src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.hpp line 37:
> 
>> 35: 
>> 36: private:
>> 37: 
> 
> 'private' usually has one space in front of it.
> Also, why the blank line after it?

It reads better with the blank line, and it's not in violation of HotSpot 
conventions.

-

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


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

2021-02-03 Thread Andrew Haley
On Tue, 2 Feb 2021 18:03:50 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
>
> 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 ?

Surely this should be 

 saved_regs = RegSet::range(r0, r1) BSD_ONLY(+ RegSet::range(r2, r17)) + lr - 
dst;```

Shouldn't it?

-

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


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

2021-02-02 Thread Daniel D . Daugherty
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

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.

make/autoconf/flags.m4 line 140:

> 138: else
> 139:   MACOSX_VERSION_MIN=10.12.0
> 140: fi

Not something that needs to be addressed here, but these changes
illustrate that our collective use of macOSX/MACOSX/MacOSX names
are tied to the fact that the macOS major version number was at 10
for a very long time.

@magicus - Do we have an RFE to rename MACOSX or are we sticking
with it and evolving our interpretation of the 'X' from '10' to */splat/asterik?

make/common/NativeCompilation.gmk line 1178:

> 1176: endif
> 1177: # This only works if the openjdk_codesign identity is 
> present on the system. Let
> 1178: # silently fail otherwise.

Might want to add a comment here:
#  The '-f' option will replace an existing signature if one exists.

src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 41:

> 39: #define __ _masm->
> 40: 
> 41: //describe amount of space in bytes occupied by type on native stack

nit - needs a space after `//`

src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 83:

> 81: // On macos/aarch64 native stack is packed, int/float are using only 4 
> bytes
> 82: // on stack. Natural alignment for types are still in place,
> 83: // for example double/long should be 8 bytes alligned

nit typo: s/alligned/aligned/
And sentence should end with a period.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 323:

> 321:   str(zr, Address(rthread, JavaThread::last_Java_pc_offset()));
> 322: 
> 323:   str(zr, Address(rthread, JavaFrameAnchor::saved_fp_address_offset()));

I don't think this switch from `JavaThread::saved_fp_address_offset()`
to `JavaFrameAnchor::saved_fp_address_offset()` is correct since
`rthread` is still used and is a JavaThread*. The new code will give you:

`rthread` + offset of the `saved_fp_address` field in a JavaFrameAnchor

The old code gave you:

`rthread` + offset of the `saved_fp_address` field in the JavaFrameAnchor 
field in the JavaThread

Those are not the same things.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 31:

> 29: #include "asm/assembler.inline.hpp"
> 30: #include "oops/compressedOops.hpp"
> 31: #include "runtime/vm_version.hpp"

It's not clear why this include needed to be added.

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 810:

> 808: #ifdef __APPLE__
> 809:   // Less-than word types are stored one after another.
> 810:   // The code unable to handle this, bailout.

Perhaps:  // The code is unable to handle this so bailout.

src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 837:

> 835: #ifdef __APPLE__
> 836:   // Less-than word types are stored one after another.
> 837:   // The code unable to handle this, bailout.

Perhaps: // The code is unable to handle this so bailout.

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

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

It is not clear wh

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

2021-02-02 Thread Bernhard Urban-Forster
On Tue, 2 Feb 2021 18:23:04 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
>
> 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`.

-

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


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

2021-02-02 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-02 Thread 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: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

2021-02-02 Thread 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 one additional 
commit since the last revision:

  support macos_aarch64 in hsdis

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2200/files
  - new: https://git.openjdk.java.net/jdk/pull/2200/files/b421e0b4..3c705ae5

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

  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 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