8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception #11308

vpaprotsk · 2022-11-23T02:59:30Z

From #10582, t0 gets clobbered if rscratch is used. Example, here:

  __ mov(t0, a0);
  __ andq(t0, ExternalAddress(poly1305_mask44()), rscratch); // First limb (R^4[43:0])

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to a2c8907f
Vladimir Ivanov (@iwanowww - Reviewer)
Roberto Castañeda Lozano (@robcasloz - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11308/head:pull/11308
$ git checkout pull/11308

Update a local copy of the PR:
$ git checkout pull/11308
$ git pull https://git.openjdk.org/jdk pull/11308/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11308

View PR using the GUI difftool:
$ git pr show -t 11308

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11308.diff

bridgekeeper · 2022-11-23T03:00:38Z

👋 Welcome back vpaprotsk! 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 · 2022-11-23T03:03:48Z

@vpaprotsk The following label will be automatically applied to this pull request:

hotspot-compiler

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

mlbridge · 2022-11-23T03:08:38Z

Webrevs

sviswa7 · 2022-11-23T18:00:58Z

src/hotspot/cpu/x86/stubGenerator_x86_64_poly.cpp

@@ -559,7 +559,7 @@ void StubGenerator::poly1305_process_blocks_avx512(
  const Register t0 = r13;
  const Register t1 = r14;
  const Register t2 = r15;
-  const Register rscratch = r13;
+  const Register rscratch = r14;


The register map above in the comments also should reflect this change that rscratch is r14 now.

done. (or rather, ended up removing rscratch completely from that function)

iwanowww · 2022-11-23T18:39:22Z

It's a general problem: it's hard to see possible interference when names alias to the same register. I suggest to get rid of rscratch declaration and directly refer to t1 followed by /*rscratch*/ comment:

  __ movq(t0, a0);
  __ andq(t0, ExternalAddress(poly1305_mask44()), t1 /*rscratch*/); // First limb (Acc[43:0])
  __ movq(C0, t0);

iwanowww · 2022-11-23T18:41:18Z

Also, -XX:+ForceUnreachable is designed to stress non-rip addressing mode. Worth adding it to the corresponding test.

vpaprotsk · 2022-11-23T19:38:19Z

Also, -XX:+ForceUnreachable is designed to stress non-rip addressing mode. Worth adding it to the corresponding test.

Thanks, was wondering about that.

It's a general problem: it's hard to see possible interference when names alias to the same register. I suggest to get rid of rscratch declaration and directly refer to t1 followed by /*rscratch*/ comment:

Hmm. I think register aliasing is pretty much a 'necessary evil' in the intrinsic code. Having better names for variables combined with too few registers leads to this issue. (unless of course we could somehow inject intrinsics before register_ allocation. Writing assembler with infinite registers.. there is an idea :) ) .

But in this specific case maybe getting rid of aliasing doesn't harm the readability that much. In some way, I would had preferred to keep rscratch instead of t1 (I already have too many 'temps') but the /*rscratch*/ comment should make things ok. 👍

vpaprotsk · 2022-11-23T20:01:56Z

@iwanowww done

Confirmed, -XX:+ForceUnreachable does indeed catch this on linux; added to half the tests
Only removed rscratch from top-level poly1305_process_blocks_avx512 function. I think calling it rscratch in 'leaf' functions (i.e. poly1305_multiply8_avx512) is ok.

sviswa7 · 2022-11-23T21:29:44Z

PR looks good to me.

openjdk · 2022-11-23T21:31:23Z

@vpaprotsk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception

Reviewed-by: sviswanathan, vlivanov, rcastanedalo, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 28 new commits pushed to the master branch:

df6cf1e: 8296886: Fix various include sort order issues
5e196b4: 8297476: Increase InlineSmallCode default from 1000 to 2500 for RISC-V
070a84c: 8297192: Warning generating API docs for javax.management.MBeanServer: overridden methods do not document exception type
3c4d520: 8296671: [JFR] jdk.ContainerConfiguration event should include host total memory
8b73970: 8296208: AArch64: Enable SHA512 intrinsic by default on supported hardware
f4cf758: 8297491: Loom: Stack chunks allocation code uses TLABs even when TLABs are disabled
5e5b774: 8296768: Use different explicit claim marks for CLDs in Parallel and Serial GC
9c77e41: 8297445: PPC64: Represent Registers as values
2f8a5c2: 8297480: GetPrimitiveArrayCritical in imageioJPEG misses result - NULL check
abeddab: 6528710: sRGB-ColorSpace to sRGB-ColorSpace Conversion
... and 18 more: https://git.openjdk.org/jdk/compare/09f70dad2fe3f0691afacded6c38f61fa8a0d28d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sviswa7, @iwanowww, @robcasloz, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

iwanowww · 2022-11-23T22:26:59Z

test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/Poly1305UnitTestDriver.java

@@ -49,14 +49,14 @@
 * @key randomness
 * @modules java.base/com.sun.crypto.provider
 * @summary Unit test for IntrinsicCandidate in com.sun.crypto.provider.Poly1305.
- * @run main/othervm -Xcomp -XX:-TieredCompilation java.base/com.sun.crypto.provider.Poly1305IntrinsicFuzzTest
+ * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:+ForceUnreachable java.base/com.sun.crypto.provider.Poly1305IntrinsicFuzzTest


Don't you need -XX:+UnlockDiagnosticVMOptions here as well?

Darn. Yes.. one min, will push.. Sorry about the noise. (thought that the fact that it failed on linux meant it wasnt a diagnostic, but it is..)

product(bool, ForceUnreachable, false, DIAGNOSTIC, \ "Make all non code cache addresses to be unreachable by " \ "forcing use of 64bit literal fixups")

iwanowww · 2022-11-23T22:33:05Z

src/hotspot/cpu/x86/stubGenerator_x86_64_poly.cpp

@@ -559,7 +558,6 @@ void StubGenerator::poly1305_process_blocks_avx512(
  const Register t0 = r13;
  const Register t1 = r14;
  const Register t2 = r15;
-  const Register rscratch = r13;


After seeing the whole patch, I noticed that t1 is used only at the very end of the stub.

Alternatively, you could keep rscratch and move t1 declaration close to the use sites.

No (unless I misunderstood), there are a couple more in the middle passed to poly1305_multiply_scalar (I prefer 'the look' of t0, t1, t2)

poly1305_multiply_scalar(a0, a1, a2, r0, r1, c1, true, t0, t1, t2, mulql, mulqh);

Yes, you are right. Overlooked those when browsed code on GitHub.

iwanowww

Looks good.

Submitted the patch for testing.

robcasloz

Thanks for promptly addressing this issue, Volodymyr, looks good. I see that @iwanowww's internal testing succeeded.

TobiHartmann

Looks good to me too.

vpaprotsk · 2022-11-24T15:23:27Z

/integrate

openjdk · 2022-11-24T15:25:38Z

@vpaprotsk
Your change (at version 0effe19) is now ready to be sponsored by a Committer.

kimbarrett · 2022-11-25T07:05:35Z

This is causing some noise in our CI. While I'm not that familiar with the
code being changed, this looks reasonable to me, and has already been approved
by 4 much more competent reviewers. So I'm going to sponsor now to get this in
sooner.

/sponsor

openjdk · 2022-11-25T07:07:50Z

Going to push as commit 74d3bac.
Since your change was applied there have been 35 commits pushed to the master branch:

0ed8b33: 8297385: Remove duplicated null typos in javadoc
1fb9dad: 8296419: [REDO] JDK-8295319: pending_cards_at_gc_start doesn't include cards in thread buffers
2f47f83: 8297569: URLPermission constructor throws IllegalArgumentException: Invalid characters in hostname after JDK-8294378
87d1097: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
390e69a: 8297150: Add a @sealedGraph tag to Reference
8effaa8: 8223783: sun/net/www/http/HttpClient/MultiThreadTest.java sometimes detect threads+1 connections
ab1f9ff: 8051627: Invariants about java.net.URI resolve and relativize are wrong
df6cf1e: 8296886: Fix various include sort order issues
5e196b4: 8297476: Increase InlineSmallCode default from 1000 to 2500 for RISC-V
070a84c: 8297192: Warning generating API docs for javax.management.MBeanServer: overridden methods do not document exception type
... and 25 more: https://git.openjdk.org/jdk/compare/09f70dad2fe3f0691afacded6c38f61fa8a0d28d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-25T07:08:11Z

@kimbarrett @vpaprotsk Pushed as commit 74d3bac.

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

fix t0 rscratch usage overlap

aa8cea4

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2022

vpaprotsk changed the title ~~8297417: x86 Poly1305 intrinsic rscratch and temp overlap~~ 8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception Nov 23, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 23, 2022

sviswa7 reviewed Nov 23, 2022

View reviewed changes

remove rscratch from top regmap and ForceUnreachable test

a2c8907

sviswa7 approved these changes Nov 23, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 23, 2022

iwanowww reviewed Nov 23, 2022

View reviewed changes

add UnlockDiagnosticVMOptions

0effe19

iwanowww approved these changes Nov 23, 2022

View reviewed changes

robcasloz approved these changes Nov 24, 2022

View reviewed changes

TobiHartmann approved these changes Nov 24, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 24, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 25, 2022

openjdk bot closed this Nov 25, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 25, 2022

vpaprotsk deleted the poly-regalloc branch November 25, 2022 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception #11308

8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception #11308

vpaprotsk commented Nov 23, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 •

edited

Loading

sviswa7 Nov 23, 2022

vpaprotsk Nov 23, 2022

iwanowww commented Nov 23, 2022 •

edited

Loading

iwanowww commented Nov 23, 2022

vpaprotsk commented Nov 23, 2022

vpaprotsk commented Nov 23, 2022

sviswa7 commented Nov 23, 2022

openjdk bot commented Nov 23, 2022 •

edited

Loading

iwanowww Nov 23, 2022

vpaprotsk Nov 23, 2022

iwanowww Nov 23, 2022

vpaprotsk Nov 23, 2022

iwanowww Nov 23, 2022

iwanowww left a comment

robcasloz left a comment

TobiHartmann left a comment

vpaprotsk commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

kimbarrett commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception #11308

8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception #11308

Conversation

vpaprotsk commented Nov 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 • edited Loading

Webrevs

sviswa7 Nov 23, 2022

Choose a reason for hiding this comment

vpaprotsk Nov 23, 2022

Choose a reason for hiding this comment

iwanowww commented Nov 23, 2022 • edited Loading

iwanowww commented Nov 23, 2022

vpaprotsk commented Nov 23, 2022

vpaprotsk commented Nov 23, 2022

sviswa7 commented Nov 23, 2022

openjdk bot commented Nov 23, 2022 • edited Loading

iwanowww Nov 23, 2022

Choose a reason for hiding this comment

vpaprotsk Nov 23, 2022

Choose a reason for hiding this comment

iwanowww Nov 23, 2022

Choose a reason for hiding this comment

vpaprotsk Nov 23, 2022

Choose a reason for hiding this comment

iwanowww Nov 23, 2022

Choose a reason for hiding this comment

iwanowww left a comment

Choose a reason for hiding this comment

robcasloz left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

vpaprotsk commented Nov 24, 2022

openjdk bot commented Nov 24, 2022

kimbarrett commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

openjdk bot commented Nov 25, 2022

vpaprotsk commented Nov 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 23, 2022 •

edited

Loading

iwanowww commented Nov 23, 2022 •

edited

Loading

openjdk bot commented Nov 23, 2022 •

edited

Loading