[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: 1-line review request: 7194607 VerifyLocalVariableTableOnRetransformTest.sh fails after JSR-292 merg
From: serguei.spitsyn () oracle ! com (serguei ! spitsyn at oracle ! com)
Date: 2012-10-31 21:13:06
Message-ID: 50919462.7010808 () oracle ! com
[Download RAW message or body]
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
> > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic