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

[Vladimir Ivanov]

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