RFR: 8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java failes with: com.sun.jdi.InvalidTypeException: Can't assign double[][][] to double[][][]

Tue Apr 9 09:36:17 UTC 2019

Hi David,

you're right! The fix for JDK-8212117 should help here as well.
After that fix we may want to revisit ValueContainer.findType 
implementors, currently they all (except for ArrayTypeImpl)
delegates to ReferenceTypeImpl.findType (which ends up in 
classloader.visibleClasses).
Someone should clarify the difference between classLoader.visibleClasses 
and allClasses filtered by the classLoader instance then.
In my opinion fixing JDK-8212117 is risky and requires heavy testing, do 
you think it may be fixed any time soon?

Thanks,
Egor

On 09-Apr-19 08:15, David Holmes wrote:
> Hi Egor,
>
> I want to re-emphasise that once JDK-8212117 is fixed then this bug 
> should also be fixed.
>
> On 5/04/2019 8:30 pm, Egor Ushakov wrote:
>> 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.
>
> Yes but I'm trying to determine why the difference exists - if we 
> already had loader.findType then why go to the bother of writing 
> findComponentType that way instead of just using findType? My concern 
> is that (ignoring the bug with not-initialized classes) the set of 
> classes returned by loader.visibleClasses may be different to that 
> which can be found via vm.classesByName.
>
> I'm not convinced we have adequate test coverage in this area.
>
>> ArrayTypeImpl.type() is unused, so I'm not sure how it was supposed 
>> to be used and where.
>
> Tracking the relationships between classes/interfaces here is quite 
> tricky and it had appeared to me that ArrayTypeImpl.type() could end 
> up getting used. But I've re-scanned the code and that seems not to be 
> the case. For good measure I changed it to always throw an exception 
> and re-ran all the JDI tests (though as I said coverage seems limited) 
> with no problems.
>
> If this is truly unused then we should delete it to avoid future 
> confusion - a separate RFE would be fine for that.
>
> Thanks,
> David
>
>> 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
>>

-- 
Egor Ushakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop