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.

Sign up for GitHub

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

Jump to bottom

[23.1] Backports resolving OpenJDK 21.0.4 updates compatibility issues #738

Merged
merged 5 commits into from
May 28, 2024
Merged

[23.1] Backports resolving OpenJDK 21.0.4 updates compatibility issues #738

merged 5 commits into from
May 28, 2024

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented May 22, 2024 •
edited
Loading

Resolves issues introduced by the JDK 21 backports of:

It does so by backporting the following:

and applying a fix on top:

  • svm: fix for JDK 21.0.4 after the downport of "8315373: Change VirtualThread to unmount after freezing, re-mount before thawing" and dependnet changes

@zakkak zakkak requested a review from jerboaa May 22, 2024 11:06
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 22, 2024
@zakkak zakkak added backport affects/23.1 affects/JDK21 and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. labels May 22, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 22, 2024
@jerboaa jerboaa changed the title [23.1] Backports resolving JDK 21 updates compatibility issues [23.1] Backports resolving OpenJDK 21.0.4 updates compatibility issues May 22, 2024
@jerboaa
Copy link
Collaborator

jerboaa commented May 22, 2024

@zakkak This supersedes #736 right?

@zakkak
Copy link
Collaborator Author

zakkak commented May 22, 2024

@zakkak This supersedes #736 right?

Right.

@zakkak zakkak mentioned this pull request May 22, 2024
Closed
@simonis
Copy link

simonis commented May 23, 2024

Sorry for being late on this, I'm now back to work on this topic. Thanks for updating the changes required for 21.0.4 compatibility. I think we still want to get these changes into the upstream https://github.com/graalvm/graalvm-for-jdk21-community-backports as well, so I'll update my old #736 with the additional changes from this PR.

@zakkak, @jerboaa what are your concrete plans on this PR? I think ideally they should be done in the https://github.com/graalvm/graalvm-for-jdk21-community-backports repository and merged from there. But I'm not sure if any of us currently has commit rights for https://github.com/graalvm/graalvm-for-jdk21-community-backports. If that's not the case and if we don't get them until the end of May (May, 28th will be the last merge from jdk21u-dev to jdk21u) I think it's fine to proceed with these changes in the Mandrel repository first.

What do you think?

@jerboaa
Copy link
Collaborator

jerboaa commented May 23, 2024

@zakkak, @jerboaa what are your concrete plans on this PR?

@simonis JDK 21.0.4 ea builds are already running in our CI and we need to get this in ASAP in order to catch other issues with newer EA builds. While it's ideal to get this into the community repos, the issue of maintenance isn't yet resolved and there is a chance that it'll be taking longer than we'd want. Nevertheless we'll sync it there once the new maintainers are in place. For 21.0.5 onwards we'll directly go to the community repo and merge downstream. If the community maintenance question is resolved earlier even better! Does that sound OK?

@jerboaa
Copy link
Collaborator

jerboaa commented May 23, 2024

Getting the maintenance repo issue resolved by Tuesday next week seems very optimistic.

@zakkak
Copy link
Collaborator Author

zakkak commented May 27, 2024

Getting the maintenance repo issue resolved by Tuesday next week seems very optimistic.

I agree. @jerboaa please review when possible. Even if we don't end up merging the exact changes upstream we can fix this retroactively (as done in the past).

jerboaa
jerboaa reviewed May 27, 2024
View reviewed changes
Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

For better per commit tracking I'd suggest to divide 79eb001 into the corresponding changes that got cherry-picked. This allows for an easier 1-to-1 mapping of the backports.

In particular these cherry-picks should get amended:

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
Comment on lines 68 to 71
@TargetElement(onlyWith = JDK21OrEarlier.class) //
static int RUNNABLE_SUSPENDED;
@Alias //
@TargetElement(onlyWith = JDK21OrEarlier.class) //
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is accounting for a backport of JDK-8312498, new in JDK 21.0.4, but not in 21.0.3, I think the correct conditional would be on JDK21_0_3OrEarlier instead of whole-sale JDK 21?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These aliases are removed by be359a8

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
Comment on lines 69 to 71
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PARKING;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PARKED;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PINNED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is related to JDK-8312498 - see the original JDK commit - and the Graal head fix got added since it's first integration was JDK 22 we should change this conditional in the backport to JDK21_0_4OrLater over JDK22OrLater, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
@@ -198,6 +197,12 @@ Thread.State threadState() {
}
} else if (state == TERMINATED) {
return Thread.State.TERMINATED;
} else if (JavaVersionUtil.JAVA_SPEC >= 22) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This conditional needs to be version >= 21.0.4 here, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
@@ -58,17 +59,20 @@ public final class Target_java_lang_VirtualThread {
// Checkstyle: stop
@Alias static int NEW;
@Alias static int STARTED;
@Alias static int RUNNABLE;
@Alias //
@TargetElement(onlyWith = JDK21OrEarlier.class) static int RUNNABLE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is pertaining to JDK-8321270 (JDK head commit for easier viewing since the JDK 21 backport squashes many bugs into one). So RUNNABLE should be conditional on JDK 21.0.3 or earlier for this backport.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
@Alias static int RUNNING;
@Alias static int PARKING;
@Alias static int PARKED;
@Alias static int PINNED;
@Alias static int YIELDING;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int YIELDED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is pertaining to JDK-8321270 (JDK head commit for easier viewing since the JDK 21 backport squashes many bugs into one). So YIELDED should be conditional on JDK 21.0.4 or later for this backport.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
@Alias static int TERMINATED;
@Alias static int SUSPENDED;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PARKING;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PARKED;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int TIMED_PINNED;
@TargetElement(onlyWith = JDK22OrLater.class) @Alias static int UNPARKED;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is pertaining to JDK-8321270 (JDK head commit for easier viewing since the JDK 21 backport squashes many bugs into one). So UNPARKED should be conditional on JDK 21.0.4 or later for this backport.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
@@ -169,7 +173,9 @@ Thread.State threadState() {
} else {
return Thread.State.RUNNABLE;
}
} else if (state == RUNNABLE) {
} else if (JavaVersionUtil.JAVA_SPEC < 22 && state == RUNNABLE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

< 21.0.4 instead of 22

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 79eb001

...m/src/com.oracle.svm.core/src/com/oracle/svm/core/thread/Target_java_lang_VirtualThread.java Outdated
} else if (state == RUNNABLE) {
} else if (JavaVersionUtil.JAVA_SPEC < 22 && state == RUNNABLE) {
return Thread.State.RUNNABLE;
} else if (JavaVersionUtil.JAVA_SPEC >= 22 && (state == UNPARKED || state == YIELDED)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

>= 21.0.4 instead of >= 22

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fixed in 79eb001

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Hence the amend comment. First cherry-picking unchanged seems OK, but then the fix-up for the release should be amended to the cherry pick.

Doing that, plus adding a good commit message helps doing source code archaeology later.

@jerboaa
Copy link
Collaborator

jerboaa commented May 27, 2024 •
edited
Loading

@zakkak I've reviewed this on a per-commit basis for context. It would also be good if the commit messages referred to the relevant JDK bug (where it doesn't).

@zakkak
Copy link
Collaborator Author

zakkak commented May 27, 2024

For better per commit tracking I'd suggest to divide 79eb001 into the corresponding changes that got cherry-picked. This allows for an easier 1-to-1 mapping of the backports.

In particular these cherry-picks should get amended:

* [1e48201](https://github.com/graalvm/mandrel/commit/1e482015f98677adaf7b4251edb67d591d587a02)

* [be359a8](https://github.com/graalvm/mandrel/commit/be359a85ee267ad6db7f37834bd08724d152ab42)

* [95d0057](https://github.com/graalvm/mandrel/commit/95d005723430fbcc465cf7392e02c170bf77e2f5)

Good, I will try to do that.

@zapster @zakkak
svm: adopt "JDK-8312498 Thread::getState and JVM TI GetThreadState sh…
d4bbda6 
…ould return TIMED_WAITING virtual thread is timed parked" [GR-48899]

(cherry picked from commit 421ff99)

Adapted conditionals tot est for <= 21.0.3 as the corresponding
upstream patches have been backported to 21.0.4
@zakkak zakkak force-pushed the 2024-05-20-23.1-backports branch from 8c7b083 to 481ef0c Compare May 28, 2024 08:09
@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

Good, I will try to do that.

Done, I also amended the commit messages to indicate that I adopted the conditions on top of the corresponding cherry picks. Let's see what the CI thinks as well.

jerboaa
jerboaa approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

3601c1a should mention https://bugs.openjdk.org/browse/JDK-8312498 in the commit message as that is the change which required the graal changes. Otherwise, looks good.

peter-hofer and others added 4 commits May 28, 2024 13:28
@peter-hofer @zakkak
[GR-48899] Support TIMED_WAITING for virtual threads.
d573c70 
Changes needed to adopt to https://bugs.openjdk.org/browse/JDK-8312498
which got backported to 21 with
https://bugs.openjdk.org/browse/JDK-8326966

(cherry picked from commit 4fad6c2)

Adapted conditionals to test for <= 21.0.3 or >= 21.0.4 as the
corresponding upstream patches have been backported to 21.0.4
@zapster @zakkak
svm: adopt "JDK-8321270: Virtual Thread.yield consumes parking permit…
c449e08 
…" [GR-50851]

openjdk/jdk@29d7a22
(cherry picked from commit 595ac63)

Adapted conditionals to test for <= 21.0.3 or >= 21.0.4 as the
corresponding upstream patches have been backported to 21.0.4
@zapster @zakkak
svm: adapt "Move ReferencedKeyMap to jdk.internal so it may be shared…
9bbd2f7 
…" [JDK-8310913]

(cherry picked from commit 0f2c3c1)
@zakkak
Adopt "JDK-8324646: Avoid Class.forName in SecureRandom constructor"
46bd410 
Fixes graalvm#716

(cherry picked from commit 39988d3)
@zakkak zakkak force-pushed the 2024-05-20-23.1-backports branch from 481ef0c to 46bd410 Compare May 28, 2024 11:29
@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

3601c1a should mention https://bugs.openjdk.org/browse/JDK-8312498 in the commit message as that is the change which required the graal changes. Otherwise, looks good.

Done. I will merge once the CI completes.

@jerboaa
Copy link
Collaborator

jerboaa commented May 28, 2024

Done. I will merge once the CI completes.

Thanks, sure!

@zakkak zakkak merged commit 0ca9d87 into graalvm:mandrel/23.1 May 28, 2024
67 checks passed
@zakkak zakkak deleted the 2024-05-20-23.1-backports branch May 28, 2024 12:59
@jerboaa jerboaa mentioned this pull request May 28, 2024
Closed
@jerboaa jerboaa added this to the 23.1.4.0-Final milestone May 28, 2024
This was referenced Jun 3, 2024
Closed
Closed
@simonis simonis mentioned this pull request Jun 4, 2024
Open
simonis
simonis reviewed Jun 4, 2024
View reviewed changes
Copy link

@simonis simonis left a comment

Choose a reason for hiding this comment

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

Sorry for being late to this PR. I've played around with it and realized that it has an issue (i.e. missing null-check in SecurityServicesFeature.java - see inline comments) which I think should be fixed.

I've created a PR for the graalvm-for-jdk21-community-backports repository which fixes the issue by downporting and adapting svm: adopt "JDK-8324646: Avoid Class.forName in SecureRandom constructor.

Please let me know if you want me to open an additional PR for the Mandrel repository to fix the issue and if your fine with my solution?

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SecurityServicesFeature.java
if (constrParamClassName != null) {
return loader.findClass(constrParamClassName).get();
if (consParamClassFieldFinal.getName().equals("constructorParameterClassName")) {
return loader.findClass((String) consParamClassFieldFinal.get(engineDescription)).get();
Copy link

Choose a reason for hiding this comment

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

We are missing a null-check here. I.e. consParamClassFieldFinal.get(engineDescription) can be null.

I wonder why we've done this custom fix at all instead of just downporting and adapting svm: adopt "JDK-8324646: Avoid Class.forName in SecureRandom constructor.

I've created a corresponding PR for graalvm-for-jdk21-community-backports. Maybe we can take that change instead of this one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@simonis OK. Thanks. Please create a PR fixing this. We've merged this change already so would prefer a follow-up fix.

I wonder why we've done this custom fix at all instead of just downporting and adapting svm: adopt "JDK-8324646: Avoid Class.forName in SecureRandom constructor.

It would be a custom change anyway as the version conditionals would need to change. We usually go with a version agnostic approach (e.g. testing if the constructor.ParameterClassName field is there. If the original fix would have been done that way it would have been a no-change backport. Those if <VERSION_1> do_thing1() else do_thing2() are a bit problematic as it needs to change with every JDK update that a JDK fix might get backported.

Copy link

Choose a reason for hiding this comment

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

I agree that a version agnostic approach is preferable. Has this been discussed with the GraalVM upstream project already?

I'll create a PR to fix this in Mandrel now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that a version agnostic approach is preferable. Has this been discussed with the GraalVM upstream project already?

It was originally an ask from them when contributing patches. Not sure why they've back-tracked on that.

I'll create a PR to fix this in Mandrel now.

Thanks!

@simonis simonis mentioned this pull request Jun 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerboaa jerboaa jerboaa approved these changes

@simonis simonis simonis left review comments

Assignees
No one assigned
Labels
affects/JDK21 affects/23.1 backport OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
23.1.4.0-Final
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zakkak @jerboaa @simonis @zapster @peter-hofer