JDK-8002087 : JSR 292: max_stack issues in interpreter code after JSR-292 merge
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: hs25
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • OS: generic
  • CPU: generic
  • Submitted: 2012-11-01
  • Updated: 2013-11-05
  • Resolved: 2013-11-05
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbd_majorResolved
Related Reports
Duplicate :  
Relates :  
Relates :  
Description
It seems the merge of the JSR-292 added some issues related to change of the Method::max_stack().
Please, see the email exchange below:

On Oct 30, 2012, at 4:25 PM, serguei.spitsyn@oracle.com wrote:

> Ok, it seems there are some suspicious fragments in the interpreter code.
> Christian, could you, please, check and comment the fragments below?
>
> This is how the Method::max_stack() is defined:
>
> src/share/vm/oops/method.hpp:
>
>   int  verifier_max_stack() const                { return _max_stack; }
>   int           max_stack() const                { return _max_stack + extra_stack_entries(); }
>   void      set_max_stack(int size)              {        _max_stack = size; }
>   . . .
>   static int extra_stack_entries() { return EnableInvokeDynamic ? 2 : 0; }
>
>
> The following code fragments are unaware that the method->max_stack() returns _max_stack + extra_stack_entries()  :
>
> src/cpu/sparc/vm/cppInterpreter_sparc.cpp:
> src/cpu/x86/vm/cppInterpreter_x86.cpp:
>
> static int size_activation_helper(int callee_extra_locals, int max_stack, int monitor_size) {
>   . . .
>   const int extra_stack = 0; //6815692//Method::extra_stack_entries();      ????
>   return (round_to(max_stack +
>                    extra_stack +

Remove extra_stack.

>                    . . .
> }
> . . .
> void BytecodeInterpreter::layout_interpreterState(interpreterState to_fill,
>   . . .
>   int extra_stack = 0; //6815692//Method::extra_stack_entries();               ????
>   to_fill->_stack_limit = stack_base - (method->max_stack() + 1 + extra_stack);

Remove extra_stack (but keep the +1; see comment nearby).

>   . . .
> }
>
>
> src/cpu/sparc/vm/templateInterpreter_sparc.cpp:
>
> static int size_activation_helper(int callee_extra_locals, int max_stack, int monitor_size) {
>   . . .
>   const int max_stack_words = max_stack * Interpreter::stackElementWords;
>   return (round_to((max_stack_words
>                    //6815692//+ Method::extra_stack_words()                           ????

The comment needs to be removed.

>   . . .
> }
>
> At the size_activation_helper call sites the second parameter is usually passed as method->max_stack().
>
>
> src/cpu/x86/vm/templateInterpreter_x86_32.cpp:
> src/cpu/x86/vm/templateInterpreter_x86_64.cpp:
>
> int AbstractInterpreter::size_top_interpreter_activation(Method* method) {
>   . . .
>   const int extra_stack = Method::extra_stack_entries();
>   const int method_stack = (method->max_locals() + method->max_stack() + extra_stack) *

Remove extra_stack.

>                            Interpreter::stackElementWords;
>   . . .
> }
>
>
> src/share/vm/interpreter/oopMapCache.cpp:
>
> void OopMapForCacheEntry::compute_map(TRAPS) {
>   . . .
>   // First check if it is a method where the stackmap is always empty
>   if (method()->code_size() == 0 || method()->max_locals() + method()->max_stack() == 0) {
>     _entry->set_mask_size(0);
>   } else {
>   . . .
> }
>
> Above, if the invokedynamic is enabled then the method()->max_stack() can not be 0.
> We need to check it if this fact does not break the fragment.

That means we are always generating oop maps even if we wouldn't need them.  Let me think more about this...

-- Chris

Comments
This bug was fixed as JDK-8010460.
05-11-2013

It looks like the fix of JDK-8010460 addressed most of the concerns. Most likely, this one should be closed as a dup of JDK-8010460. Otherwise, it can take a role of the cleanup bug. I'm lowering the priority to P4 to eliminate the noise in the JDK-8 Rampdown dahsboards. Waiting for reply from Roland...
15-10-2013

I think some of the issues of this bug (maybe all?) got fixed by JDK-8010460. Roland, can you verify?
15-10-2013

Serguei - this sounds like a compiler issue / enhancement?
11-10-2013

There can be more max_stack issues. This is a email from Volker Simonis (volker.simonis@gmail.com): Hi Christian, I run into this issue in our PPC port when switching to HS24. The problem is that there's the following assertion in bytecodeInterpreter.cpp: assert(labs(istate->_stack_base - istate->_stack_limit) == (istate->_method->max_stack() + 1)) failed: bad stack limit This fails because 'istate->_method->max_stack()' takes into account the 'extra_stack_entries()' while the value for 'istate->_stack_limit' does not account for it. 'istate->_stack_limit' is computed in 'CppInterpreterGenerator::generate_compute_interpreter_state()' right from the 'max_stack' member of methodOopDesc. Now if I understand you right, you suggest to remove all the parts which add the 'extra_stack_entries()' to the stack size. But wouldn't that be wrong? Isn't it necessary to prepare the stack to hold these two extra field in case they are needed by a method which contains invokedynamic calls? As far as I can see the template interpreter is still using this 'extra_stack' in 'AbstractInterpreter::size_top_interpreter_activation' although you advised to remove it in your mail. If the 'extra_stack' is really not needed, would it be reasonable to change the mention assertion to use 'verifier_max_stack()' instead of 'max_stack()'? Thank you and best regards, Volker
01-03-2013

For OopMapForCacheEntry::compute_map we should try to find out how much memory we waste here. One partial solution would be to scan the bytecodes for invoke instructions and only increase the stack size in that case. I have some code laying that does the bytecode parsing and sets a flag.
01-11-2012

Fixed a typo in the Summary header: interprter => interpreter.
01-11-2012