RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Vladimir Ivanov]

> http://cr.openjdk.java.net/~jvernee/8233920/webrev.04/

Looks good.

Best regards,
Vladimir Ivanov

> On 13/11/2019 11:28, Vladimir Ivanov wrote:
>>
>>> http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/
>>
>> I like how the patch shapes. Looks good.
>>
>> In addition, you can encapsulate the following code into a helper method:
>>
>>           if (isNonVoid) {
>> -             mv.visitInsn(Opcodes.POP);
>> +             if (isDoubleWord) {
>> +                 mv.visitInsn(Opcodes.POP2);
>> +             } else {
>> +                 mv.visitInsn(Opcodes.POP);
>> +             }
>>           }
>>
>>
>>     if (isNonVoid) {
>>       BasicType basicReturnType = BasicType.basicType(returnType);
>>       emitPopInsn(basicReturnType);
>>     }
>>
>>     private void emitPopInsn(BasicType type) {
>>       mv.visitVarInsn(popInsnOpcode(type));
>>     }
>>
>>     private int popInsnOpcode(BasicType type) {
>>        switch (type) {
>>          case I_TYPE: return Opcodes.POP;
>>          case J_TYPE: return Opcodes.POP2;
>>          case F_TYPE: return Opcodes.POP;
>>          case D_TYPE: return Opcodes.POP2;
>>          case L_TYPE: return Opcodes.POP;
>>          default:
>>            throw new InternalError("unknown type: " + type);
>>     }
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> As long as Claes is okay with it, I think we should go for that one 
>>> since the code/doc is simpler. I can file an RFE for examining the 
>>> performance of different code shapes.
>>>
>>> Thanks,
>>> Jorn
>>>
>>> On 12/11/2019 16:35, Claes Redestad wrote:
>>>> Interesting, and you might be right. However I think it'd be better to
>>>> file a follow-up to examine this (pre-existing) behavior than to 
>>>> expand the scope of this bug fix.
>>>>
>>>> /Claes
>>>>
>>>> On 2019-11-12 16:08, Remi Forax wrote:
>>>>> Hi Jorn, Claes,
>>>>> Is it not better to always store in a local variable, like javac 
>>>>> does, instead of doing the double SWAP if there is a return value 
>>>>> of size 1 ?
>>>>>
>>>>> Rémi
>>>>>
>>>>> ----- Mail original -----
>>>>>> De: "Jorn Vernee" <jorn.vernee at oracle.com>
>>>>>> À: "Claes Redestad" <claes.redestad at oracle.com>, "core-libs-dev" 
>>>>>> <core-libs-dev at openjdk.java.net>
>>>>>> Envoyé: Mardi 12 Novembre 2019 15:56:06
>>>>>> Objet: Re: RFR 8233920: MethodHandles::tryFinally generates 
>>>>>> illegal bytecode for long/double return types
>>>>>
>>>>>> Hi Claes,
>>>>>>
>>>>>> Thanks for the comments!
>>>>>>
>>>>>> Updated webrev: 
>>>>>> http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/
>>>>>>
>>>>>> Also, thanks for sponsoring.
>>>>>>
>>>>>> Jorn
>>>>>>
>>>>>> On 12/11/2019 15:30, Claes Redestad wrote:
>>>>>>> Hi Jorn,
>>>>>>>
>>>>>>> good catch!
>>>>>>>
>>>>>>> Some minor stylistic concerns:
>>>>>>>
>>>>>>> "* = depends on whether the return type is a category 2 type, which
>>>>>>> takes up 2 stack slots."
>>>>>>>
>>>>>>> While long/double are known to use two stack slots, "category 2 
>>>>>>> type"
>>>>>>> isn't used much outside the specification. I'd either simplify to
>>>>>>> "* = depends on whether the return type takes up 1 or 2 stack 
>>>>>>> slots."
>>>>>>> or add a reference to the definition of type categories.
>>>>>>>
>>>>>>> Similarly I think:
>>>>>>>
>>>>>>> boolean returnsCat2Type = returnType == long.class || returnType ==
>>>>>>> double.class;
>>>>>>>
>>>>>>> could be simplified as:
>>>>>>>
>>>>>>> boolean isDoubleWord = basicReturnType.isDoubleWord(); //
>>>>>>> returnsDoubleWord?
>>>>>>>
>>>>>>> In the test, is it possible to narrow the expected exception to the
>>>>>>> exact type being thrown (T1?) rather than Throwable?
>>>>>>>
>>>>>>> I can sponsor the patch.
>>>>>>>
>>>>>>> /Claes
>>>>>>>
>>>>>>> On 2019-11-12 12:09, Jorn Vernee wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the following patch that fixes handling of 
>>>>>>>> long/double
>>>>>>>> returns for the tryFinally MethodHandle combinator.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
>>>>>>>> (Testing=tier1)
>>>>>>>>
>>>>>>>> FWIW, I also added some missing close tags to the javadoc in the
>>>>>>>> InvokerBytecodeGenerator class (IntelliJ was warning about this).
>>>>>>>>
>>>>>>>> As a heads-up; I'm not a committer on the jdk project, so a sponsor
>>>>>>>> would be needed to push this.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jorn