8248238: Implementation: JEP 388: Windows AArch64 Support #301
Conversation
|
👋 Welcome back rnkovacs! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
|
This backport pull request has now been updated with issue from the original commit. |
|
/contributor add Monica Beckwith mbeckwit@openjdk.org |
|
@rnkovacs |
|
@rnkovacs |
|
/issue add 8272181 |
|
@rnkovacs |
|
@rnkovacs |
rnkovacs
force-pushed
the
8248238-win-aarch64
branch
from
931f39e
to
4f530dd
Compare
Webrevs
|
|
Hi @RealCLanger, I attempted to do that but apparently failed - I forgot to change the target branch. Do I need to close #299 and this one and open completely new PRs? |
|
Seems like I could do the change on the GUI. Could you please check if it's correct now? |
Yes, I think this looks good now, thanks. |
rnkovacs
force-pushed
the
8248238-win-aarch64
branch
from
4f530dd
to
f7f50ab
Compare
|
One question, about R18 exclusion part. it's just reading of x18, not writing, however it might indicate something is still missing there. Could you try to replicate this result with win-arm64 build of jdk11 ( make sure to put hsdis in there) |
|
An example, it's always in Stub Code |
|
So it's void MacroAssembler::pusha() { where push is: // Push lots of registers in the bit set supplied. Don't push sp. // Scan bitset to accumulate register pairs @theRealAph could you please suggest, do we need to change the mask in pusha for win-arm64/mac-arm64 to not save r18 by pusha ? |
|
Yes, exartly. It could be:
|
There was a problem hiding this comment.
Yes, exartly.
It could be:
push(RegSet::range(r0, r29) R18_RESERVED_ONLY(-r18_tls), sp);
But it should probably be done here
https://github.com/openjdk/jdk11u-dev/pull/301/files#diff-0f4150a9c607ccd590bf256daa800c0276144682a92bc6bdced5e8bc1bb81f3aR2548
ah, you were probably talking about pusha(), I initially thought it was about call_clobbered_registers()
| void MacroAssembler::push_call_clobbered_registers() { | ||
| int step = 4 * wordSize; | ||
| push(RegSet::range(r0, r18) - RegSet::of(rscratch1, rscratch2), sp); | ||
| push(call_clobbered_registers() - RegSet::of(rscratch1, rscratch2), sp); |
There was a problem hiding this comment.
is it right to substract rscratch1/2 twice, fist in call_clobbered_registers, then here ?
There was a problem hiding this comment.
It's not exactly wrong. The readability of the code seems to have suffered in translation.
There was a problem hiding this comment.
please take a look at
and call site:
call site doesn't expect rscratch2 to be removed from the list
There was a problem hiding this comment.
Looking at the actual change make to development head, this is a misapplied patch. To match head, this should be:
RegSet MacroAssembler::call_clobbered_registers() {
RegSet regs = RegSet::range(r0, r17) - RegSet::of(rscratch1, rscratch2);
#ifndef R18_RESERVED
regs += r18_tls;
#endif
return regs;
}
void MacroAssembler::push_call_clobbered_registers() {
int step = 4 * wordSize;
push(call_clobbered_registers(), sp);
Strictly speaking it should also push rscratch1 and rscratch2 - they are call clobbered - but we don't care because any macro can trash rscratch1 and rscratch2, so there's no point saving and restoring them.
There was a problem hiding this comment.
Thanks for catching this @VladimirKempik.
There was a problem hiding this comment.
@theRealAph you're right about push_call_clobbered_registers(), but I think call_clobbered_registers() is actually fine the way it is. The register named r18_tls on tip has been renamed to r18_reserved in this backport.
There was a problem hiding this comment.
Looks to me like the only real issue here is warn(). stop() doesn't restore the registers, so that's fine. We do want to see all the registers in a register dump. It would be better if pusha() were left as it is, but popa() did not clobber r18.
There was a problem hiding this comment.
I believe this concern is addressed by openjdk/jdk#5828. Are you okay with leaving this patch as-is and backporting that soon after?
There was a problem hiding this comment.
The register named
r18_tlson tip has been renamed tor18_reservedin this backport.
Why has it been renamed r18_reserved ?
There was a problem hiding this comment.
Turns out I was missing some history & context on this - it was r18_reserved originally, but there was a partial rewrite to r18_tls that I finished in the wrong direction. @lewurm's testing the fix, then I'll update.
Nice catch! I'm not sure if this should go as part of this PR. The same problem exists in jdk-tip and therefore should be addressed there first: On jdk-tip there is only one usage left in the template interpreter generator: https://github.com/openjdk/jdk/blob/c05dc268acaf87236f30cf700ea3ac778e3b20e5/src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp#L1400-L1404 Seems like the code path is uncommon, but we should still fix it imho. What do you think? |
|
Since this PR is kind of stuck, can we get R18 patch out of it and push it under separate PR ? this will make macos-aarch64 port not dependent on win-aarch64 anymore |
|
Thank you all for your thoroughness, support, guidance, and perseverance throughout. |
|
Could you /reviewer credit me too? Thanks |
|
/reviewer credit @VladimirKempik |
|
@rnkovacs Reviewer |
Sorry I thought you are. It seems like it picks up people who actually approve the PR. |
|
@rnkovacs Reviewer |
I'm looking at the OpenJDK census, and vkempik seems to be a committer, not a reviewer. It would be quite wrong for us to record a non-reviewer as a reviewer. |
Should I remove @lewurm and @mo-beck from the reviewers then? The tooling itself seems to be confused about GH reviews from people with and without Reviewer status, as they got added automatically. |
|
/reviewer remove burban |
|
@rnkovacs There are no manually specified reviewers associated with this pull request. |
|
@rnkovacs There are no manually specified reviewers associated with this pull request. |
|
I cannot do any meaningful modifications to the reviewer list. Could someone please sponsor the change? |
No, everybody who reviews a PR (marks the PR as reviewed) and has a census entry will be credited as a reviewer by Skara. But only if somebody is a project Reviewer - by census role - this will be an authoritative review, allowing the author to merge a PR. And I also think that's right, anybody who spent time and effort to review a particular change should also be credited, no matter of project role. |
OK, I hope @VladimirKempik and @magicus will be credited correctly, but they should. The preliminary commit message in the commit above is hopefully misleading. So: |
|
Going to push as commit 7bdb8ac.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@RealCLanger @rnkovacs Pushed as commit 7bdb8ac. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hm, my hope was wrong, I'm sorry, @VladimirKempik. I now think you would have had to approve the PR in GitHub once again... |
|
Thanks so much, @RealCLanger! |
|
Thank you to all the contributors and reviewers who have helped with this PR. It's very much appreciated! |
Yes, I get that, but it's a trap for all contributors: if you go to "Reviewers" at the top of the page you just see a list. I've got nothing against credit where it's due, but this workflow looks rather error prone. |
|
I agree with @RealCLanger. The original work was done by 3 contributors, so I think it is appreciable (and should be encouraged) if any of the original contributors spent their time reviewing the backport, and hence they should be listed as reviewers (despite their project status). Similarly, @VladimirKempik and @magicus spent their time and effort helping out with the original work and the backport as well. For some reason, I don't see them both in the reviewer for the backport and I don't see any of their code comments when I click on their
|
|
probably it's github issue, the link that says %Username% left review comments is not working anymore for me and magicus |
Sure, it's a joint contribution, and all authors should be credited.
Fair enough, but that's about acquiring brownie points, whereas I'm talking about processes. The purpose of a review tick is to mark a change as reviewed, and we need Reviewers to do that. However, the list of "Reviewers" at the top right of this page doesn't indicate which ones are Reviewers. Thankfully, Skara does provide the information we need (in the Reviewers section at the top of the discussion) so we have what we need. |
Main commit of the Windows/AArch64 port.
Also fixes JDK-8272181 by adding
src/hotspot/os_cpu/windows_aarch64/pauth_windows_aarch64.inline.hpp, which is needed for a successful Windows-AArch64 build.Depends on #274 and #299.
Progress
Issues
Backtracing broken on PAC enabled systemsReviewers
Contributors
<mbeckwit@openjdk.org><luhenry@openjdk.org><burban@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/301/head:pull/301$ git checkout pull/301Update a local copy of the PR:
$ git checkout pull/301$ git pull https://git.openjdk.java.net/jdk11u-dev pull/301/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 301View PR using the GUI difftool:
$ git pr show -t 301Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/301.diff