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

[Jorn Vernee]

Hi Vladimir,

Thanks for the suggestion, I think it sounds good.

Here is the updated webrev: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.04/

Thanks,
Jorn

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