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
