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

[Volker Simonis]

On Wed, Apr 3, 2013 at 9:33 PM, Christian Thalinger <
christian.thalinger at oracle.com> wrote:

>
> 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
>
>
Of course it does exist, otherwise I wouldn't had to fix it:)
Please read the above comment for the details why it occurs.

@Roland: the above fix still uses 'extra_stack_entries = 2' which would
have to be changed to '1' with your change. An even better solution would
be to define it as a public constant or enum in the 'Method' class and use
that in 'extra_stack_entries()' and in the above assertion. I wanted to do
that in our port but then left it as is to avoid changes in shared code.

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/20130404/4928db84/attachment.html