Hi Volker,
I've added your questions into a comment section of the bug report:
https://jbs.oracle.com/bugs/browse/JDK-8002087
It is to keep all possible issues accounted.
I do not have a personal preference yet on how to resolve this better.
So, let's wait for Christian to answer.
Thanks,
Serguei
On 3/1/13 10:49 AM, Volker Simonis wrote:
Hi Christian,
I run into this issue in our PPC port when switching to HS24.
The problem is that there's the following assertion in bytecodeInterpreter.cpp:
assert(labs(istate->_stack_base - istate->_stack_limit) ==
(istate->_method->max_stack() + 1)) failed: bad stack limit
This fails because 'istate->_method->max_stack()' takes into account
the 'extra_stack_entries()' while the value for 'istate->_stack_limit'
does not account for it. 'istate->_stack_limit' is computed in
'CppInterpreterGenerator::generate_compute_interpreter_state()' right
from the 'max_stack' member of methodOopDesc.
Now if I understand you right, you suggest to remove all the parts
which add the 'extra_stack_entries()' to the stack size. But wouldn't
that be wrong? Isn't it necessary to prepare the stack to hold these
two extra field in case they are needed by a method which contains
invokedynamic calls? As far as I can see the template interpreter is
still using this 'extra_stack' in
'AbstractInterpreter::size_top_interpreter_activation' although you
advised to remove it in your mail.
If the 'extra_stack' is really not needed, would it be reasonable to
change the mention assertion to use 'verifier_max_stack()' instead of
'max_stack()'?
Thank you and best regards,
Volker
On Wed, Oct 31, 2012 at 6:54 PM, Christian Thalinger
<christian.thalin...@oracle.com> 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
