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

[Volker Simonis]

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

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/940bba24/attachment.html