RFR (XS): 8010460: Interpreter on some platforms loads ConstMethod::_max_stack and misses extra stack slots for JSR 292

[Christian Thalinger]

On Apr 3, 2013, at 5:06 AM, Volker Simonis <volker.simonis at gmail.com> wrote:

> Hi Roland,
> 
> this issue is almost the same as:
> 
> 8002087 : JSR 292: max_stack issues in interpreter code after JSR-292 merge
> (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8002087)
> 
> I think besides the oop map problem (in OopMapForCacheEntry::compute_map) your change fixes all other issues described there.
> 
> On Fri, Mar 29, 2013 at 7:22 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> Thanks for reviewing this.
> 
> > Good.  It's time to get rid of the //6815692// stuff.  The extra_stack variables (always zero now) can go away too.
> 
> I'm not sure what to do with this:
>   guarantee(!EnableInvokeDynamic, "no support yet for java.lang.invoke.MethodHandle"); //6815692
>   //6815692//if (EnableInvokeDynamic)
>   //6815692//  __ inc(O3, Method::extra_stack_entries());
> 
> in cppInterpreter_sparc.cpp
> 
> 
> Well, it wouldn't work anyway if  running with 'EnableInvokeDynamic' enabled.
> But you have:
> 1217   __ lduh(O3, in_bytes(ConstMethod::max_stack_offset()), O3);
> 1218   guarantee(!EnableInvokeDynamic, "no support yet for java.lang.invoke.MethodHandle"); //6815692
> 1219   //6815692//if (EnableInvokeDynamic)
> 1220   //6815692//  __ inc(O3, Method::extra_stack_entries());
> and now, after your change, max_stack automatically accounts for the extra stack entries, so I'd delete the lines:
> 1219   //6815692//if (EnableInvokeDynamic)
> 1220   //6815692//  __ inc(O3, Method::extra_stack_entries());
> Just for the case that somebody ever wants to implement 'EnableInvokeDynamic' for the CPP-interpreter on SPARC. Maybe you could also remove the guarantee and hope for the best..
> 
> For our ppc-port (http://hg.openjdk.java.net/ppc-aix-port/jdk7u/hotspot/file/tip/src/cpu/ppc/vm/cppInterpreter_ppc.cpp) I've fixed this by inserting:
>   __ ppc_lhz(max_stack, method_(max_stack));
> 
>   if (EnableInvokeDynamic) {
>     // Take into account 'extra_stack_entries' needed by method handles (see method.hpp)
>     __ ppc_addi(max_stack, max_stack, methodOopDesc::extra_stack_entries());
> 
>   }
> but that's not necessary anymore, after your change.
>  
> > I think extra_stack_words can be deleted, and extra_stack_entries can be made private to Method.
> >
> > This line of code is redundant in src/cpu/x86/vm/templateInterpreter_x86_{32,64}.cpp:
> >    const int extra_stack = Method::extra_stack_entries();
> 
> I applied Christian's suggestions and yours and found a reference to Method::extra_stack_entries() in c2 that I assume can be removed. Here is a new webrev:
> 
> http://cr.openjdk.java.net/~roland/8010460/webrev.01/
> 
> Roland.
> 
> You may also consider to fix another problem which only occurs if you use a hsx24 or later with pre-hsx24 class library (i.e. if building hsx24/hsx25 with a pre-hsx24 bootstrap JDK). It is an assertion in bytecodeInterpreter.cpp:
> 
>     // We have a problem here if we are running with a pre-hsx24 JDK (for example during bootstrap)
>     // because in that case, EnableInvokeDynamic is true by default but will be later switched off
>     // if java_lang_invoke_MethodHandle::compute_offsets() detects that the JDK only has the classes
> 
>     // for the old JSR292 implementation.
>     // This leads to a situation where 'istate->_stack_limit' always accounts for
>     // methodOopDesc::extra_stack_entries() because it is computed in
>     // CppInterpreterGenerator::generate_compute_interpreter_state() which was generated while
> 
>     // EnableInvokeDynamic was still true. On the other hand, istate->_method->max_stack() doesn't
>     // account for extra_stack_entries() anymore because at the time when it is called
>     // EnableInvokeDynamic was already set to false.
> 
>     // So we have a second version of the assertion which handles the case where EnableInvokeDynamic was
>     // switched off because of the wrong classes.
>     if (EnableInvokeDynamic || FLAG_IS_CMDLINE(EnableInvokeDynamic)) {
> 
>       assert(labs(istate->_stack_base - istate->_stack_limit) == (istate->_method->max_stack() + 1), "bad stack limit");
>     }
>     else {
>       const int extra_stack_entries = 2; // MUST match the value in methodOopDesc::extra_stack_entries()
> 
>       assert(labs(istate->_stack_base - istate->_stack_limit) == (istate->_method->max_stack() + extra_stack_entries
>                                                                                                + 1), "bad stack limit");
> 
>     }

Oh boy.  I'm not sure if that bootstrap problem still exists.  Could someone verify that?

-- Chris

> You can see the actual patch we did at: http://hg.openjdk.java.net/ppc-aix-port/jdk7u/hotspot/diff/61615792f0fe/src/share/vm/interpreter/bytecodeInterpreter.cpp
> 
> Regards,
> Volker
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20130403/cb768a39/attachment.html