-
Notifications
You must be signed in to change notification settings - Fork 137
Ctor debug #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?
Ctor debug #1834
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,8 +147,16 @@ public boolean vmNotSuspended(VMAction action) { | |
|
|
||
| public boolean equals(Object obj) { | ||
| if (obj instanceof ObjectReferenceImpl other) { | ||
| return (ref() == other.ref()) && | ||
| super.equals(obj); | ||
| if (ref() == other.ref() && super.equals(obj)) { | ||
| return true; | ||
| } | ||
| // We can get equal value objects with different IDs. | ||
| // TODO: do it only for value objects. | ||
| try { | ||
| return JDWP.ObjectReference.IsSameObject.process(vm, this, other).isSameObject; | ||
| } catch (JDWPException exc) { | ||
| throw exc.toJDIException(); | ||
| } | ||
|
Comment on lines
+155
to
+159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the downside here is that when we don't have equality w.r.t. ObjectIDs, we do the heavyweight call over JDWP. A while back I did a scan of users of equals() in the JDI implementation and jdb, and it seemed it was always done on known subtypes (ClassLoaderReference, ThreadReference, ClassReference, etc). So I doubt in practice you would ever see two value types being compared, unless the debugger was doing it for some reason. So if you did limit this JDWP call to value types, probably it would be rare to non-existent that it would ever be called.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed. Do I understand correctly that I cannot modify existing command (jdwp protocol shoul be backward compatible?) to get classes and need a new one to check if the class is value class?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what your backward compatible question is. I think we need an API to determine if a ClassType is a value type or not. This could be expensive to maintain. Probably we need to only look it up when needed and can keep it cached with the ClassType. Another option is to only try to avoid the IsSameObject callback when we are not dealing with an ObjectReferenceImpl subclass. We can make a check something like |
||
| } else { | ||
| return false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -357,6 +357,63 @@ referringObjects(PacketInputStream *in, PacketOutputStream *out) | |
| return JNI_TRUE; | ||
| } | ||
|
|
||
| static jboolean | ||
| isSameObjectImpl(PacketInputStream *in, PacketOutputStream *out) | ||
| { | ||
| jlong id1; | ||
| jlong id2; | ||
| jobject ref1; | ||
| jobject ref2; | ||
| JNIEnv *env; | ||
|
|
||
| env = getEnv(); | ||
| id1 = inStream_readObjectID(in); | ||
| id2 = inStream_readObjectID(in); | ||
| if (inStream_error(in)) { | ||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| if (id1 == NULL_OBJECT_ID || id2 == NULL_OBJECT_ID) { | ||
| outStream_setError(out, JDWP_ERROR(INVALID_OBJECT)); | ||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| ref1 = commonRef_idToRef(env, id1); | ||
| ref2 = commonRef_idToRef(env, id2); | ||
| (void)outStream_writeBoolean(out, isSameObject(env, ref1, ref2)); | ||
|
|
||
| commonRef_idToRef_delete(env, ref1); | ||
| commonRef_idToRef_delete(env, ref2); | ||
|
|
||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| static jboolean | ||
| objectHashCodeImpl(PacketInputStream *in, PacketOutputStream *out) | ||
| { | ||
| jlong id; | ||
| jobject ref; | ||
| JNIEnv *env; | ||
|
|
||
| env = getEnv(); | ||
| id = inStream_readObjectID(in); | ||
| if (inStream_error(in)) { | ||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| if (id == NULL_OBJECT_ID) { | ||
| outStream_setError(out, JDWP_ERROR(INVALID_OBJECT)); | ||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| ref = commonRef_idToRef(env, id); | ||
| (void)outStream_writeInt(out, objectHashCode(ref)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting that the debug agent already has an objectHashCode() but there were no users of it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| commonRef_idToRef_delete(env, ref); | ||
|
|
||
| return JNI_TRUE; | ||
| } | ||
|
|
||
| Command ObjectReference_Commands[] = { | ||
| {referenceType, "ReferenceType"}, | ||
| {getValues, "GetValues"}, | ||
|
|
@@ -367,7 +424,9 @@ Command ObjectReference_Commands[] = { | |
| {disableCollection, "DisableCollection"}, | ||
| {enableCollection, "EnableCollection"}, | ||
| {isCollected, "IsCollected"}, | ||
| {referringObjects, "ReferringObjects"} | ||
| {referringObjects, "ReferringObjects"}, | ||
| {isSameObjectImpl, "IsSameObject"}, | ||
| {objectHashCodeImpl, "ObjectHashCode"} | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest to keep current naming convention (just first letter is changed to capital):
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have isSameObject too (and it's used in many places) |
||
|
|
||
| DEBUG_DISPATCH_DEFINE_CMDSET(ObjectReference) | ||
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.
I'm wondering if the super.equals(obj) call is necessary. The way JDI is implemetned, there is a 1-to-1 relationship between the ObjectID and the ObjectReference. If the ObjectIDs match, then the ObjectRefernces instances should be one in the same. So either one of these checks should be sufficient. I don't think both are needed.
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.
I agree that super.equals call is not necessary, just kept it as it was