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
<christian.thalin...@oracle.com> wrote:
>
> On Oct 30, 2012, at 4:25 PM, serguei.spit...@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/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, serguei.spit...@oracle.com 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, serguei.spit...@oracle.com 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
>>>>>
>>>
>>
>
