RFR(xs): 8196920: VerifierTest_1A.java and VerifierTest_1B.java fail: assert(receiver_klass->is_subtype_of(static_receiver_klass))

[Calvin Cheung]

On 2/13/18, 9:41 PM, Ioi Lam wrote:
>
>
> On 2/13/18 8:15 PM, David Holmes wrote:
>> On 14/02/2018 1:52 PM, Ioi Lam wrote:
>>> There is one set of braces for each method. This is pretty common in 
>>> the HotSpot tests. E.g.
>>>
>>> http://hg.openjdk.java.net/jdk/hs/file/e0d3be81da47/test/hotspot/jtreg/runtime/BootstrapMethod/BSMCalledTwice.java#l128 
>>
>>
>>
>> I suspect most of these were written by the same person and then 
>> copied as examples.
>>
>> It is a very odd style to me.
>>
>
> It's the format output by
>
>     java jdk.internal.org.objectweb.asm.util.ASMifier HelloWorld.class
>
> When a classfile can have hundreds of methods, you want to separate 
> them in some way.
Based on the discussion and explanation, I'll leave the change as is.

thanks,
Calvin

>
> - Ioi
>
>>> So we should leave the braces and indentation as is.
>>
>> Ok. Not worth arguing over. :)
>>
>> David
>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>>
>>> On 2/13/18 7:21 PM, David Holmes wrote:
>>>> On 14/02/2018 12:19 PM, Calvin Cheung wrote:
>>>>> On 2/13/18, 5:48 PM, David Holmes wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> Changes look good to me.
>>>>> Thanks for your review, David.
>>>>>>
>>>>>> Aside: why does this test have unnecessary {} scopes ??
>>>>> Not sure since I didn't write the original test. Perhaps they're 
>>>>> there for better code readability?
>>>>> I can replace them with blank lines like the following:
>>>>>>         cw.visit(V1_8, ACC_ABSTRACT + ACC_INTERFACE, 
>>>>>> "UnverifiableIntf", null, "java/lang/Object", null);
>>>>>>
>>>>>>             fv = cw.visitField(ACC_PUBLIC + ACC_FINAL + 
>>>>>> ACC_STATIC, "x", "LVerifierTest0;", null, null);
>>>>>>             fv.visitEnd();
>>>>>>
>>>>>>             mv = cw.visitMethod(ACC_STATIC, "<clinit>", "()V", 
>>>>>> null, null);
>>>>>>             mv.visitCode();
>>>>>>             mv.visitTypeInsn(NEW, "VerifierTest0");
>>>>>>             mv.visitInsn(DUP);
>>>>>>             mv.visitMethodInsn(INVOKESPECIAL, "VerifierTest0", 
>>>>>> "<init>", "()V", false);
>>>>>>             mv.visitFieldInsn(PUTSTATIC, "UnverifiableIntf", "x", 
>>>>>> "LVerifierTest0;");
>>>>>>             mv.visitInsn(RETURN);
>>>>>>             mv.visitMaxs(2, 0);
>>>>>>             mv.visitEnd();
>>>>>>
>>>>>>         addBadMethod(cw);
>>>>>>         cw.visitEnd();
>>>>> Do you prefer the above?
>>>>
>>>> Sure - with indent fixed too of course. :)
>>>>
>>>> But don't waste too much time on it.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 14/02/2018 9:23 AM, Calvin Cheung wrote:
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8196920
>>>>>>> (sorry, the bug was marked confidential)
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8196920/webrev.00/
>>>>>>> (the above change was suggested by Ioi)
>>>>>>>
>>>>>>> By design, the test created a unverifiable class:
>>>>>>>
>>>>>>> Expected exception:
>>>>>>> java.lang.VerifyError: Call to wrong <init> method
>>>>>>> Exception Details:
>>>>>>>    Location:
>>>>>>>      UnverifiableBase.<clinit>()V @4: invokespecial
>>>>>>>    Reason:
>>>>>>>      Type 'java/lang/Object' (constant pool 4) is not assignable 
>>>>>>> to 'VerifierTest0' (constant pool 14)
>>>>>>>
>>>>>>> Due to some recent changes, the test doesn't work with the 
>>>>>>> following vm options combination in hs-tier4 testing:
>>>>>>> -Xcomp -XX:-TieredCompilation -Xverify:none
>>>>>>> and resulting in assert in sharedRuntime.cpp:
>>>>>>> assert(receiver_klass->is_subtype_of(static_receiver_klass)) 
>>>>>>> failed: actual receiver must be subclass of static receiver klass
>>>>>>>
>>>>>>> A fix is to replace the above with a "bad" method which will 
>>>>>>> never be executed during runtime:
>>>>>>>
>>>>>>> Expected exception:
>>>>>>> java.lang.VerifyError: Operand stack underflow
>>>>>>> Exception Details:
>>>>>>>    Location:
>>>>>>>      UnverifiableBase.bad()V @0: areturn
>>>>>>>    Reason:
>>>>>>>      Attempt to pop empty stack.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>
>