Skip to content

Conversation

@mo-beck
Copy link
Contributor

@mo-beck mo-beck commented Sep 16, 2020

This is a continuation of https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html.

Changes since then:

[1] https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8248238: Implementation: JEP 388: Windows AArch64 Support

Reviewers

Contributors

Download

$ git fetch https://git.openjdk.java.net/jdk pull/212/head:pull/212
$ git checkout pull/212

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 16, 2020

👋 Welcome back mbeckwit! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck The following labels will be automatically applied to this pull request: build core-libs hotspot serviceability shenandoah.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@mo-beck
Copy link
Contributor Author

mo-beck commented Sep 16, 2020

/issue add 8248672,8248500,8248500,8248676,8248663,8248681,8248498,8248656,8248659,8248660,8248670,8248787,824794

@mo-beck
Copy link
Contributor Author

mo-beck commented Sep 16, 2020

/contributor add @mo-beck
/contributor add @luhenry
/contributor add @lewurm

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck The issue 824794 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck
Contributor Monica Beckwith <[email protected]> successfully added.

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck
Contributor Ludovic Henry <[email protected]> successfully added.

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck
Contributor Bernhard Urban-Forster <[email protected]> successfully added.

@lewurm
Copy link
Member

lewurm commented Sep 16, 2020

/issue add 8248672,8248500,8248500,8248676,8248663,8248681,8248498,8248656,8248659,8248660,8248670,8248787,8247941

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@lewurm Only the author (@mo-beck) is allowed to issue the /issue command.

@mo-beck
Copy link
Contributor Author

mo-beck commented Sep 16, 2020

/issue add 8248672,8248500,8248500,8248676,8248663,8248681,8248498,8248656,8248659,8248660,8248670,8248787,8247941

@openjdk
Copy link

openjdk bot commented Sep 16, 2020

@mo-beck
Adding additional issue to issue list: 8248672: utilities: Disallow cmp method using C++ feature.

Adding additional issue to issue list: 8248500: AArch64: Remove the r18 dependency on Windows AArch64.

Adding additional issue to issue list: 8248500: AArch64: Remove the r18 dependency on Windows AArch64.

Adding additional issue to issue list: 8248676: AArch64: Add workaround for LITable constructor.

Adding additional issue to issue list: 8248663: AArch64: Avoid existing macros/keywords of MSVC.

Adding additional issue to issue list: 8248681: AArch64: MSVC doesn't support __PRETTY_FUNCTION__.

Adding additional issue to issue list: 8248498: Add build system support for Windows AArch64.

Adding additional issue to issue list: 8248656: Add Windows AArch64 platform support code.

Adding additional issue to issue list: 8248659: AArch64: Extend CPU Feature detection.

Adding additional issue to issue list: 8248660: AArch64: Make _clear_cache and _nop portable.

Adding additional issue to issue list: 8248670: Windows: Exception handling support on AArch64.

Adding additional issue to issue list: 8248787: G1: Add workaround for MSVC bug.

Adding additional issue to issue list: 8247941: Use Vectored Exception Handling on Windows.

@mo-beck
Copy link
Contributor Author

mo-beck commented Sep 17, 2020

/issue remove 8247941

@openjdk
Copy link

openjdk bot commented Sep 17, 2020

@mo-beck
Removing additional issue from issue list: 8247941.

@lewurm lewurm force-pushed the jdk-windows branch 2 times, most recently from bb5aeb3 to 26e4af3 Compare September 18, 2020 10:09
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 18, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 18, 2020

Webrevs

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look good to me. I will take this branch for a spin.

@erikj79
Copy link
Member

erikj79 commented Sep 18, 2020

Our linux-aarch64 build fails with this:
cc: error: unrecognized command line option '-std=c++14'
when compiling build/linux-aarch64/buildjdk/hotspot/variant-server/libjvm/objs/precompiled/precompiled.hpp.gch

I'm trying to configure a windows-aarch64 build, but it fails on fixpath. Is this something you are also experiencing, and if so, how are you addressing it?

@lewurm
Copy link
Member

lewurm commented Sep 18, 2020

Hey @erikj79, thank you so much for giving it a try!

Our linux-aarch64 build fails with this:
cc: error: unrecognized command line option '-std=c++14'
when compiling build/linux-aarch64/buildjdk/hotspot/variant-server/libjvm/objs/precompiled/precompiled.hpp.gch

Hmm, that's interesting. What environment is that exactly? What configure line are you using there? We have tested on such a system:

$ cat /etc/issue
Ubuntu 19.10 \n \l
$ bash configure --with-boot-jdk=/home/beurba/work/jdk-16+13 --with-jtreg
$ make clean CONF=linux-aarch64-server-release
$ make images JOBS=255 LOG=info CONF=linux-aarch64-server-release
$ ./build/linux-aarch64-server-release/images/jdk/bin/java -XshowSettings:properties -version 2>&1 | grep aarch64
    java.home = /home/beurba/work/jdk/build/linux-aarch64-server-release/images/jdk
    os.arch = aarch64
    sun.boot.library.path = /home/beurba/work/jdk/build/linux-aarch64-server-release/images/jdk/lib

I'm trying to configure a windows-aarch64 build, but it fails on fixpath. Is this something you are also experiencing, and if so, how are you addressing it?

Yes. As far as I understand, the problem is that fixpath.exe isn't built properly when doing cross-compiling on Windows targets (as it hasn't been a thing so far). We use a workaround internally https://gist.github.com/lewurm/c099a4b5fcd8a182510cbdeebcb41f77 , but a proper solution is under discussion on build-dev: https://mail.openjdk.java.net/pipermail/build-dev/2020-July/027872.html

Copy link
Member

@erikj79 erikj79 Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a problem. In our linux cross compile build, we rely on the PATH being completely overwritten with the paths from the devkit here. Otherwise the UTIL_REQUIRE_PROGS may find /usr/bin/cc before $BUILD_DEVKIT_TOOLCHAIN_PATH/gcc.

This is the reason my linux-aarch64 (cross compile) build failed. The system installed cc was too old to recognize the -stdc=c++14 argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you need the rest of the PATH on Windows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you need the rest of the PATH on Windows.

Doesn't look like it actually. I've reverted it, thanks for catching it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I tried the updated patch and it works now.

@mlbridge
Copy link

mlbridge bot commented Sep 19, 2020

Mailing list message from Andrew Haley on build-dev:

On 18/09/2020 11:14, Monica Beckwith wrote:

This is a continuation of
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html

Changes since then:
* We've improved the write barrier as suggested by Andrew [1]

It's still wrong, I'm afraid. This is not a full barrier:

+#define FULL_MEM_BARRIER atomic_thread_fence(std::memory_order_acq_rel);

it is only StoreStore|LoadStore|LoadLoad, but you need StoreLoad as
well. It might well be that you get the same DMB ISH instruction, but
unless you use a StoreLoad barrier here it's theoretically possible
for a compiler to reorder accesses so that a processor sees its own
stores before other processors do. (And it's confusing for the reader
too.)

Use:

+#define FULL_MEM_BARRIER atomic_thread_fence(std::memory_order_seq_cst);

See here:

https://en.cppreference.com/w/cpp/atomic/memory_order

memory_order_seq_cst "...plus a single total order exists in which all
threads observe all modifications in the same order (see
Sequentially-consistent ordering below)"

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

@mlbridge
Copy link

mlbridge bot commented Sep 19, 2020

Mailing list message from Andrew Haley on build-dev:

On 18/09/2020 11:14, Monica Beckwith wrote:

This is a continuation of https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html

The diffs in assembler_aarch64.cpp are mostly spurious. Please try this.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
-------------- next part --------------
diff --git a/src/hotspot/cpu/aarch64/aarch64-asmtest.py b/src/hotspot/cpu/aarch64/aarch64-asmtest.py
index f5a5c6b5aee..43bac8e8142 100644
--- a/src/hotspot/cpu/aarch64/aarch64-asmtest.py
+++ b/src/hotspot/cpu/aarch64/aarch64-asmtest.py
@@ -12,8 +12,11 @@ class Operand(object):
class Register(Operand):

def generate(self):
- self.number = random.randint(0, 30)
- return self
+ while True:
+ self.number = random.randint(0, 30)
+ # r18 is used for TLS on Windows ABI.
+ if self.number != 18:
+ return self

def astr(self, prefix):
return prefix + str(self.number)
@@ -36,8 +39,10 @@ class GeneralRegister(Register):
class GeneralRegisterOrZr(Register):

def generate(self):
- self.number = random.randint(0, 31)
- return self
+ while True:
+ self.number = random.randint(0, 31)
+ if self.number != 18:
+ return self

def astr(self, prefix = ""):
if (self.number == 31):
@@ -53,8 +58,10 @@ class GeneralRegisterOrZr(Register):

class GeneralRegisterOrSp(Register):
def generate(self):
- self.number = random.randint(0, 31)
- return self
+ while True:
+ self.number = random.randint(0, 31)
+ if self.number != 18:
+ return self

def astr(self, prefix = ""):
if (self.number == 31):
@@ -1331,7 +1338,7 @@ generate(SpecialCases, [["ccmn", "__ ccmn(zr, zr, 3u, Assembler::LE);",
["st1w", "__ sve_st1w(z0, __ S, p1, Address(r0, 7));", "st1w\t{z0.s}, p1, [x0, #7, MUL VL]"],
["st1b", "__ sve_st1b(z0, __ B, p2, Address(sp, r1));", "st1b\t{z0.b}, p2, [sp, x1]"],
["st1h", "__ sve_st1h(z0, __ H, p3, Address(sp, r8));", "st1h\t{z0.h}, p3, [sp, x8, LSL #1]"],
- ["st1d", "__ sve_st1d(z0, __ D, p4, Address(r0, r18));", "st1d\t{z0.d}, p4, [x0, x18, LSL #3]"],
+ ["st1d", "__ sve_st1d(z0, __ D, p4, Address(r0, r17));", "st1d\t{z0.d}, p4, [x0, x17, LSL #3]"],
["ldr", "__ sve_ldr(z0, Address(sp));", "ldr\tz0, [sp]"],
["ldr", "__ sve_ldr(z31, Address(sp, -256));", "ldr\tz31, [sp, #-256, MUL VL]"],
["str", "__ sve_str(z8, Address(r8, 255));", "str\tz8, [x8, #255, MUL VL]"],

@mlbridge
Copy link

mlbridge bot commented Sep 29, 2020

Mailing list message from Andrew Haley on build-dev:

On 28/09/2020 20:12, Bernhard Urban-Forster wrote:

The idea is that the naming should suggest that `r18` shouldn't be used on that particular platform. Same is true for
macOS, but the ABI docs suggest a different usage, hence we have something like that in our internal branch for macOS:
Suggestion:

\"r17\"\, NOT\_R18\_RESERVED\(\"r18\"\) WIN64\_ONLY\(\"rtls\"\) MACOS\_ONLY\(\"rplatform\"\)\, \"r19\"\,

Are you suggesting it should rather be something like this eventually?
Suggestion:

\"r17\"\, LINUX\_ONLY\(\"r18\"\) WIN64\_ONLY\(\"rtls\"\) MACOS\_ONLY\(\"rplatform\"\)\, \"r19\"\,

This is wrong. We can't use the register in Linux either, except in
Linux-specific code of which there is almost none. It's also a
pointless complication. r18 should be called r18_tls everywhere, as
discussed.

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

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look good.

@lewurm
Copy link
Member

lewurm commented Sep 29, 2020

@theRealAph okay, I've changed the string representation of r18 to "r18_tls" on every platform.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 29, 2020
@plummercj
Copy link
Contributor

@plummercj thank you for your feedback. I've updated the copyright in mentioned files.

Changes look good. Thanks. I'm marking as "reviewed" for serviceability changes and copyrights.

@mo-beck
Copy link
Contributor Author

mo-beck commented Sep 30, 2020

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 30, 2020
@openjdk
Copy link

openjdk bot commented Sep 30, 2020

@mo-beck
Your change (at version 15466ab) is now ready to be sponsored by a Committer.

@mlbridge
Copy link

mlbridge bot commented Oct 2, 2020

Mailing list message from Ludovic Henry on hotspot-dev:

Hi,

As we now have a whole bunch of reviews (thank you all!), we would need a sponsor to get it merged.

Thank you :)

-------------

PR: https://github.com//pull/212

@mlbridge
Copy link

mlbridge bot commented Oct 2, 2020

Mailing list message from David Holmes on build-dev:

Hi,

On 2/10/2020 1:48 am, Ludovic Henry wrote:

Hi,

As we now have a whole bunch of reviews (thank you all!), we would need a sponsor to get it merged.

The JEP is not yet targeted so we have to wait for that formality. But
once that happens I can sponsor for you.

Also note that the PR references the wrong JEP so can you please edit
the description to fix that.

Meanwhile I'll see if I can take this for a spin through our internal
testing.

Cheers,
David
-----

Thank you :)

-------------

PR: https://github.com//pull/212

@mlbridge
Copy link

mlbridge bot commented Oct 2, 2020

Mailing list message from Ludovic Henry on build-dev:

Hi David,

The JEP is not yet targeted so we have to wait for that formality. But once that happens I can sponsor for you.

Perfect, I didn't know about the need for the JEP to be targeted before the merge.

Also note that the PR references the wrong JEP so can you please edit the description to fix that.

I'll work with @monica to update the PR's description to point to https://openjdk.java.net/jeps/391 instead.

Thank you!
Ludovic

@mlbridge
Copy link

mlbridge bot commented Oct 2, 2020

Mailing list message from Daniel D. Daugherty on build-dev:

So I'm confused... this PR is associated with this bug ID:

  Issue

* JDK-8248238 <https://bugs.openjdk.java.net/browse/JDK-8248238>:
Implementation of JEP: Windows AArch64 Support

and JDK-8248238 is associated with this JEP:

JDK-8248496 <https://bugs.openjdk.java.net/browse/JDK-8248496> JEP
388: Windows/AArch64 Port

Am I missing something here?

Dan

On 10/1/20 5:56 PM, Ludovic Henry wrote:

Hi David,

The JEP is not yet targeted so we have to wait for that formality. But once that happens I can sponsor for you.
Perfect, I didn't know about the need for the JEP to be targeted before the merge.

Also note that the PR references the wrong JEP so can you please edit the description to fix that.
I'll work with @monica to update the PR's description to point to https://openjdk.java.net/jeps/391 instead.

Thank you!
Ludovic

@mlbridge
Copy link

mlbridge bot commented Oct 2, 2020

Mailing list message from Ludovic Henry on build-dev:

It?s me who made a mistake. This PR should be associated with JEP 388 as you are rightly pointing out.

From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
Sent: Thursday, October 1, 2020 3:05 PM
To: Ludovic Henry <luhenry at microsoft.com>; David Holmes <david.holmes at oracle.com>; David Holmes <dholmes at openjdk.java.net>; Andrew Haley <aph at openjdk.java.net>; Chris Plummer <cjplummer at openjdk.java.net>; Magnus Ihse Bursie <ihse at openjdk.java.net>; build-dev at openjdk.java.net; core-libs-dev at openjdk.java.net; hotspot-dev at openjdk.java.net; serviceability-dev at openjdk.java.net; shenandoah-dev at openjdk.java.net; Monica Beckwith <Monica.Beckwith at microsoft.com>
Subject: Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]

So I'm confused... this PR is associated with this bug ID:

Issue

* JDK-8248238<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8248238&data=02%7C01%7Cluhenry%40microsoft.com%7Ce428223ea04a45c2d40308d866562328%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371867436454353&sdata=FV0tr%2FktSPQDxHkI1JMr7UCgW4ygPi8d4yKsGuPVUg8%3D&reserved=0>: Implementation of JEP: Windows AArch64 Support

and JDK-8248238 is associated with this JEP:

JDK-8248496<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8248496&data=02%7C01%7Cluhenry%40microsoft.com%7Ce428223ea04a45c2d40308d866562328%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371867436454353&sdata=HAjFLX%2BtIaKz%2FFULuav%2BUXn6qTZb%2BkjiS4ijsWw7RQE%3D&reserved=0> JEP 388: Windows/AArch64 Port

Am I missing something here?

Dan

On 10/1/20 5:56 PM, Ludovic Henry wrote:

Hi David,

The JEP is not yet targeted so we have to wait for that formality. But once that happens I can sponsor for you.

Perfect, I didn't know about the need for the JEP to be targeted before the merge.

Also note that the PR references the wrong JEP so can you please edit the description to fix that.

I'll work with @monica to update the PR's description to point to https://openjdk.java.net/jeps/391<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fopenjdk.java.net%2Fjeps%2F391&data=02%7C01%7Cluhenry%40microsoft.com%7Ce428223ea04a45c2d40308d866562328%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637371867436464348&sdata=Pj%2FNQvzEa8mrhvY75OFVBG7k623Mgnr56Xo3On%2BoQGo%3D&reserved=0> instead.

Thank you!

Ludovic

@mo-beck mo-beck changed the title 8248238: Implementation of JEP: Windows AArch64 Support 8248238: Implementation: JEP 388: Windows AArch64 Support Oct 2, 2020
@mo-beck
Copy link
Contributor Author

mo-beck commented Oct 2, 2020

The JEP is not yet targeted so we have to wait for that formality. But
once that happens I can sponsor for you.

Thanks.

Also note that the PR references the wrong JEP so can you please edit
the description to fix that.

Added JEP # (388) here and updated the JBS entry.
After looking at JEPs 386, 377, and 379, I also did the following:

  • listed JDK-8248238 as a sub-task for JDK-8248496
  • added this PR link in a comment for the JEP.

As soon as the JEP is targetted, I will update the "Fix version" for the 'Implementation' (JDK-8248238) and ping you @dholmes-ora .

Meanwhile I'll see if I can take this for a spin through our internal
testing.
Thanks so much.

Regards,
Monica

Cheers,
David

@dholmes-ora
Copy link
Member

dholmes-ora commented Oct 5, 2020

@mo-beck The initial comment still has this incorrect link:

[2] https://openjdk.java.net/jeps/8251280

Please edit the comment and fix the link.

@dholmes-ora
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Oct 5, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 5, 2020
@openjdk
Copy link

openjdk bot commented Oct 5, 2020

@dholmes-ora @mo-beck Since your change was applied there have been 93 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 9604ee8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mo-beck
Copy link
Contributor Author

mo-beck commented Oct 5, 2020

@mo-beck The initial comment still has this incorrect link:

[2] https://openjdk.java.net/jeps/8251280

Please edit the comment and fix the link.

That was a link to the macOS + Arm64 port. But I have removed it as it wasn't needed in the description of this implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

9 participants