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

[Roland Westrelin]

Hi Volker,

> 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.

Thanks. I wasn't aware of this one. I've linked the 2 bugs in the database.

> 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..

I'll go with your suggestion.

I'll let Christian or John comment on the change below.

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");
> 
>     }
> 
> 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