RFR: 8067247: Crash: assert(method_holder->data() == 0 ...) failed: a) MT-unsafe modification of inline cache

[Jamsheed C m]

Sure. Thank you Vladimir Ivanov!

Best Regards,
Jamsheed

On 4/1/2016 3:18 PM, Vladimir Ivanov wrote:
> Looks good!
>
> Small detail: the following comment in the test is misleading:
>
>   71         test();  // new LF creation should fail.
>
> LF shouldn't be unloaded, so no new LF is normally not instantiated.
>
> Something like the following:
>   // Trigger call site re-resolution. Invoker LambdaForm should stay 
> the same.
>   test();
>
> No need to send new webrev.
>
> Best regards,
> Vladimir Ivanov
>
> On 4/1/16 12:02 PM, Jamsheed C m wrote:
>> Hi Vladimir Ivanov,
>>
>> I used overloaded clearInlineCaches wb api.
>>
>> revised webrevs:
>> hs: http://cr.openjdk.java.net/~jcm/8067247/webrev.hs.02/
>> root: http://cr.openjdk.java.net/~jcm/8067247/webrev.hs_comp.01/
>>
>> Best Regards,
>> Jamsheed
>>
>> On 3/28/2016 9:17 PM, Vladimir Ivanov wrote:
>>>>> in addition it clears this.
>>>>> void static_stub_Relocation::clear_inline_cache() {
>>>>>   // Call stub is only used when calling the interpreted code.
>>>>>   // It does not really need to be cleared, except that we want to
>>>>> clean out the methodoop.
>>>>>   CompiledStaticCall::set_stub_to_clean(this);
>>>>
>>>> i want assert to catch this issue. if static stubs are cleared, assert
>>>> wouldn't fail.
>>> I see. Then I suggest to rename the method to
>>> WhiteBox.cleanupInlineCaches() and iterate over the whole code cache
>>> (don't specify Method*).
>>>
>>> void CodeCache::cleanup_inline_caches() {
>>>   assert_locked_or_safepoint(CodeCache_lock);
>>>   NMethodIterator iter;
>>>   while(iter.next_alive()) {
>>>     iter.method()->cleanup_inline_caches(true);
>>>   }
>>> }
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>
>>>> -Jamsheed
>>>>> }
>>>>>
>>>>> Best Regards,
>>>>> Jamsheed
>>>>>>
>>>>>> WB_ENTRY(void, WB_ClearInlineCaches(JNIEnv* env, jobject wb))
>>>>>>   VM_ClearICs clear_ics;
>>>>>>   VMThread::execute(&clear_ics);
>>>>>> WB_END
>>>>>>
>>>>>> class VM_ClearICs: public VM_Operation {
>>>>>> ...
>>>>>>   void doit()         { CodeCache::clear_inline_caches(); }
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> void CodeCache::clear_inline_caches() {
>>>>>>   assert_locked_or_safepoint(CodeCache_lock);
>>>>>>   NMethodIterator iter;
>>>>>>   while(iter.next_alive()) {
>>>>>>     iter.method()->clear_inline_caches();
>>>>>>   }
>>>>>> }
>>>>>>
>>>>>> void nmethod::clear_inline_caches() {
>>>>>>   assert(SafepointSynchronize::is_at_safepoint(), "cleaning of IC's
>>>>>> only allowed at safepoint");
>>>>>>   if (is_zombie()) {
>>>>>>     return;
>>>>>>   }
>>>>>>
>>>>>>   RelocIterator iter(this);
>>>>>>   while (iter.next()) {
>>>>>>     iter.reloc()->clear_inline_cache();
>>>>>>   }
>>>>>> }
>>>>>>
>>>>>> void static_call_Relocation::clear_inline_cache() {
>>>>>>   // Safe call site info
>>>>>>   CompiledStaticCall* handler = compiledStaticCall_at(this);
>>>>>>   handler->set_to_clean();
>>>>>> }
>>>>>>
>>>>>> void opt_virtual_call_Relocation::clear_inline_cache() {
>>>>>>   // No stubs for ICs
>>>>>>   // Clean IC
>>>>>>   ResourceMark rm;
>>>>>>   CompiledIC* icache = CompiledIC_at(this);
>>>>>>   icache->set_to_clean();
>>>>>> }
>>>>>>
>>>>>> void virtual_call_Relocation::clear_inline_cache() {
>>>>>>   // No stubs for ICs
>>>>>>   // Clean IC
>>>>>>   ResourceMark rm;
>>>>>>   CompiledIC* icache = CompiledIC_at(this);
>>>>>>   icache->set_to_clean();
>>>>>> }
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>>>
>>>>>>> Best Regards,
>>>>>>> Jamsheed
>>>>>>>
>>>>>>> On 3/26/2016 1:50 PM, Dean Long wrote:
>>>>>>>> Instead of changing cleanup_inline_caches() to take a new flag, 
>>>>>>>> can
>>>>>>>> you use the existing
>>>>>>>> clear_inline_caches()?
>>>>>>>>
>>>>>>>> dl
>>>>>>>>
>>>>>>>> On 3/25/2016 9:54 AM, Jamsheed C m wrote:
>>>>>>>>> Thank you Chris.
>>>>>>>>> I have updated the code.
>>>>>>>>>
>>>>>>>>> + if (method == NULL) {
>>>>>>>>> + return;
>>>>>>>>> + }
>>>>>>>>> + nmethod* nm = method->code();
>>>>>>>>> + if (nm == NULL || nm->is_unloaded()) {
>>>>>>>>> + return;
>>>>>>>>> + }
>>>>>>>>> + nm->cleanup_inline_caches(true);
>>>>>>>>> Best Regards,
>>>>>>>>> Jamsheed
>>>>>>>>>
>>>>>>>>> On 3/25/2016 6:58 AM, Christian Thalinger wrote:
>>>>>>>>>>
>>>>>>>>>>> On Mar 24, 2016, at 11:05 AM, Jamsheed C m
>>>>>>>>>>> <jamsheed.c.m at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi All,
>>>>>>>>>>>
>>>>>>>>>>> Request for review,
>>>>>>>>>>>
>>>>>>>>>>> bug url: https://bugs.openjdk.java.net/browse/JDK-8067247
>>>>>>>>>>>
>>>>>>>>>>> webrevs:
>>>>>>>>>>> fix:
>>>>>>>>>>>   jdk part:
>>>>>>>>>>> http://cr.openjdk.java.net/~jcm/8067247/webrev.jdk.00/
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Ejcm/8067247/webrev.jdk.00/>
>>>>>>>>>>>
>>>>>>>>>>> newly added test case
>>>>>>>>>>> hotspot part:
>>>>>>>>>>> http://cr.openjdk.java.net/~jcm/8067247/webrev.hs.00/
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Ejcm/8067247/webrev.hs.00/>
>>>>>>>>>>> under hs-comp/test
>>>>>>>>>>> http://cr.openjdk.java.net/~jcm/8067247/webrev.hs_comp.00/
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Ejcm/8067247/webrev.hs_comp.00/>
>>>>>>>>>>>
>>>>>>>>>>> Unit Test: test/compiler/jsr292/misc/gc/MHInvokeTest.java
>>>>>>>>>>> Testing: JPRT with new test case, with fix, without fix
>>>>>>>>>>>
>>>>>>>>>>> Problem Summary:  MH.invoke linksite take assistance of java 
>>>>>>>>>>> code
>>>>>>>>>>> to get an adapter method. Here a new method holder class and a
>>>>>>>>>>> adapter method are created for a MT and lform instance is 
>>>>>>>>>>> cached.
>>>>>>>>>>> Normally this cached lform get returned for a linksite 
>>>>>>>>>>> request of
>>>>>>>>>>> same MT.  When these cached lform get collected(due to memory
>>>>>>>>>>> pressure),  a new class and method gets created for same 
>>>>>>>>>>> MT(even
>>>>>>>>>>> though old method holder class and  adapter method are live).
>>>>>>>>>>> Fix Summary: Kept a strong reference to lform instance in 
>>>>>>>>>>> adapter
>>>>>>>>>>> method holder class of  MT.
>>>>>>>>>>
>>>>>>>>>> Wow!  You found the cause for his long-standing issue? Nice.
>>>>>>>>>> + if (method == NULL) { return; }
>>>>>>>>>> + nmethod* nm = method->code();
>>>>>>>>>> + if (nm == NULL) { return; }
>>>>>>>>>> + if (nm->is_unloaded()) { return; }
>>>>>>>>>> Please put the return and } on separate lines.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Best Regards,
>>>>>>>>>>> Jamsheed
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>