[9] RFR(S): 8152947: VM crash with assert(!removed || is_in_use()) failed: unused osr nmethod should be invalidated

[Tobias Hartmann]

Thanks, Vladimir!

Best regards,
Tobias

On 29.03.2016 18:14, Vladimir Ivanov wrote:
> Reviewed.
> 
> Best regards,
> Vladimir Ivanov
> 
> On 3/29/16 6:56 PM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> On 29.03.2016 17:42, Vladimir Ivanov wrote:
>>> Does it still work when method() == NULL?
>>>
>>> +#ifdef ASSERT
>>> +    // Make sure osr nmethod is invalidated, i.e. not on the list
>>> +    bool found = method()->method_holder()->remove_osr_nmethod(this);
>>> +    assert(!found, "osr nmethod should have been invalidated");
>>> +#endif
>>
>> Yes, you are right. We should check for method() != NULL. New webrev:
>> http://cr.openjdk.java.net/~thartmann/8152947/webrev.01
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 3/29/16 6:28 PM, Tobias Hartmann wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 29.03.2016 15:05, Dmitry Dmitriev wrote:
>>>>> Hi Tobias,
>>>>>
>>>>> I think that "#ifdef ASSERT" must be instead of "#ifndef ASSERT" on lines 1338 and 1516. And it seems that the whole fix should be retested in this case.
>>>>
>>>> Oh, thanks for catching this!
>>>>
>>>> Here is the new webrev:
>>>> http://cr.openjdk.java.net/~thartmann/8152947/webrev.01
>>>>
>>>> I verified that this (still) fixes the problem and re-submitted the tests.
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>>>
>>>>> Thanks,
>>>>> Dmitry
>>>>>
>>>>> On 29.03.2016 15:01, Tobias Hartmann wrote:
>>>>>> Hi,
>>>>>>
>>>>>> please review the following patch:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8152947
>>>>>> http://cr.openjdk.java.net/~thartmann/8152947/webrev.00/
>>>>>>
>>>>>> JDK-8023191 added an assert to check if osr nmethods are invalidated (i.e. removed from the osr list). The assert is too strong because nmethod::invalidate_osr_method() may be executed by multiple threads that compete for making an osr nmethod not-entrant:
>>>>>>
>>>>>> Thread 1: Invalidates the osr nmethod and blocks just before executing the assert in line 1397.
>>>>>> Thread 2: nmethod is already invalidated. Acquires patching lock and sets the nmethod to not-entrant.
>>>>>> Thread 1: Continues execution and hits the assert because the nmethod was on the list and is (now) not-entrant.
>>>>>>
>>>>>> Thread 1 should ignore this and return. See also comment in nmethod::make_not_entrant_or_zombie():
>>>>>>      // another thread already performed this transition so nothing
>>>>>>      // to do, but return false to indicate this.
>>>>>>
>>>>>> I changed the verification code to check for osr invalidation *after* the state of the nmethod was set.
>>>>>>
>>>>>> Tested with failing test, JPRT and RBT (in progress).
>>>>>>
>>>>>> Thanks,
>>>>>> Tobias
>>>>>