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

[serguei.spitsyn at oracle.com]

Hi Goetz,

Thank you for the explanations.
A thumbs up from me.

Thanks,
Serguei

On 7/6/13 12:35 PM, Lindenmaier, Goetz wrote:
>
> 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/20130708/503fc85b/attachment-0001.html