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

[Claes Redestad]

On 2019-11-13 21:49, John Rose wrote:

On Nov 13, 2019, at 12:39 PM, Claes Redestad  wrote:

Similar to other promising candidates like constant folding I think it
opens up some more general questions about observability. For example:
do we retain the original bytecode and mappings everything back to it
so that a debugger would be none the wiser about any inlining that may
have happened? Do we "simply" deoptimize and redefine everything back to
the stashed original when someone hooks in a debugger..? (Do we go so
far as to allow opting out of debuggers entirely?)

Yep.  These questions are why link-time optimization is not a slam dunk.
If it were, I suppose we’d have it already.

Debugging is especially hard.  We don’t want to give it up, but any
program transformation is going to make debugging harder to implement.
You should see what the JITs do to support deopt; like exact GC, it’s
something that makes them different animals from the usual llvm/gcc
compilers.

That said, a bytecode-to-bytecode “deoptimization” is easier to specify
and think about than a machine-code-to-bytecode deopt, which is what
the JITs support.  Juicy problems everywhere!


Yes, not trivial, but likely much easier.



Also I wonder if stashing classifies for debugging only will motivate a partial
resurrection of pack200, for storing said things “cold” on disk.


AFAIU, pack200 comes with a maintenance burden that we'd like to
permanently get rid of. For a stashed-away class file archive only used
when debugging then plain old zip at a high compression level might be
sufficient.

/Claes

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

[John Rose]

On Nov 13, 2019, at 12:39 PM, Claes Redestad  wrote:
> Similar to other promising candidates like constant folding I think it
> opens up some more general questions about observability. For example:
> do we retain the original bytecode and mappings everything back to it
> so that a debugger would be none the wiser about any inlining that may
> have happened? Do we "simply" deoptimize and redefine everything back to
> the stashed original when someone hooks in a debugger..? (Do we go so
> far as to allow opting out of debuggers entirely?)

Yep.  These questions are why link-time optimization is not a slam dunk.
If it were, I suppose we’d have it already.

Debugging is especially hard.  We don’t want to give it up, but any
program transformation is going to make debugging harder to implement.
You should see what the JITs do to support deopt; like exact GC, it’s
something that makes them different animals from the usual llvm/gcc
compilers.

That said, a bytecode-to-bytecode “deoptimization” is easier to specify
and think about than a machine-code-to-bytecode deopt, which is what
the JITs support.  Juicy problems everywhere!

Also I wonder if stashing classifies for debugging only will motivate a partial
resurrection of pack200, for storing said things “cold” on disk.

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

[Claes Redestad]

On 2019-11-13 21:04, John Rose wrote:
On Nov 13, 2019, at 11:24 AM, Claes Redestad > wrote:


Thanks for suggesting that Vladimir.  That indeed is how the existing 
code is organized.


Yes, not very startup friendly... ;-)


No, but it’s maintainer friendly.  We can walk and chew gum at the same 
time >

Is it time to think about a jlink plugin which inlines simple method calls?
Perhaps we need to wait until we understand (semi-)closed-world packaging.

Long, long ago, a very early version of javac did inlining of simple 
method calls.
That functionality was removed when we realized that this was placing 
“getfield”
instructions in classes which didn’t have access to the referenced 
private fields,
causing code to break.  It also messed up the binary compatibility story 
required

to make sense of independently recompiled class files.  How innocent we were
back then.  We won’t make those same mistakes again, but we could try the
same trick again, more carefully, in jlink or some other static 
packaging workflow.


As a startup optimization, inlining small (private?) methods at link-
time seems like a promising candidate, yes.

Similar to other promising candidates like constant folding I think it
opens up some more general questions about observability. For example:
do we retain the original bytecode and mappings everything back to it
so that a debugger would be none the wiser about any inlining that may
have happened? Do we "simply" deoptimize and redefine everything back to
the stashed original when someone hooks in a debugger..? (Do we go so
far as to allow opting out of debuggers entirely?)

/Claes

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

[John Rose]

On Nov 13, 2019, at 11:24 AM, Claes Redestad  wrote:
> 
>> Thanks for suggesting that Vladimir.  That indeed is how the existing code 
>> is organized.
> 
> Yes, not very startup friendly... ;-)

No, but it’s maintainer friendly.  We can walk and chew gum at the same time.

Is it time to think about a jlink plugin which inlines simple method calls?
Perhaps we need to wait until we understand (semi-)closed-world packaging.

Long, long ago, a very early version of javac did inlining of simple method 
calls.
That functionality was removed when we realized that this was placing “getfield”
instructions in classes which didn’t have access to the referenced private 
fields,
causing code to break.  It also messed up the binary compatibility story 
required
to make sense of independently recompiled class files.  How innocent we were
back then.  We won’t make those same mistakes again, but we could try the
same trick again, more carefully, in jlink or some other static packaging 
workflow.

— John

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

[Claes Redestad]

On 2019-11-13 20:05, John Rose wrote:
On Nov 13, 2019, at 2:28 AM, Vladimir Ivanov 
mailto:vladimir.x.iva...@oracle.com>> wrote:


   private void emitPopInsn(BasicType type) {
 mv.visitVarInsn(popInsnOpcode(type));
   }



Thanks for suggesting that Vladimir.  That indeed is how the existing 
code is organized.


Yes, not very startup friendly... ;-)

I'll do some sanity testing before push.

/Claes

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

[John Rose]

On Nov 13, 2019, at 2:28 AM, Vladimir Ivanov mailto:vladimir.x.iva...@oracle.com>> wrote:
> 
>private void emitPopInsn(BasicType type) {
>  mv.visitVarInsn(popInsnOpcode(type));
>}
> 

Thanks for suggesting that Vladimir.  That indeed is how the existing code is 
organized.

Re: 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" 
À: "Claes Redestad" , "core-libs-dev" 


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

Re: 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" 
À: "Claes Redestad" , "core-libs-dev" 


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

Re: 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" 
À: "Claes Redestad" , "core-libs-dev" 


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

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

[Claes Redestad]

Hi, since you've already done the work I guess I'm ok with it. :-)

While I don't expect there to be a noticeable performance difference, it
seems prudent to investigate and add a simple microbenchmark. This can
be done as a future RFE.

/Claes

On 2019-11-12 17:05, Jorn Vernee wrote:

Hi Rémi, Claes,

Thanks for the suggestion, Rémi. I avoided using a store/load for all 
types since I didn't want to alter the existing code shape. But, using 
store/load for all types simplifies the generator code & javadoc, so it 
would be nice to do that I think. However, this might have other effects 
on peak/link-time performance as well.


Given that using store/load aligns the code-shape with what javac does, 
I think chances are good that the JIT will have an easier time 
optimizing that code shape. That said, it really needs more investigation.


Here is an updated webrev that uses store/load for all return types: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/


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" 
À: "Claes Redestad" , "core-libs-dev" 


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

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

[Jorn Vernee]

Hi Rémi, Claes,

Thanks for the suggestion, Rémi. I avoided using a store/load for all 
types since I didn't want to alter the existing code shape. But, using 
store/load for all types simplifies the generator code & javadoc, so it 
would be nice to do that I think. However, this might have other effects 
on peak/link-time performance as well.


Given that using store/load aligns the code-shape with what javac does, 
I think chances are good that the JIT will have an easier time 
optimizing that code shape. That said, it really needs more investigation.


Here is an updated webrev that uses store/load for all return types: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/


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" 
À: "Claes Redestad" , "core-libs-dev" 


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

Re: 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" 
À: "Claes Redestad" , "core-libs-dev" 

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

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

[Remi Forax]

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" 
> À: "Claes Redestad" , "core-libs-dev" 
> 
> 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

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

[Jorn Vernee]

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

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

[Claes Redestad]

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

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

[Jorn Vernee]

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