8282573: ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y) by eme64 · Pull Request #7674 · openjdk/jdk

eme64 · 2022-03-03T11:24:04Z

There are two functions that check for equality.
void ck(long x, long y): already throws a RuntimeException if we observe inequality
void ck(double x, double y): called itself, leading to endless recursion and a StackOverflowError.

Instead of the recursion, I now also throw a RuntimeException.

This can currently be triggered with
path-to-jtreg/jtreg -va -s -jdk:jdk-path -javaoptions:"-XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -XX:+StressGCM -XX:+OptoScheduling -XX:StressSeed=293843391" test/hotspot/jtreg/compiler/intrinsics/unsafe/HeapByteBufferTest.java
(see bug JDK-8282555)

This way I could verify that instead of StackOverflowError we now get RuntimeException, as desired.
Ran some basic tests to ensure I didn't break things.

Progress

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

Issue

JDK-8282573: ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)

Reviewers

Paul Sandoz (@PaulSandoz - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7674/head:pull/7674
$ git checkout pull/7674

Update a local copy of the PR:
$ git checkout pull/7674
$ git pull https://git.openjdk.java.net/jdk pull/7674/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7674

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7674.diff

…xception in void ck(double x, double y)

bridgekeeper · 2022-03-03T11:24:40Z

👋 Welcome back eme64! 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-03-03T11:26:19Z

@eme64 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-03-03T11:34:20Z

Webrevs

01: Full - Incremental (17ad7a21)
00: Full (4eca7cf8)

eme64 · 2022-03-03T11:41:17Z

/label add nio

openjdk · 2022-03-03T11:42:13Z

@eme64
The nio label was successfully added.

TobiHartmann · 2022-03-03T14:26:41Z

test/hotspot/jtreg/compiler/intrinsics/unsafe/ByteBufferTest.java

@@ -217,7 +217,9 @@ void ck(long x, long y) {

    void ck(double x, double y) {
        if (x == x && y == y && x != y) {


What are the x == x and y == y checks good for?

I removed it. @PaulSandoz agreed that this method was very suspect. I should try removing it. There is an off chance it could create some NaN issues on odd platforms.

eme64 · 2022-03-03T14:41:20Z

/label remove nio

openjdk · 2022-03-03T14:42:35Z

@eme64
The nio label was successfully removed.

openjdk · 2022-03-03T17:56:47Z

@eme64 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:

8282573: ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)

Reviewed-by: psandoz, 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 13 new commits pushed to the master branch:

d5e8e52: 8282532: Allow explicitly setting build platform alongside --openjdk-target
b383780: 8282343: Create a regression test for JDK-4518432
b629782: 8279886: C1: Turn off SelectivePhiFunctions in presence of irreducible loops
7e1c67d: 8282608: RawNativeLibraryImpl can't be passed to NativeLibraries::findEntry0
8478173: 8282583: Update BCEL md to include the copyright notice
fb6b929: 8277474: jarsigner does not check if algorithm parameters are disabled
1581e3f: 8282402: Create a regression test for JDK-4666101
268fa69: 8282511: Use fixed certificate validation date in SSLExampleCert template
c777bb3: 8282619: G1: Fix indentation in G1CollectedHeap::mark_evac_failure_object
080baff: 8282483: Ensure that Utils.getAllInterfaces returns unique instances
... and 3 more: https://git.openjdk.java.net/jdk/compare/832729b41e690d6f05da71997bbe2a74b2d6dada...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 (@PaulSandoz, @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).

TobiHartmann

Looks good to me.

eme64 · 2022-03-04T12:46:35Z

Thanks @PaulSandoz and @TobiHartmann for the help and the reviews!
/integrate

openjdk · 2022-03-04T12:47:52Z

@eme64
Your change (at version 17ad7a2) is now ready to be sponsored by a Committer.

TobiHartmann · 2022-03-04T12:54:41Z

/sponsor

openjdk · 2022-03-04T12:56:07Z

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

d5e8e52: 8282532: Allow explicitly setting build platform alongside --openjdk-target
b383780: 8282343: Create a regression test for JDK-4518432
b629782: 8279886: C1: Turn off SelectivePhiFunctions in presence of irreducible loops
7e1c67d: 8282608: RawNativeLibraryImpl can't be passed to NativeLibraries::findEntry0
8478173: 8282583: Update BCEL md to include the copyright notice
fb6b929: 8277474: jarsigner does not check if algorithm parameters are disabled
1581e3f: 8282402: Create a regression test for JDK-4666101
268fa69: 8282511: Use fixed certificate validation date in SSLExampleCert template
c777bb3: 8282619: G1: Fix indentation in G1CollectedHeap::mark_evac_failure_object
080baff: 8282483: Ensure that Utils.getAllInterfaces returns unique instances
... and 3 more: https://git.openjdk.java.net/jdk/compare/832729b41e690d6f05da71997bbe2a74b2d6dada...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-04T12:56:27Z

@TobiHartmann @eme64 Pushed as commit a584c90.

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

dcubed-ojdk · 2022-03-04T15:29:57Z

test/hotspot/jtreg/compiler/intrinsics/unsafe/ByteBufferTest.java

-        if (x == x && y == y && x != y) {
-            ck(x, y);
+        if (x != y) {
+            throw new RuntimeException(" x = " + Double.toString(x) + ", y = " + Double.toString(y)


You might want to add some explanation to the exception:
s/" x = "/"Expected x == y: x = "

or something like that.

eme64 added 2 commits March 3, 2022 12:15

8282573: ByteBufferTest.java: replace endless recursion with RuntimeE…

b7ac5a1

…xception in void ck(double x, double y)

missed a line

4eca7cf

openjdk bot added the rfr Pull request is ready for review label Mar 3, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 3, 2022

openjdk bot added the nio nio-dev@openjdk.org label Mar 3, 2022

TobiHartmann reviewed Mar 3, 2022

View reviewed changes

openjdk bot removed the nio nio-dev@openjdk.org label Mar 3, 2022

removed unnecessary conditions

17ad7a2

PaulSandoz approved these changes Mar 3, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 3, 2022

TobiHartmann approved these changes Mar 4, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 4, 2022

openjdk bot added the integrated Pull request has been integrated label Mar 4, 2022

openjdk bot closed this Mar 4, 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 Mar 4, 2022

dcubed-ojdk reviewed Mar 4, 2022

View reviewed changes

eme64 mentioned this pull request Mar 4, 2022

8282661: [BACKOUT] ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y) #7699

Closed

3 tasks

		@@ -217,7 +217,9 @@ void ck(long x, long y) {

		void ck(double x, double y) {
		if (x == x && y == y && x != y) {

Conversation

eme64 commented Mar 3, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 3, 2022

Uh oh!

openjdk bot commented Mar 3, 2022

Uh oh!

mlbridge bot commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

eme64 commented Mar 3, 2022

Uh oh!

openjdk bot commented Mar 3, 2022

Uh oh!

TobiHartmann Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

eme64 Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

eme64 commented Mar 3, 2022

Uh oh!

openjdk bot commented Mar 3, 2022

Uh oh!

openjdk bot commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 commented Mar 4, 2022

Uh oh!

openjdk bot commented Mar 4, 2022

Uh oh!

TobiHartmann commented Mar 4, 2022

Uh oh!

openjdk bot commented Mar 4, 2022

Uh oh!

openjdk bot commented Mar 4, 2022

Uh oh!

dcubed-ojdk Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

eme64 commented Mar 3, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 3, 2022 •

edited

Loading

openjdk bot commented Mar 3, 2022 •

edited

Loading