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

[Claes Redestad]

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