Thanks. -- Chris On Oct 31, 2012, at 5:46 PM, serguei.spit...@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.spit...@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.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 >>>>>>> >> >
