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

[Christian Thalinger]

On Oct 18, 2011, at 8:25 PM, 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) {

OK.

> 
> 
> 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 do you think that?  I very easy to read with the if.

-- Chris

> 
> 
> Why assert was not added to sparc?
> 
> You did not explain changes in bytecodeTracer.cpp and in frame.cpp (frame::describe()). They seems unrelated to this fix.
> 
> 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