Hi David, thanks for your comments!
I'm not sure how findComponentType worked in the current state:
Type findComponentType(String signature) throws ClassNotLoadedException {
93 byte tag = (byte)signature.charAt(0);
94 if (PacketStream.isObjectTag(tag)) {
95 // It's a reference type
96 JNITypeParser parser = new JNITypeParser(componentSignature());
97 List<ReferenceType> list = vm.classesByName(parser.typeName());
98 Iterator<ReferenceType> iter = list.iterator();
99 while (iter.hasNext()) {
100 ReferenceType type = iter.next();
101 ClassLoaderReference cl = type.classLoader();
102 if ((cl == null)?
103 (classLoader() == null) :
104 (cl.equals(classLoader()))) {
105 return type;
106 }
107 }
108 // Component class has not yet been loaded
109 throw new ClassNotLoadedException(componentTypeName());
110 } else {
111 // It's a primitive type
112 return vm.primitiveTypeMirror(tag);
113 }
114 }
the method receives signature string but uses it only for object
checking, then inside it does the search by componentSignature() instead...
Most probably the method is always called passing componentSignature()
string as a parameter anyway.
Regarding the fix:
the only thing I'm trying to change here is where the requested type
name is looked at - loader.visibleClasses (this is where findType ends
up) instead of vm.classesByName.
This is how it works in all other implementors of
com.sun.tools.jdi.ValueContainer.findType.
ArrayTypeImpl.type() is unused, so I'm not sure how it was supposed to
be used and where.
I've run all jdi tests and see no regression, probably
ClassNotLoadedException is never thrown in this case.
Removed the test from the ProblemList in the new cr:
http://cr.openjdk.java.net/~eushakov/8221503/webrev.01/
On 05-Apr-19 07:58, David Holmes wrote:
Hi Egor,
This doesn't seem right to me sorry ...
On 2/04/2019 7:42 pm, Egor Ushakov wrote:
Please review the fix
this test started to fail after the fix of
https://bugs.openjdk.java.net/browse/JDK-8146986
previously any call to ClassLoaderReference.visibleClasses as a side
effect cached the results in VirtualMachineImpl.typesBySignature
and made them available to any VirtualMachine.classesByName call.
This is somewhat not consistent for unprepared classes, I reported
that, check
Bug 26148990 : JDI - VirtualMachine.allClasses does not return loaded
but uninitialized class,
That issue is now a duplicate of this core-libs issue:
https://bugs.openjdk.java.net/browse/JDK-8212117
which is in the process of being fixed. Once it is fixed then this
workaround would no longer be needed.
That said I'm not at all clear how the proposed workaround actually
works-around the underlying issue.
but it seems that it allowed the ArrayTypeImpl.componentType to work
even if the type was not prepared,
check how ArrayTypeImpl#findComponentType uses vm.classesByName.
It seems that ArrayTypeImpl#findComponentType should simply call
findType for component signature.
Just how the unused com.sun.tools.jdi.ArrayTypeImpl#type method
works, not sure why it was dropped...
The package-private:
Type type() throws ClassNotLoadedException {
return findType(elementSignature());
}
seems to have always existed but never been used as the implementation
for the public componentType() method (previously elementType()). In
fact the two methods appear to have different requirements with
regards to what they should be returning. AFAICS:
- ArrayTypeImpl.type() returns the actual array type
- ArrayTypeImpl.componentType() returns the actual ultimate component
type
i.e if I understand correctly, given double[][][] then
- ArrayTypeImpl.type() == double[][]
- ArrayTypeImpl.componentType() == double
so implementing them both the same way can't possibly be correct!
bugid https://bugs.openjdk.java.net/browse/JDK-8221503
cr http://cr.openjdk.java.net/~eushakov/8221503/webrev.00/
Thanks!
<http://cr.openjdk.java.net/~eushakov/8221503/webrev.00/>
You'd also need to update the ProblemList so the test gets re-enabled.
diff -r 532e88de77eb test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt
+++ b/test/hotspot/jtreg/ProblemList.txt
@@ -161,8 +161,6 @@
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021/TestDescription.java
8065773 generic-all
vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses023/TestDescription.java
8065773 generic-all
-vmTestbase/nsk/jdb/eval/eval001/eval001.java 8221503 generic-all
-
vmTestbase/metaspace/gc/firstGC_10m/TestDescription.java 8208250
generic-all
vmTestbase/metaspace/gc/firstGC_50m/TestDescription.java 8208250
generic-all
vmTestbase/metaspace/gc/firstGC_99m/TestDescription.java 8208250
generic-all
It also appears to me that the exception message of the
ClassNotLoadedException will be different now. Have you re-run all the
JDI tests?
Thanks,
David
-----
--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
--
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
