review for 7057587: JSR 292 - crash with jruby in test/test_respond_to.rb

[Tom Rodriguez]

Thanks for the reviews.

tom

On Jun 22, 2011, at 2:50 PM, Vladimir Kozlov wrote:

> Good.
> 
> Vladimir
> 
> Tom Rodriguez wrote:
>> I updated the comment to this:
>> diff -r de6a837d75cf src/share/vm/code/nmethod.cpp                                                                                                   --- a/src/share/vm/code/nmethod.cpp                                                                                                                  +++ b/src/share/vm/code/nmethod.cpp                                                                                                                  @@ -1832,7 +1832,9 @@
>>   if (!method()->is_native()) {                                                                                                                           SimpleScopeDesc ssd(this, fr.pc());                                                                                                                   Bytecode_invoke call(ssd.method(), ssd.bci());                                                                                                  -    bool has_receiver = call.has_receiver();                                                                                                        +    // compiled invokedynamic call sites have an implicit receiver at                                                                                +    // resolution time, so make sure it gets GC'ed.                                                                                                  +    bool has_receiver = !call.is_invokestatic();                                                                                                         Symbol* signature = call.signature();                                                                                                                 fr.oops_compiled_arguments_do(signature, has_receiver, reg_map, f);                                                                                 }
>> tom
>> On Jun 22, 2011, at 2:12 PM, Tom Rodriguez wrote:
>>> On Jun 22, 2011, at 2:06 PM, Vladimir Kozlov wrote:
>>> 
>>>> Tom Rodriguez wrote:
>>>>> On Jun 22, 2011, at 12:23 PM, Vladimir Kozlov wrote:
>>>>>> Is it possible to add an assert in nmethod::preserve_callee_argument_oops() to verify that invokedynamic has a receiver?
>>>>> Do you mean to verify that the receiver is really a valid oop or something else?
>>>> Yes, check for a valid oop since it will be GC'ed. What confused me is the sentence: "at this point". Does it mean that at some point invokedynamic does not have a valid receiver?
>>> Yes that's confusing.  At a compiled call site the receiver is always there at a point where we might gc.  In other contexts the order of operations might be different.  In the interpreter the implicit receiver is injected after resolution.  I think this might get cleaned up later to be more consistent.
>>> 
>>> tom
>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>>> tom
>>>>>> Otherwise looks good.
>>>>>> 
>>>>>> thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> Tom Rodriguez wrote:
>>>>>>> http://cr.openjdk.java.net/~never/7057587
>>>>>>> 40 lines changed: 6 ins; 0 del; 34 mod; 4861 unchg
>>>>>>> 7057587: JSR 292 - crash with jruby in test/test_respond_to.rb
>>>>>>> Summary: don't skip receiver when GC'ing compiled invokedynamic callsites
>>>>>>> Reviewed-by:
>>>>>>> When GC'ing at a call site during resolution the arguments to the call
>>>>>>> have to be handled specially.  In most cases the implicit receiver to
>>>>>>> method handle invoke is ignored but in this case it must be treated as
>>>>>>> real so that it is properly GC'ed.  Tested with failing test from
>>>>>>> report.  Also ran jck, regression tests and vm.mlvm tests.
>>>>>>> I also included a fix to the inline level printing.  For method handle
>>>>>>> invokes we don't want to count them against MaxInlineLevel and
>>>>>>> previously this was done by adding a bias to the inline_depth.  This
>>>>>>> screwed up the indenting making the inlining output unreadable.
>>>>>>> Instead we should keep the depth count consistent and adjust the max
>>>>>>> inline level for the subtree.  This is done by keeping that value in
>>>>>>> the InlineTree.  inline_depth was renamed to inline_level to be
>>>>>>> consistent with MaxInlineLevel.