[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8221503: vmTestbase/nsk/jdb/eval/eval001/eval001.java failes with: com.sun.jdi.InvalidTypeE
From:       David Holmes <david.holmes () oracle ! com>
Date:       2019-04-09 12:53:34
Message-ID: 057e627a-f488-0a30-d9e1-147627237383 () oracle ! com
[Download RAW message or body]

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
>>>
> 
[prev in list] [next in list] [prev in thread] [next in thread]