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.ver...@oracle.com>
À: "Claes Redestad" <claes.redes...@oracle.com>, "core-libs-dev"
<core-libs-dev@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
