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

[Jamsheed C m]

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