Check for all interfaces implemented by java/lang/Class in VP - #7691
Conversation
Code in VPClassType::isJavaLangClassObject() tests whether the VPClassType object refers to an instance of one of the interfaces that are known to be implemented by java/lang/Class. The set of interfaces that were considered was incomplete, as it was missing interfaces that were added in JDK 12: java.lang.constant.Constable java.lang.invoke.TypeDescriptor java.lang.invoke.TypeDescriptor$OfField That resulted in VPClassType::isJavaLangClassObject() returning TR_no for any reference that is known to be an instance of one of those interface classes rather than TR_maybe. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
|
@vijaysun-omr, may I ask you to review this change? |
| (_len == 33 && strncmp(_sig, "Ljava/lang/invoke/TypeDescriptor;", 33) == 0) || | ||
| (_len == 41 && strncmp(_sig, "Ljava/lang/invoke/TypeDescriptor$OfField;", 41) == 0)) | ||
| return TR_maybe; | ||
| return TR_no; // java.lang.Class is final and is the direct subclass of Object. |
There was a problem hiding this comment.
Could we create an assert that guards against this in the future ? e.g. use the front end apis to get the opaque class blocks for all the interfaces implemented by java/lang/Class and compare vs the "known set of interfaces" ?
Maybe this could even be used to create an alternate fix that does not depend on the signature matching of a fixed set, but instead a signature matching based on the set of opaque class blocks for all the interfaces implemented by java/lang/Class as per a front end query that we create.
If this has merit, I would be okay with you rolling it into this PR or alternately, choosing to do it via a separate PR. Please let me know your thoughts so that I can decide whether to run testing or not.
There was a problem hiding this comment.
I had been thinking along the same lines, but I was worried that that approach might make the fix slightly riskier. If you don’t mind, I would like to go ahead with this version of the fix, so we can get it double-delivered for the JDK 24 release. Then I could work on a follow-on PR using the cleaner approach for the main branch.
There was a problem hiding this comment.
That is fine with me. I'll start testing, thanks.
|
Jenkins build all |
|
Tests have passed. Merging since it looks like a safe change to me. |
2 participants
Code in
VPClassType::isJavaLangClassObject()tests whether theVPClassTypeobject refers to an instance of one of the interfaces that are known to be implemented byjava/lang/Class. The set of interfaces that were considered was incomplete, as it was missing interfaces that were added in JDK 12:That resulted in
VPClassType::isJavaLangClassObject()returningTR_nofor any reference that is known to be an instance of one of those interface classes rather thanTR_maybe.Fixes: eclipse-openj9/openj9#21031