RFR (M): 8019519: PPC64 (part 105): C interpreter: implement support for jvmti early return.

[Lindenmaier, Goetz]

Hi Serguei,

>The above will reset the msg to the return_from_method
>when it was already set to the early_return which is
>inconsistent with the popping_frame case.

That's because the frame manager tests for popping_frame.
The early_return case is handled as a normal return, we faked the
return result in the cppInterpreter code that's in this change.

See also
http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/874e94d3b21d/src/cpu/ppc/vm/cppInterpreter_ppc.cpp
line 1922

>  jc.can_force_early_return = 1;
We will enable the capabilities in a change later on.

Best regards & thanks for your review,
  Goetz.



From: serguei.spitsyn at oracle.com [mailto:serguei.spitsyn at oracle.com]
Sent: Saturday, July 06, 2013 3:10 AM
To: Lindenmaier, Goetz
Cc: Vladimir Kozlov; 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
Subject: Re: RFR (M): 8019519: PPC64 (part 105): C interpreter: implement support for jvmti early return.


Hi Goetz,

As before it looks good in general.

I'm not sure the following change is correct:



-      THREAD->clr_pop_frame_in_process();

+    } else {

+      istate->set_msg(return_from_method);

     }

The above will reset the msg to the return_from_method
when it was already set to the early_return which is
inconsistent with the popping_frame case.

Could you explain why did you make it for non-popping_frame cases only?
It would not surprise me if it does not matter though. :)


Also, one more case needs to be added to the  BytecodeInterpreter::C_msg():

322?      case BytecodeInterpreter::early_return:  return("early_return");


One more comment below...


On 7/4/13 5:39 AM, Lindenmaier, Goetz wrote:
Hi,

pop_frame_in_process will be cleared when the frame manager calls
the interpreter loop again with the message popping_frame.
6953477 broke our pop_frame tests. I don't see though how pop_frame
could be tested with 6953477, as the capability is not set
if CC_INTERP is defined.

It is explicitly disabled in the jvmtiManageCapabilities.cpp:
#ifndef CC_INTERP
  jc.can_pop_frame = 1;
  jc.can_force_early_return = 1;
#endif // !CC_INTERP

It is because it was not fully implemented yet for CC_INTERP.
You can take the can_force_early_return out of the ifdef.


Thanks,
Serguei


 (I removed a ShouldNotReachHere in the updated change, that was
in the wrong patch of the port but belongs here.)

_return_kind is unused.  In the updated webrev I also removed the
definition of the field.

I also fixed the syntax stuff.

This is the new webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8019519-cInter_earlyRet-2/<http://cr.openjdk.java.net/%7Egoetz/webrevs/8019519-cInter_earlyRet-2/>

Best regards,
  Goetz.





From: serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com> [mailto:serguei.spitsyn at oracle.com]
Sent: Mittwoch, 3. Juli 2013 21:59
To: Lindenmaier, Goetz
Cc: 'hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>'; 'ppc-aix-port-dev at openjdk.java.net<mailto:ppc-aix-port-dev at openjdk.java.net>'; Vladimir Kozlov
Subject: Re: RFR (M): 8019519: PPC64 (part 105): C interpreter: implement support for jvmti early return.

Hi Goetz,

The fix looks good in general.
Thank you for doing this!

A couple of comments:

src/share/vm/interpreter/bytecodeInterpreter.cpp:

It looks like you have accidentally dropped the line:

2981       THREAD->clr_pop_frame_in_process();

What is the reason that the following lines are deleted? (the same as Vladimir already asked):

2962       istate->set_return_kind((Bytecodes::Code)opcode);

2987     istate->set_return_kind((Bytecodes::Code)opcode);

Thanks,
Serguei

On 7/3/13 3:24 AM, Lindenmaier, Goetz wrote:

Hi,



This change implements jvmti early return  and pop frame support

in the cppInterpreter. To work properly, the corresponding properties in

JvmtiManageCapabilities::init_onload_capabilities() must be enabled.

We will add this in a later change.



I would be happy about a review of this change:

http://cr.openjdk.java.net/~goetz/webrevs/8019519-cInter_earlyRet/<http://cr.openjdk.java.net/%7Egoetz/webrevs/8019519-cInter_earlyRet/>



Best regards,

  Goetz.




-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20130706/5e51c91a/attachment.html