1-line review request: 7194607 VerifyLocalVariableTableOnRetransformTest.sh fails after JSR-292 merge

[serguei.spitsyn at oracle.com]

Hi Volker,

I've added your questions into a comment section of the bug report:
   https://jbs.oracle.com/bugs/browse/JDK-8002087

It is to keep all possible issues accounted.
I do not have a personal preference yet on how to resolve this better.
So, let's wait for Christian to answer.

Thanks,
Serguei

On 3/1/13 10:49 AM, Volker Simonis wrote:
> 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.thalinger at oracle.com> wrote:
>> On Oct 30, 2012, at 4:25 PM, serguei.spitsyn at 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.spitsyn at 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.spitsyn at 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
>>>>>>