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

[Vladimir Ivanov]

> 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