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

[Christian Thalinger]

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
>>>> 
>> 
>