-
Notifications
You must be signed in to change notification settings - Fork 144
8373202: [lworld] ObjectReference.equals should follow == semantics for value objects #1834
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.
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
base: lworld
Are you sure you want to change the base?
Conversation
|
👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into |
|
@alexmenkov 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: 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 269 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
Outdated
Show resolved
Hide resolved
|
|
Webrevs
|
src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/ReferenceTypeImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/ReferenceTypeImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/VirtualMachineImpl.java
Outdated
Show resolved
Hide resolved
src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public boolean canUseIsSameObject() { | ||
| public boolean supportsValueClasses() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if not running in preview mode? Value classes are not supported in that case, but as implemented this API will say they are supported, leading to isValueClass() looking at modifiers in a way that is not compatible when value classes are not supported.
JDI didn't really run into this type of problem with virtual thread support. It did modify a couple of new public APIs with:
@PreviewFeature(feature = PreviewFeature.Feature.VIRTUAL_THREADS)
However, I don't see how that helps us here. It seems we need to query the debug agent to find out if value classes are supported. There's no good way to do that. Is there any way for JDI to somewhat detect that value class support is supported (preview mode is enabled).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if not running in preview mode? Value classes are not supported in that case, but as implemented this API will say they are supported, leading to isValueClass() looking at modifiers in a way that is not compatible when value classes are not supported.
It should work correctly. The IDENTITY modifier bit is not used for other purposes in JDK 27 (Alex, please, fix me if it is wrong). So, the check for version >= 27 is needed. If preview is not enabled in target VM, so it does not support value classes then the IDENTITY modifier bit will be always set which results in isValueClass() to always return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in JDK 27, with or without enable preview, the IDENTITY modifier bit is accurate and can be looked at, but in previous JDK versions it might be used for something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in JDK 27, with or without enable preview, the IDENTITY modifier bit is accurate and can be looked at, but in previous JDK versions it might be used for something else?
Correct
I'd suggest to run mach5 full tier-5 and tier-6 as well. |
in progress |
sspitsyn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updates! It looks good to me.
Updated implementation of ObjectReference.equals and ObjectReference.hashCode to comply the spec for value objects.
Added the test for value object ctor debugging, the test verifies the behaviour is expected.
There is an issue with instance filter, it till be fixed separately (it's not yet clear how it would be better to fix it)
testing: tier1..4, hs-tier5-svc
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1834/head:pull/1834$ git checkout pull/1834Update a local copy of the PR:
$ git checkout pull/1834$ git pull https://git.openjdk.org/valhalla.git pull/1834/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1834View PR using the GUI difftool:
$ git pr show -t 1834Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1834.diff
Using Webrev
Link to Webrev Comment