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

[Christian Thalinger]

On Oct 18, 2011, at 11:16 PM, Vladimir Kozlov wrote:

> Christian Thalinger wrote:
>>> 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.
> 
> I don't trust C++ compilers to eliminate is_interpreted_frame() call in product VM even if the body of 'if' statement is empty. You can enclose it with #ifdef ASSERT if you want to keep 'if'.

:-)  Well, I would trust them since it's the simplest optimization of all but I'll add an #ifdef ASSERT.  Thanks for the review.

-- Chris

> 
> Vladimir
> 
>> -- 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