Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253795: Implementation of JEP 391: macOS/AArch64 Port #2200

Closed
wants to merge 117 commits into from
Closed
Changes from all commits
Commits
Show all changes
117 commits
Select commit Hold shift + click to select a range
4cbb953
Import JDK-8253015 v1: Aarch64: Move linux code out from generic CPU …
AntonKozlov Sep 24, 2020
3b98dba
JDK-8248500: AArch64: Remove the r18 dependency on Windows AArch64
AntonKozlov Aug 20, 2020
7092a06
Import JDK-8234930 v1: Use MAP_JIT when allocating pages for code cac…
AntonKozlov Aug 25, 2020
368f06e
JDK-8253817: Support macOS Aarch64 ABI in Interpreter
AntonKozlov Aug 24, 2020
f888acc
JDK-8253816: Support macOS W^X
AntonKozlov Aug 10, 2020
80847e9
JDK-8253839: Update tests and JDK code for macOS/Aarch64
Sep 10, 2020
f9cdb3f
JDK-8253819: Copy linux_aarch64 to bsd_aarch64
AntonKozlov Sep 30, 2020
58a63b6
JDK-8253819: Add missing parts from bsd_x86
AntonKozlov Sep 30, 2020
39042c4
JDK-8253816: W^X part for bsd_aarch64
AntonKozlov Sep 30, 2020
8ab282d
JDK-8253819: Finish the bsd_aarch64
AntonKozlov Oct 1, 2020
f69c83a
Merge remote-tracking branch 'jdk-sandbox/JEP-391-branch' into HEAD
luhenry Oct 9, 2020
82895a8
Add macOS-AArch64 build and platform support code (#2)
luhenry Nov 3, 2020
4994873
Minor clean-up in #2 (#4)
AntonKozlov Nov 11, 2020
d81cca3
Revert "Take f69c83ad6b part for MAP_JIT"
AntonKozlov Nov 11, 2020
0ab1e1c
Revert "Import JDK-8234930 v2: Use MAP_JIT when allocating pages for …
AntonKozlov Nov 11, 2020
f7c60d7
Merge remote-tracking branch 'upstream/master' into macos-aarch64-public
AntonKozlov Nov 12, 2020
051357e
Fix merge
AntonKozlov Nov 10, 2020
33c4761
Add missing include StubRoutines inline
AntonKozlov Nov 12, 2020
b41b3a9
Import JDK-8234930 v3: Use MAP_JIT when allocating pages for code cac…
AntonKozlov Aug 25, 2020
8738c21
Merge remote-tracking branch 'upstream/master' into jdk-macos
luhenry Nov 17, 2020
b8df9dd
JDK-8253816: Add missing WX transition
AntonKozlov Nov 10, 2020
e3ab996
JDK-8254941: Update SA implementation
AntonKozlov Nov 11, 2020
f037321
JDK-8255776: Change build system for macOS/AArch64
VladimirKempik Oct 12, 2020
9a75bc3
JDK-8253816: Fix W^X transitions in gtests
AntonKozlov Nov 2, 2020
0008b77
JDK-8254941: Implement SA
AntonKozlov Nov 2, 2020
5feabb4
JDK-8253839: Do not use objc_msgSend_stret
AntonKozlov Aug 7, 2020
7d8c1e1
JDK-8255776: Make tier1 is broken with xcode12
Jul 24, 2020
1cc5358
JDK-8255776: Make codesign to rewrite signature when signing
Sep 10, 2020
0d65571
JDK-8253819: Disable aot on macarm until it's ready
Sep 11, 2020
bc80064
JDK-8253819: Disable CDS on macarm
VladimirKempik Oct 15, 2020
fe696e6
JDK-8253819: MacARM: CPU feature detection
AntonKozlov Sep 24, 2020
3bdd1f9
JDK-8253839: gssapi.h needs to be updated for aarch64
VladimirKempik Sep 21, 2020
355c9c1
JDK-8253839: Fix NativeCallStack for aarch64
VladimirKempik Oct 7, 2020
b01425e
Fixes after review
AntonKozlov Nov 18, 2020
47396be
Add Xcode reference in gssapi.h
AntonKozlov Nov 19, 2020
8383f41
JDK-8253816: Switch to Exec W^X mode after JNI DetachCurrentThread (#10)
AntonKozlov Dec 17, 2020
4885779
Fix libcodegenAttach build failures on various OS
AntonKozlov Jan 15, 2021
99a3dce
Revert "Import JDK-8234930 v3: Use MAP_JIT when allocating pages for …
AntonKozlov Jan 15, 2021
4fa5c96
Merge remote-tracking branch 'upstream/master' into jdk-macos
AntonKozlov Jan 15, 2021
85f76eb
JDK-8221554: bsd_aarch64 part
AntonKozlov Nov 30, 2020
7fe50a9
JDK-8257882: bsd_aarch64 part
AntonKozlov Jan 15, 2021
80ad49a
JDK-8253742: bsd_aarch64 part
AntonKozlov Nov 30, 2020
ec1def6
Use r18_tls instead of r18_reserved
AntonKozlov Jan 18, 2021
c88419a
Fix windows_aarch64
AntonKozlov Jan 18, 2021
3a482df
Merge remote-tracking branch 'upstream/master' into jdk-macos
AntonKozlov Jan 19, 2021
a71bac7
Fix gtests in debug
AntonKozlov Jan 20, 2021
943b93f
Revert gtest changes
AntonKozlov Jan 20, 2021
f0f62da
Rework gtests to always have wx_write
AntonKozlov Jan 21, 2021
ecbf70f
JDK-8253816: Update after recent changes
AntonKozlov Jan 21, 2021
3d4f4c2
Fix build
AntonKozlov Jan 21, 2021
b90c13b
Enable -Wformat-nonliteral back
AntonKozlov Jan 23, 2021
50b55f6
Address feedback for signature generators
AntonKozlov Jan 23, 2021
b3adff5
Redo builsys support for aarch64-darwin
VladimirKempik Jan 25, 2021
0c2cb0a
Refactor CDS disabling
VladimirKempik Jan 25, 2021
a8d8f4c
Revert "Address feedback for signature generators"
AntonKozlov Jan 26, 2021
cd794ba
Partially bring previous commit
AntonKozlov Jan 26, 2021
fef3658
Little adjustement of SlowSignatureHandler
AntonKozlov Jan 26, 2021
b2b396f
Revert harfbuzz changes, disable warnings for it
AntonKozlov Jan 26, 2021
f1ef624
Redo buildsys fix
VladimirKempik Jan 26, 2021
3d7ef7b
Fix inclusing of StubRoutines header
VladimirKempik Jan 27, 2021
9d8b07c
Update copyright year for BsdAARCH64ThreadContext.java
VladimirKempik Jan 27, 2021
b61e633
Revert w^x in gtests
AntonKozlov Jan 29, 2021
b421e0b
Merge branch 'master' into jdk-macos
VladimirKempik Jan 31, 2021
3c705ae
support macos_aarch64 in hsdis
VladimirKempik Feb 2, 2021
3e7b08d
Do not require known W^X state
AntonKozlov Feb 1, 2021
741d070
Add W^X to tests
AntonKozlov Feb 2, 2021
e218beb
Use macro conditionals instead of empty functions
AntonKozlov Feb 3, 2021
7e4d85d
Add comments to WX transitions
AntonKozlov Feb 3, 2021
8082717
Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
AntonKozlov Feb 3, 2021
8652d21
Cleanup SA changes
AntonKozlov Feb 5, 2021
b873c25
Extract SafeFetch32/N to stubRoutines.inline.hpp
AntonKozlov Feb 5, 2021
0d0e9ba
Update signal handler part for debugger
AntonKozlov Feb 5, 2021
6936894
Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubr…
AntonKozlov Feb 10, 2021
d93c0ae
Merge remote-tracking branch 'upstream/jdk/master' into 8261075-stubr…
AntonKozlov Feb 11, 2021
a00d906
Update copyrights
AntonKozlov Feb 11, 2021
ad4e4c6
JDK-8257882: oops, fixed 7fe50a996b6f436932452d220b351c73153ed945
AntonKozlov Feb 12, 2021
4094f35
Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
AntonKozlov Feb 15, 2021
43a2cae
Merge branch 'master' into jdk-macos
VladimirKempik Feb 15, 2021
a9452a4
Pull/2200 (#5)
VladimirKempik Feb 15, 2021
f4426e7
Fix typo in comments
VladimirKempik Feb 15, 2021
419c2b1
Merge pull request #6 from VladimirKempik/pull/2200
VladimirKempik Feb 15, 2021
daf35f0
Removed unused variables
VladimirKempik Feb 15, 2021
90e244e
Merge pull request #9 from VladimirKempik/pull/2200
VladimirKempik Feb 15, 2021
d2957b9
stubRoutines.inline.hpp -> safefetch.hpp
AntonKozlov Feb 15, 2021
4f1f43e
Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp"
AntonKozlov Feb 16, 2021
eb9ea4d
Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline'…
AntonKozlov Feb 16, 2021
07a499c
Re-do safefetch.hpp
AntonKozlov Feb 17, 2021
ab72613
Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
AntonKozlov Feb 17, 2021
74687c0
Merge branch 'master' into jdk-macos
VladimirKempik Feb 26, 2021
5bf2d82
Fix build after merge with master
VladimirKempik Feb 26, 2021
241aede
Merge pull request #10 from VladimirKempik/pull/2200
VladimirKempik Feb 26, 2021
56416b5
Minor fixes
AntonKozlov Feb 26, 2021
663cb4a
Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
AntonKozlov Feb 26, 2021
e42b82d
Update comments
AntonKozlov Mar 2, 2021
097cd89
Cleanup os_bsd_aarch64 signal handling
AntonKozlov Mar 2, 2021
e538ae6
JDK-8257828: bsd_aarch64 part
AntonKozlov Mar 2, 2021
74063fa
JDK-8259539: bsd_aarch64 part
AntonKozlov Mar 2, 2021
5ba8ba7
JDK-8260471: bsd_aarch64 part
AntonKozlov Mar 2, 2021
4c37f06
Fix after JDK-8259539, partially revert preconditions
AntonKozlov Mar 2, 2021
489a03c
Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
AntonKozlov Mar 9, 2021
d178376
workaround JDK-8262895 by disabling subtest
VladimirKempik Mar 9, 2021
416e433
JDK-8259937: bsd_aarch64 part
AntonKozlov Mar 9, 2021
acda02f
Rename threadWXSetters.hpp -> threadWXSetters.inline.hpp
AntonKozlov Mar 9, 2021
c4947cc
Fix typo
AntonKozlov Mar 9, 2021
a72f683
Merge commit 'refs/pull/11/head' of https://github.com/AntonKozlov/jd…
AntonKozlov Mar 9, 2021
f6fb01b
8262903: [macos_aarch64] Thread::current() called on detached thread
AntonKozlov Mar 10, 2021
fd4812e
Use Thread::current_or_null_safe in SafeFetch
AntonKozlov Mar 12, 2021
127c60e
Update Oracle copyright years
AntonKozlov Mar 12, 2021
29991c9
Add Azul copyright
AntonKozlov Mar 12, 2021
5bfe0f0
Fix most of issues in java/foreign/ tests
VladimirKempik Mar 12, 2021
3d0f4d2
Wider #ifdef block
AntonKozlov Mar 15, 2021
117dd68
Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
AntonKozlov Mar 15, 2021
6e39762
JDK-8263002: bsd_aarch64 part
AntonKozlov Mar 15, 2021
806fc61
JDK-8262491: bsd_aarch64 part
AntonKozlov Mar 15, 2021
5add926
Merge branch 'master' into jdk-macos
VladimirKempik Mar 22, 2021
b1c516d
Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
AntonKozlov Mar 25, 2021
d362996
JDK-8261397: bsd_aarch64 part
AntonKozlov Mar 25, 2021
File filter
Filter file types
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -1,6 +1,7 @@
#!/bin/sh
#
# Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2021, Azul Systems, Inc. All rights reserved.
# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
@@ -101,6 +102,14 @@ if [ "x$OUT" = x ]; then
fi
fi

# Test and fix cpu on macos-aarch64, uname -p reports arm, buildsys expects aarch64
echo $OUT | grep arm-apple-darwin > /dev/null 2> /dev/null
if test $? = 0; then
if [ `uname -m` = arm64 ]; then
OUT=aarch64`echo $OUT | sed -e 's/[^-]*//'`
fi
fi

# Test and fix cpu on Macosx when C preprocessor is not on the path
echo $OUT | grep i386-apple-darwin > /dev/null 2> /dev/null
if test $? = 0; then
@@ -125,19 +125,25 @@ AC_DEFUN([FLAGS_SETUP_MACOSX_VERSION],
[
# Additional macosx handling
if test "x$OPENJDK_TARGET_OS" = xmacosx; then
# The expected format for <version> is either nn.n.n or nn.nn.nn. See
# /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/AvailabilityVersions.h
# MACOSX_VERSION_MIN specifies the lowest version of Macosx that the built
# binaries should be compatible with, even if compiled on a newer version
# of the OS. It currently has a hard coded value. Setting this also limits
# exposure to API changes in header files. Bumping this is likely to
# require code changes to build.
MACOSX_VERSION_MIN=10.12.0
if test "x$OPENJDK_TARGET_CPU_ARCH" = xaarch64; then
MACOSX_VERSION_MIN=11.00.00
else
MACOSX_VERSION_MIN=10.12.0
fi
Comment on lines -133 to +140

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Feb 2, 2021
Member

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?

This comment has been minimized.

@magicus

magicus Feb 5, 2021
Member

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

MACOSX_VERSION_MIN_NODOTS=${MACOSX_VERSION_MIN//\./}
AC_SUBST(MACOSX_VERSION_MIN)
# Setting --with-macosx-version-max=<version> makes it an error to build or
# link to macosx APIs that are newer than the given OS version. The expected
# format for <version> is either nn.n.n or nn.nn.nn. See /usr/include/AvailabilityMacros.h.
# link to macosx APIs that are newer than the given OS version.
AC_ARG_WITH([macosx-version-max], [AS_HELP_STRING([--with-macosx-version-max],
[error on use of newer functionality. @<:@macosx@:>@])],
[
@@ -1,5 +1,5 @@
#
# Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
@@ -242,7 +242,7 @@ AC_DEFUN_ONCE([JVM_FEATURES_CHECK_AOT],
elif test "x$OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU" = "xlinux-aarch64"; then
AC_MSG_RESULT([yes])
else
AC_MSG_RESULT([no, $OPENJDK_TARGET_CPU])
AC_MSG_RESULT([no, $OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU])
AVAILABLE=false
fi
@@ -264,11 +264,13 @@ AC_DEFUN_ONCE([JVM_FEATURES_CHECK_CDS],
[
JVM_FEATURES_CHECK_AVAILABILITY(cds, [
AC_MSG_CHECKING([if platform is supported by CDS])
if test "x$OPENJDK_TARGET_OS" != xaix; then
AC_MSG_RESULT([yes])
else
AC_MSG_RESULT([no, $OPENJDK_TARGET_OS])
if test "x$OPENJDK_TARGET_OS" = xaix || \
( test "x$OPENJDK_TARGET_OS" = "xmacosx" && \
test "x$OPENJDK_TARGET_CPU" = "xaarch64" ) ; then
This conversation was marked as resolved by AntonKozlov

This comment has been minimized.

@magicus

magicus Jan 25, 2021
Member

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

if OS=aix || OS+CPU = mac+aarch64; then
  no
else
 yes
fi
AC_MSG_RESULT([no, $OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU])
AVAILABLE=false
else
AC_MSG_RESULT([yes])
fi
])
])
@@ -1,5 +1,5 @@
#
# Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
@@ -1177,7 +1177,7 @@ define SetupNativeCompilationBody
# This only works if the openjdk_codesign identity is present on the system. Let
# silently fail otherwise.

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Feb 2, 2021
Member

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

This comment has been minimized.

@magicus

magicus Feb 5, 2021
Member

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.

ifneq ($(CODESIGN), )
$(CODESIGN) -s "$(MACOSX_CODESIGN_IDENTITY)" --timestamp --options runtime \
$(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" --timestamp --options runtime \
This conversation was marked as resolved by AntonKozlov

This comment has been minimized.

@magicus

magicus Jan 25, 2021
Member

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

This comment has been minimized.

@VladimirKempik

VladimirKempik Jan 25, 2021

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

--entitlements $$(call GetEntitlementsFile, $$@) $$@
endif
endif
@@ -1,5 +1,5 @@
#
# Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2013, 2021, Oracle and/or its affiliates. All rights reserved.
# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
@@ -88,6 +88,9 @@ ifeq ($(call check-jvm-feature, compiler2), true)
ADLCFLAGS += -DAIX=1
else ifeq ($(call isTargetOs, macosx), true)
ADLCFLAGS += -D_ALLBSD_SOURCE=1 -D_GNU_SOURCE=1
ifeq ($(HOTSPOT_TARGET_CPU_ARCH), aarch64)
ADLCFLAGS += -DR18_RESERVED
endif
else ifeq ($(call isTargetOs, windows), true)
ifeq ($(call isTargetCpuBits, 64), true)
ADLCFLAGS += -D_WIN64=1
@@ -468,7 +468,8 @@ else
maybe-uninitialized class-memaccess
HARFBUZZ_DISABLED_WARNINGS_clang := unused-value incompatible-pointer-types \
tautological-constant-out-of-range-compare int-to-pointer-cast \
undef missing-field-initializers range-loop-analysis
undef missing-field-initializers range-loop-analysis \
deprecated-declarations c++11-narrowing
HARFBUZZ_DISABLED_WARNINGS_microsoft := 4267 4244 4090 4146 4334 4819 4101 4068 4805 4138

LIBFONTMANAGER_CFLAGS += $(HARFBUZZ_CFLAGS)
@@ -31,7 +31,7 @@ ifeq ($(call isTargetOs, linux), true)
SA_CFLAGS := -D_FILE_OFFSET_BITS=64

else ifeq ($(call isTargetOs, macosx), true)
SA_CFLAGS := -Damd64 -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \
This conversation was marked as resolved by AntonKozlov

This comment has been minimized.

@magicus

magicus Jan 25, 2021
Member

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

This comment has been minimized.

@VladimirKempik

VladimirKempik Jan 25, 2021

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

This comment has been minimized.

@lewurm

lewurm Jan 25, 2021
Member

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

Do you think there is any problem with this approach?

This comment has been minimized.

@magicus

magicus Jan 27, 2021
Member

@lewurm No, that's okay. I just wanted to know that this had not been lost.

SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \
-mstack-alignment=16 -fPIC
LIBSA_EXTRA_SRC := $(SUPPORT_OUTPUTDIR)/gensrc/jdk.hotspot.agent
else ifeq ($(call isTargetOs, windows), true)
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1999, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2015, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
@@ -58,7 +58,7 @@ const bool CCallingConventionRequiresIntsAsLongs = false;

#define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS false

#if defined(_WIN64)
#if defined(__APPLE__) || defined(_WIN64)
#define R18_RESERVED

This comment has been minimized.

@theRealAph

theRealAph Mar 1, 2021
Contributor

#define R18_RESERVED true

This comment has been minimized.

@AntonKozlov

AntonKozlov Mar 9, 2021
Author Member

We always check for R18_RESERVED with #if(n)def, is there any reason to define the value for the macro?

This comment has been minimized.

@theRealAph

theRealAph Mar 12, 2021
Contributor

Robustness, clarity, maintainability, convention. Why not?

This comment has been minimized.

@AntonKozlov

AntonKozlov Mar 15, 2021
Author Member

I've tried to implement the suggestion, but it pulled more unnecessary changes. It makes the intended way to check the condition less clear (#ifdef and not #if). The rest of the defines in this file follows the pattern: a define without a value to be checked with #ifdef and define with a value to be checked with #if. To be consistent, I would need to add #define R18_RESERVED false to the #else clause and change every #ifdef R18_RESERVED/#ifndef R18_RESERVED to #if R18_RESERVED/#if !R18_RESERVED. I think we'll win in clarity in the long term if I will not implement the suggestion without related changes. (And related changes would introduce additional noise, which we are fighting with).

#define R18_RESERVED_ONLY(code) code
#define NOT_R18_RESERVED(code)
@@ -1,6 +1,7 @@
/*
* Copyright (c) 2003, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2003, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2020, Red Hat Inc. All rights reserved.
* Copyright (c) 2021, Azul Systems, Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -57,9 +58,14 @@ FloatRegister InterpreterRuntime::SignatureHandlerGenerator::next_fpr() {
return fnoreg;
}

int InterpreterRuntime::SignatureHandlerGenerator::next_stack_offset() {
// On macos/aarch64 native stack is packed, int/float are using only 4 bytes
// on stack. Natural alignment for types are still in place,
// for example double/long should be 8 bytes aligned.

int InterpreterRuntime::SignatureHandlerGenerator::next_stack_offset(unsigned elem_size) {
MACOS_ONLY(_stack_offset = align_up(_stack_offset, elem_size));
int ret = _stack_offset;
_stack_offset += wordSize;
_stack_offset += NOT_MACOS(wordSize) MACOS_ONLY(elem_size);
return ret;
}

@@ -71,6 +77,30 @@ InterpreterRuntime::SignatureHandlerGenerator::SignatureHandlerGenerator(
_stack_offset = 0;
}

void InterpreterRuntime::SignatureHandlerGenerator::pass_byte() {
const Address src(from(), Interpreter::local_offset_in_bytes(offset()));

Register reg = next_gpr();
if (reg != noreg) {
__ ldr(reg, src);
} else {
__ ldrb(r0, src);
__ strb(r0, Address(to(), next_stack_offset(sizeof(jbyte))));
}
}

void InterpreterRuntime::SignatureHandlerGenerator::pass_short() {
const Address src(from(), Interpreter::local_offset_in_bytes(offset()));

Register reg = next_gpr();
if (reg != noreg) {
__ ldr(reg, src);
} else {
__ ldrh(r0, src);
__ strh(r0, Address(to(), next_stack_offset(sizeof(jshort))));
}
}

void InterpreterRuntime::SignatureHandlerGenerator::pass_int() {
const Address src(from(), Interpreter::local_offset_in_bytes(offset()));

@@ -79,7 +109,7 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_int() {
__ ldr(reg, src);
} else {
__ ldrw(r0, src);
__ strw(r0, Address(to(), next_stack_offset()));
__ strw(r0, Address(to(), next_stack_offset(sizeof(jint))));
}
}

@@ -91,7 +121,7 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_long() {
__ ldr(reg, src);
} else {
__ ldr(r0, src);
__ str(r0, Address(to(), next_stack_offset()));
__ str(r0, Address(to(), next_stack_offset(sizeof(jlong))));
}
}

@@ -103,7 +133,7 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_float() {
__ ldrs(reg, src);
} else {
__ ldrw(r0, src);
__ strw(r0, Address(to(), next_stack_offset()));
__ strw(r0, Address(to(), next_stack_offset(sizeof(jfloat))));
}
}

@@ -115,7 +145,7 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_double() {
__ ldrd(reg, src);
} else {
__ ldr(r0, src);
__ str(r0, Address(to(), next_stack_offset()));
__ str(r0, Address(to(), next_stack_offset(sizeof(jdouble))));
}
}

@@ -139,7 +169,8 @@ void InterpreterRuntime::SignatureHandlerGenerator::pass_object() {
__ cbnz(temp(), L);
__ mov(r0, zr);
__ bind(L);
__ str(r0, Address(to(), next_stack_offset()));
static_assert(sizeof(jobject) == wordSize, "");
__ str(r0, Address(to(), next_stack_offset(sizeof(jobject))));
}
}

@@ -164,7 +195,7 @@ class SlowSignatureHandler
: public NativeSignatureIterator {
This conversation was marked as resolved by AntonKozlov

This comment has been minimized.

@theRealAph

theRealAph Jan 23, 2021
Contributor

SlowSignatureHandler is turning into a maintenance nightmare. This isn't the fault of this commit so much as some nasty cut-and-paste programming in the code you're editing. It might well be better to rewrite this whole thing to use a table-driven approach, with a table that contains the increment and the size of each native type: then we'd have only a single routine which could pass data of any type, and each OS needs only supply a table of constants.

This comment has been minimized.

@AntonKozlov

AntonKozlov Jan 24, 2021
Author Member

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

This comment has been minimized.

@theRealAph

theRealAph Jan 25, 2021
Contributor

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

This comment has been minimized.

@AntonKozlov

AntonKozlov Feb 15, 2021
Author Member

The latest merge with JDK-8261071 should resolve the issue. Please take a look.

This comment has been minimized.

@theRealAph

theRealAph Feb 17, 2021
Contributor

Looks much better, thanks.

private:
address _from;
intptr_t* _to;
char* _to;
intptr_t* _int_args;
intptr_t* _fp_args;
intptr_t* _fp_identifiers;
@@ -199,36 +230,53 @@ class SlowSignatureHandler
return -1;
}

void pass_stack(intptr_t value) {
*_to++ = value;
template<typename T>
void pass_stack(T value) {
MACOS_ONLY(_to = align_up(_to, sizeof(value)));
*(T *)_to = value;
_to += NOT_MACOS(wordSize) MACOS_ONLY(sizeof(value));
}

virtual void pass_byte() {
jbyte value = *(jbyte*)single_slot_addr();
if (pass_gpr(value) < 0) {
pass_stack<>(value);
}
}

virtual void pass_short() {
jshort value = *(jshort*)single_slot_addr();
if (pass_gpr(value) < 0) {
pass_stack<>(value);
}
}

virtual void pass_int() {
jint value = *(jint*)single_slot_addr();
if (pass_gpr(value) < 0) {
pass_stack(value);
pass_stack<>(value);
}
}

virtual void pass_long() {
intptr_t value = *double_slot_addr();
if (pass_gpr(value) < 0) {
pass_stack(value);
pass_stack<>(value);
}
}

virtual void pass_object() {
intptr_t* addr = single_slot_addr();
intptr_t value = *addr == 0 ? NULL : (intptr_t)addr;
if (pass_gpr(value) < 0) {
pass_stack(value);
pass_stack<>(value);
}
}

virtual void pass_float() {
jint value = *(jint*)single_slot_addr();
if (pass_fpr(value) < 0) {
pass_stack(value);
pass_stack<>(value);
}
}

@@ -238,7 +286,7 @@ class SlowSignatureHandler
if (0 <= arg) {
*_fp_identifiers |= (1ull << arg); // mark as double
} else {
pass_stack(value);
pass_stack<>(value);
}
}

@@ -247,7 +295,7 @@ class SlowSignatureHandler
: NativeSignatureIterator(method)
{
_from = from;
_to = to;
_to = (char *)to;

_int_args = to - (method->is_static() ? 16 : 17);
_fp_args = to - 8;
@@ -1,6 +1,7 @@
/*
* Copyright (c) 1998, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, Red Hat Inc. All rights reserved.
* Copyright (c) 2021, Azul Systems, Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -38,6 +39,8 @@ class SignatureHandlerGenerator: public NativeSignatureIterator {
unsigned int _num_reg_int_args;
int _stack_offset;

void pass_byte();
void pass_short();
void pass_int();
void pass_long();
void pass_float();
@@ -46,7 +49,7 @@ class SignatureHandlerGenerator: public NativeSignatureIterator {

Register next_gpr();
FloatRegister next_fpr();
int next_stack_offset();
int next_stack_offset(unsigned elem_size);

public:
// Creation
@@ -28,6 +28,7 @@

#include "asm/assembler.inline.hpp"
#include "oops/compressedOops.hpp"
#include "runtime/vm_version.hpp"

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Feb 2, 2021
Member

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

This comment has been minimized.

@AntonKozlov

AntonKozlov Feb 17, 2021
Author Member

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

#include "utilities/powerOfTwo.hpp"

// MacroAssembler extends Assembler by frequently used macros.
ProTip! Use n and p to navigate between commits in a pull request.