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 On Wed, Oct 31, 2012 at 6:54 PM, Christian Thalinger <[email protected]> wrote: > > On Oct 30, 2012, at 4:25 PM, [email protected] 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/sparc/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 > >> >> >> I'm still looking at other places... >> >> >> Thanks, >> Serguei >> >> >> On 10/30/12 10:41 AM, [email protected] wrote: >>> I have a plan to look at it, at least for other serviceablity code. >>> It'd be good if someone from the runtime or compiler team checked it too. >>> >>> Thanks, >>> Serguei >>> >>> >>> On 10/30/12 10:37 AM, Daniel D. Daugherty wrote: >>>> Thumbs up. >>>> >>>> Is someone going to do an audit for similar missing changes >>>> from max_stack() (not max_size()) to verifier_max_stack()? >>>> >>>> Dan >>>> >>>> >>>> On 10/30/12 1:30 PM, [email protected] wrote: >>>>> Hello, >>>>> >>>>> >>>>> Please, review the fix for CR: >>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7194607 >>>>> >>>>> CR in JIRA: >>>>> https://jbs.oracle.com/bugs/browse/JDK-7194607 >>>>> >>>>> Open webrev: >>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2012/7194607-JVMTI-max_size >>>>> >>>>> >>>>> Summary: >>>>> >>>>> This issue is caused by the changes in the oops/method.hpp for >>>>> invokedynamic (JSR 292). >>>>> Now the max_stack() adds +2 to the original code attribute stack size if >>>>> invokedynamic is enabled. >>>>> The verifier_max_stack() must be used in the >>>>> jvmtiClassFileReconstituter.cpp >>>>> instead of the max_size() to get the code attribute stack size. >>>>> >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>> >> >
