RFR (XS): 8023037 : Race between ciEnv::register_method and nmethod::make_not_entrant_or_zombie

[Vladimir Kozlov]

Vladimir,

Thank you for finding the real cause.

I think you need the comment before nm->verify() in new_nmethod() 
explaining why we should avoid safepoint as you explained here.

I am not comfortable about passing new allow_safepoints parameter 
through all verify calls because it is the only one case. We still have 
not installed nmethod at this point. There should be some check 
(nmethod's flags/state) which we can use to skip CompiledIC_lock.

I thought about the need to verify scopes in verify_interrupt_point() in 
all cases. But the verification code in ScopeDesc::verify() is 
commented. What? In general it is good to call it anyway. And you don't 
need IC to get the end_of_call() - I think we can inline what 
CompiledIC_at() does: nativeCall_at(call_site)->end_of_call().

This code calls CompiledIC_at() because it does IC verification which we 
can skip in our case.

Thanks,
Vladimir

On 11/6/13 6:57 AM, Vladimir Ivanov wrote:
> Very good point, Vladimir!
>
> I should have looked for the bug in a different place :-)
>
> The problem is triggered by class redefinition and it can be performed
> under Compile_lock or during safepoint. So, in ciEnv::register_method
> it's not enough to grab Compile_lock. In addition, we need to ensure
> that there are no safepoints possible during method installation to
> guarantee no dependency is invalidated.
>
> There's one place in nmethod verification code (see MutexLocker
> ml_verify (CompiledIC_lock) in nmethod::verify_interrupt_point) where
> safepoint is possible and it causes the failure.
>
> The bug is present only in non-product builds since nmethod verification
> is absent in product bits.
>
> Previous fix also works, but I decided to go another route and forbid
> safepoints at all while holding Compile_lock.
>
> Updated webrev (will appear once cr.ojn is up):
> http://cr.openjdk.java.net/~vlivanov/8023037/webrev.02/
>
> Best regards,
> Vladimir Ivanov
>
> On 11/6/13 2:36 AM, Vladimir Kozlov wrote:
>> You need to be careful to avoid deadlock since there is call:
>>
>> 1035               old->make_not_entrant();
>>
>> Any way there is comment in register_method():
>>
>>   936     // Prevent SystemDictionary::add_to_hierarchy from running
>>   937     // and invalidating our dependencies until we install this
>> method.
>>   938     MutexLocker ml(Compile_lock);
>>
>> So how it happens that the method is invalidated by dependencies?
>>
>> Thanks,
>> Vladimir
>>
>> On 11/5/13 1:52 PM, Vladimir Ivanov wrote:
>>> Ouch, I was misled by it's name. So, I just lowered the likelihood of
>>> the failure then.
>>>
>>> make_not_entrant_or_zombie holds Patching_lock when it patches & unlinks
>>> a nmethod.
>>>
>>> Do you see any problems with using it to guard method installation
>>> (method->set_code(method, nm)) in register_method() as well?
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 11/6/13 1:11 AM, Vladimir Kozlov wrote:
>>>> Note nmethodLocker is not lock:
>>>>
>>>> void nmethodLocker::lock_nmethod(nmethod* nm, bool zombie_ok) {
>>>>    if (nm == NULL)  return;
>>>>    Atomic::inc(&nm->_lock_count);
>>>>    guarantee(zombie_ok || !nm->is_zombie(), "cannot lock a zombie
>>>> method");
>>>> }
>>>>
>>>> The guard is nm->is_locked_by_vm() but neither
>>>> make_not_entrant_or_zombie () or register_method() use it.
>>>>
>>>> So how nmethodLocker helps here?
>>>>
>>>> Vladimir
>>>>
>>>> On 11/5/13 12:42 PM, Vladimir Ivanov wrote:
>>>>> Thanks! Missed that one. Fixed.
>>>>>
>>>>> http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> On 11/5/13 11:25 PM, Vladimir Kozlov wrote:
>>>>>> Should be
>>>>>>
>>>>>> http://cr.openjdk.java.net/~vlivanov/8023037/webrev.01/
>>>>>>
>>>>>> What about this place:
>>>>>>
>>>>>> src/cpu/sparc/vm/sharedRuntime_sparc.cpp:  if (StressNonEntrant) {
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> On 11/5/13 11:10 AM, Vladimir Ivanov wrote:
>>>>>>> Updated fix:
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/
>>>>>>>
>>>>>>> Removed broken StressNonEntrant code.
>>>>>>> Updated comments.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>> On 11/5/13 3:39 PM, Vladimir Ivanov wrote:
>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8023037/webrev.00/
>>>>>>>>
>>>>>>>> There's a race between compiler thread during method registration
>>>>>>>> and
>>>>>>>> sweeper: sweeper can invalidate a nmethod which hasn't been
>>>>>>>> installed
>>>>>>>> yet.
>>>>>>>>
>>>>>>>> Consider the following scenario:
>>>>>>>>    ciEnv::register_method:
>>>>>>>>      - new nmethod(...)
>>>>>>>>
>>>>>>>>    sweeper:
>>>>>>>>      - invalidates newly allocated nmethod and patches VEP to call
>>>>>>>> handle_wrong_method
>>>>>>>>      - tries to unlink it, but method()->from_compiled_entry() !=
>>>>>>>> verified_entry_point() since nmethod hasn't been installed yet
>>>>>>>>
>>>>>>>>    ciEnv::register_method:
>>>>>>>>      - installs already invalidated nmethod
>>>>>>>>
>>>>>>>> Calling corresponding Java method will lead to infinite loop:
>>>>>>>> VEP of
>>>>>>>> the
>>>>>>>> compiled method calls handle_wrong_method and call site resolution
>>>>>>>> returns the very same compiled method.
>>>>>>>>
>>>>>>>> The fix is to grab a lock after nmethod is allocated in the code
>>>>>>>> cache
>>>>>>>> and check that it hasn't been already invalidated by the sweeper
>>>>>>>> before
>>>>>>>> proceeding. Sweeper already synchronizes on a nmethod before
>>>>>>>> invalidating it.
>>>>>>>>
>>>>>>>> Testing: failing test w/ diagnostic output.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov