[9] RFR(S): 8153134: Infinite loop in handle_wrong_method in jmod

[Tobias Hartmann]

Thanks, Vladimir and Dean!

Best regards,
Tobias

On 13.10.2016 06:15, dean.long at oracle.com wrote:
> +1
> 
> dl
> 
> 
> On 10/12/16 10:55 AM, Vladimir Kozlov wrote:
>> Looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/12/16 12:04 AM, Tobias Hartmann wrote:
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8153134
>>> http://cr.openjdk.java.net/~thartmann/8153134/webrev.01/
>>>
>>> There is a race between Method::set_code() and Method::clear_code() that may cause a Method to end up in an inconsistent state. As a result, we loop forever in handle_wrong_method() trying to resolve a call to this Method.
>>>
>>> In more detail, we have method A that calls method B and the following threads:
>>> - Thread T1: CompilerThread that is compiling method B and currently registers the nmethodB.
>>> - Thread T2: Currently executes nmethodA.
>>>
>>> Here is the execution sequence:
>>> T1: Registers nmethodB in Method::set_code():
>>>     _code = nmethodB
>>> T2: Resolves call to method B in CompiledIC::compute_monomorphic_entry:
>>>     Since Method::_code is set, entry is set to nmethodB::_verified_entry_point
>>> T2: nmethodB is converted to zombie in nmethodB::make_not_entrant_or_zombie():
>>>     The verified entry is patched to jump to handle_wrong_method_stub() and Method::clear_code() is called:
>>>     _from_compiled_entry = c2i
>>>     _from_interpreted_entry = i2i
>>> T1: Continues in Method::set_code():
>>>     _from_compiled_entry = _verified_entry_point
>>>     _from_interpreted_entry = i2c
>>> T2: Continues in Method::clear_code():
>>>     _code = NULL
>>>
>>> Method B ends up with:
>>> _from_compiled_entry = _verified_entry_point
>>> _from_interpreted_entry = i2c
>>> _code = NULL
>>>
>>> As a result, handle_wrong_method() returns _from_compiled_entry == _verified_entry_point which was patched to jump to handle_wrong_method() again. We loop forever.
>>>
>>> Unfortunately, I was not able to reproduce the problem but got all the information from the core file/logs. As Dean suggested, I used the Patching_lock to synchronize access to these fields. I removed the call to clear_code() in classLoader.cpp because make_not_entrant() includes clear_code().
>>>
>>> I verified that the additional locking does not affect performance. I also did correctness testing with RBT.
>>>
>>> Thanks,
>>> Tobias
>>>
>