Request for reviews (M): 7090904: JSR 292: JRuby junit test crashes in PSScavengeRootsClosure::do_oop

[Vladimir Kozlov]

Thank you, Tom, for explaining changes in those files. They look good.

Vladimir

Tom Rodriguez wrote:
> On Oct 18, 2011, at 11:25 AM, Vladimir Kozlov wrote:
> 
>> Christian,
>>
>> In deoptimization.cpp add parenthesis:
>>
>> !     if (index == array->frames() - 1 && caller_was_method_handle) {
>> ---
>> !     if ((index == array->frames() - 1) && caller_was_method_handle) {
>>
>>
>> I think you need move the check inside assert:
>>
>> +     if (caller->is_interpreted_frame()) {
>> +       assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
>> +     }
>> ---
>> +     assert(!caller->is_interpreted_frame() ||
>> +            (locals < caller->fp() + frame::interpreter_frame_initial_sp_offset), "bad placement");
>>
>>
>> Why assert was not added to sparc?
> 
> I believe sparc already has equivalent assertions.  Christian, did you try the jruby tests on sparc looking for similar issues?  Are we able to run +DeoptimizeALot now or at there still more issues?
> 
>> You did not explain changes in bytecodeTracer.cpp and in frame.cpp (frame::describe()). They seems unrelated to this fix.
> 
> They were changes I made while diagnosing the problem I was seeing.  The frame.cpp are about allowing describe be called on a different thread and not producing garbage fp values for compiled frames.  The bytecodeTracer.cpp fixes printing of invokedynamic bytecodes.  Previously it would always complain about the index being wrong but now it actually prints it like an invoke call site.
> 
> tom
> 
>> Thanks,
>> Vladimir
>>
>> Christian Thalinger wrote:
>>> http://cr.openjdk.java.net/~twisti/7090904/
>>> 7090904: JSR 292: JRuby junit test crashes in PSScavengeRootsClosure::do_oop
>>> Reviewed-by:
>>> A couple of JRuby junit tests crash in various forms (mostly GC
>>> related) when running on 64-bit Linux.  Although it looks like a GC
>>> related problem it actually is a deoptimization bug.
>>> Tom already had a fix ready but we were unable to write a test case to
>>> prove the correctness of the fix.  An assert added in
>>> AbstractInterpreter::layout_activation shows that the JRuby crashes
>>> are indeed an incarnation of this deoptimization bug.
>>> The bug itself is in the deoptimization logic that calculates the size
>>> of parameter space in the caller frame.  Method handle invokes may
>>> involve fairly arbitrary chains of calls so it's impossible to know
>>> how much actual space the caller has for locals.
>>> The fix is to always assume zero parameters for method handle call
>>> sites.
>>> Extensive testing with JRuby junit tests.
>>> src/share/vm/runtime/deoptimization.cpp
>>> src/cpu/x86/vm/templateInterpreter_x86_32.cpp
>>> src/cpu/x86/vm/templateInterpreter_x86_64.cpp
>>> src/share/vm/interpreter/bytecodeTracer.cpp
>>> src/share/vm/runtime/frame.cpp
>>> src/share/vm/runtime/frame.hpp
>>> src/share/vm/runtime/thread.cpp
>