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.

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

Throw IdentityException if operand of monent is a value type object #21218

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hzongaro
Copy link
Member

Previously, specifying an instance of a value type class on a monitorenter bytecode would result in an IllegalMonitorStateException; a java.lang.IdentityException must now be thrown instead.

This commit introduces a non-helper, <isIdentityObject>, that checks that the operand of a TR::monent instruction is an instance of an identity class rather than a value type class. If it is a value type class, it throws an IdentityException. The actual test is expanded to test the flags field of the object's class during the Tree Lowering optimization pass.

The result of <isIdentityObject> is tested on a ZEROCHK operation that is generated before the monent during IL generation.

This change includes only required functional changes that allow JIT-compiled code to detect whether it should throw an IdentityException. A planned improvement to Value Propagation will fold calls to <isIdentityObject> if the argument is recognized to be an instance of a value type class or recognized to be an instance of an identity type class at compile-time.

This change depends on eclipse-omr/omr#7674

Fixes #20984

@hzongaro hzongaro requested a review from dsouzai as a code owner February 27, 2025 18:43
@hzongaro hzongaro added comp:jit project:valhalla Used to track Project Valhalla related work labels Feb 27, 2025
@hzongaro hzongaro marked this pull request as draft February 27, 2025 18:44
@hzongaro
Copy link
Member Author

I have marked this pull request as draft, as it depends on eclipse-omr/omr#7674. However, I believe it is complete and ready for review.

@a7ehuo, @0xdaryl, may I ask you to review this change?
@theresa-m, @hangshao0, may I ask you to review the VM-related portions of this change?

@hzongaro hzongaro requested review from 0xdaryl and a7ehuo and removed request for dsouzai February 27, 2025 18:48
@0xdaryl 0xdaryl self-assigned this Feb 28, 2025
/**
* \brief
* Finds the <isIdentityObject> "non-helper" symbol reference, creating it if
* necessary. The non-helper is used to test whether an object is an instance
Copy link
Contributor

Choose a reason for hiding this comment

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

An extra space in necessary. The

Copy link
Member Author

Choose a reason for hiding this comment

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

That was intentional. I always use two spaces after the period at the end of sentence, or after a colon. It's a style that was common using typewriters that printed using monospace fonts like Courier, and as we usually look at our code in Courier font, I prefer to continue to follow that convention.

Previously, specifying an instance of a value type class on a
monitorenter bytecode would result in an IllegalMonitorStateException;
a java.lang.IdentityException must now be thrown instead.

This commit introduces a non-helper, <isIdentityObject>, that checks
that the operand of a TR::monent instruction is an instance of an
identity class rather than a value type class.  If it is a value type
class, it throws an IdentityException.  The actual test is expanded
to test the flags field of the object's class during the Tree Lowering
optimization pass.

The result of <isIdentityObject> is tested on a ZEROCHK operation that
is generated before the monent during IL generation.

Signed-off-by:  Henry Zongaro <zongaro@ca.ibm.com>
@hzongaro hzongaro force-pushed the monenter-throws-IdentityExcept branch from 408c0ae to cfe2348 Compare February 28, 2025 19:45
Copy link
Contributor

@a7ehuo a7ehuo left a comment

Choose a reason for hiding this comment

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

Thank you very much for the fix! Just some minor comments.

Comment on lines +1562 to +1569
* @verbatim
n88n PassThrough
n99n iand
n98n iloadi <isClassFlags>
n97n aloadi <vft-symbol>
n77n aload x
n96n iconst 0x80000 // Test whether class is an identity class
* @endverbatim
Copy link
Contributor

Choose a reason for hiding this comment

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

iconst node doesn't seem indented correctly to align with the iloadi node. I also wonder if putting the comment at the iand node could make it clearer.

 * @verbatim
   n88n  PassThrough
   n99n    iand                     // Test whether class is an identity class
   n98n      iloadi  <isClassFlags>
   n97n        aloadi  <vft-symbol>
   n77n          aload x
   n96n      iconst 0x80000
 * @endverbatim

Comment on lines +1084 to +1086
#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
SET(TR_identityException, (void *)jitThrowIdentityException, TR_Helper);
#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if TR_identityException should be guarded with J9VM_OPT_VALHALLA_VALUE_TYPES in this file. First, the JIT code generally doesn't guard Valhalla specific change with build flags such as J9VM_OPT_VALHALLA_VALUE_TYPES. Secondly, anywhere else that references TR_identityException is not guarded with this flag either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think at one point, I had the entire definition of old_slow_jitThrowIdentityException enclosed in an

#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)

and so I conditionally compiled these uses of jitThrowIdentityException. But then I ended up with link errors on a couple of platforms when building without valhalla enabled, so I adjusted the definition of old_slow_jitThrowIdentityException to allow it to compile even without value types enabled. But I forgot to remove these ifdefs.

Yes, I think they can safely be removed. Thanks for pointing that out!

Comment on lines +146 to +148
#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
JIT_HELPER(jitThrowIdentityException); // asm calling-convention helper
#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the above regarding to J9VM_OPT_VALHALLA_VALUE_TYPES.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as the previous comment - I think I had left this ifdef in because of the way I defined old_slow_jitThrowIdentityException in an earlier version of the code. I will remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit project:valhalla Used to track Project Valhalla related work
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

ValueTypes: java.lang.IdentityException MonitorEnterTest.java
5 participants