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

[Christian Thalinger]

Thanks.  -- Chris

On Oct 31, 2012, at 5:46 PM, serguei.spitsyn at oracle.com wrote:

> I've created the hotspot/runtime CR:
>  https://jbs.oracle.com/bugs/browse/JDK-8002087
> 
> Assigned to myself.
> 
> Thanks,
> Serguei
> 
> 
> On 10/31/12 2:13 PM, serguei.spitsyn at oracle.com wrote:
>> Thanks, Christian.
>> 
>> I'm not comfortable to fix it as a part of 7194607.
>> One question is what tests are need to be run to verify possible fixes.
>> 
>> I'll open a separate CR under hotspot/runtime to track the Interpreter issues related to max_stack.
>> 
>> 
>> Thanks,
>> Serguei
>> 
>> On 10/31/12 10:54 AM, Christian Thalinger 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
>>>>>>> 
>> 
>