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

[Jamsheed C m]

On 3/28/2016 4:48 PM, Jamsheed C m wrote:
>
>
> On 3/28/2016 4:44 PM, Vladimir Ivanov wrote:
>>
>>> clear_inline_caches clears static stubs. i wanted this api to look more
>>> like cleanup_inline_caches functionality(with clean all ics). So 
>>> used this.
>>
>> Are you sure? WhiteBox.clearInlineCaches() should clear all ICs in 
>> alive nmethods. Why doesn't it work for you?
>
> 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.
-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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>