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

[Egor Ushakov]

Hi David,

any news on https://bugs.openjdk.java.net/browse/JDK-8212117?
I do not see any activity on this :(

Thanks,
Egor

On 09-Apr-19 15:53, David Holmes wrote:
> On 9/04/2019 7:36 pm, Egor Ushakov wrote:
>> 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).
>
> ArrayTypeImpl inherits findType from ReferenceTypeImpl.
>
> The issue is only the non-public findComponentType method.
>
>> Someone should clarify the difference between 
>> classLoader.visibleClasses and allClasses filtered by the classLoader 
>> instance then.
>
> That would be good, I just don't know if there is anyone who would 
> know (as opposed to trying to figure it out).
>
>> In my opinion fixing JDK-8212117 is risky and requires heavy testing, 
>> do you think it may be fixed any time soon?
>
> Yes it's under way but the person doing it is away this week.
>
> Thanks,
> David
>
>> 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