[9] RFR(S): 8058737: CodeCache::find_blob fails with 'unsafe access to zombie method'

[Vladimir Kozlov]

On 9/24/14 5:24 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> thanks for the review.
>
> On 23.09.2014 23:05, Vladimir Kozlov wrote:
>> Why it is on confidential list?
>
> I originally set the bug to confidential because some of the failing tests are closed. Should I change it to open?

Changes are in open code and they should be discussed in open. I changed mailing list.

About ICStub.
new_ic_stub() creates empty ICStub, the initialized with IC data is previous stub:
ic_stub->set_stub(ic, cached_value, entry);

So when first safepoint happens interesting ICStub is not empty. My question is why it is not cleared during first 
safepoint? Is nmethod::cleanup_inline_caches() called during first safepoint again or not? I mean from 
CodeCache::gc_epilogue(). If it is not called again then it explains the situation.

Thanks,
Vladimir

>
>> On 9/23/14 6:37 AM, Tobias Hartmann wrote:
>>> Igor, Vladimir, thanks for the reviews!
>>>
>>> Please see comments inline.
>>>
>>> On 22.09.2014 23:47, Igor Veresov wrote:
>>>> Could in theory the blob be deallocated by the time we look at it in
>>>> ICStub::finalize?
>>>
>>> Yes, that could happen. As Vladimir mentioned, more than one traversal
>>> of the sweeper may happen, changing the state of the nmethod from zombie
>>> to unloaded and deallocating it.
>>
>> Tobias, just to be clear. The state 'zombie' does not converted to state 'unloaded'. The 'unloaded' state is set only
>> in nmethod::do_unloading() when we unload classes. And 'unloaded' state is converted to 'zombie'. This is the only
>> direction of a state change.
>
> Yes, thanks. I meant to say that the zombie method may be unloaded during another traversal of the sweeper.
>
>> On 23.09.2014 03:12, Vladimir Kozlov wrote:
>>>> I think safer solution will be to cleanup corresponding ICStub when
>>>> nmethod is converted to zombie. Otherwise you will get incorrect
>>>> result as Igor pointed when nmethod is removed - we can get several
>>>> sweeps before second 6) sefapoint.
>>>
>>> Yes, I agree. I changed the code to clear ICStubs when the nmethod is
>>> converted to zombie. It should be sufficient to do this for the
>>> non-entrant->zombie transition in the sweeper. The other calls to
>>> 'nmethod::make_zombie' either happen at a safepoint (see
>>> 'CodeCache::make_marked_nmethods_zombies') or for the unloaded->zombie
>>> transition in the sweeper that should be safe.
>>>
>>> New webrev: http://cr.openjdk.java.net/~thartmann/8058737/webrev.01/
>>
>> One thing is still not clear for me. Please, explain why during the first safepoint which should clean StubQueue that
>> ICStub was not cleaned? We at safepoint and it is safe to cleanr it:
>>
>> nmethod::cleanup_inline_caches() {
>> ...
>>           if (!nm->is_in_use() || (nm->method()->code() != nm)) ic->set_to_clean();
>>
>> CompiledIC::set_to_clean() {
>> ...
>>   if (safe_transition) {
>>     // Kill any leftover stub we might have too
>>     if (is_in_transition_state()) {
>>       ICStub* old_stub = ICStub_from_destination_address(stub_address());
>>       old_stub->clear();
>>     }
>
> The problem is that the first safepoint happens while processing a non-entrant nmethod in the sweeper: The sweeper tries
> to cleanup the inline caches of this nmethod but fails while creating the transition stub because there is no space in
> the StubQueue. A safepoint is forced to free up space by back-patching and removing ICStubs.
>
> VM_ForceSafepoint vfs;
> InlineCacheBuffer::new_ic_stub()
> InlineCacheBuffer::create_transition_stub()
> CompiledIC::set_to_clean()
> nmethod::cleanup_inline_caches()
> NMethodSweeper::process_nmethod() line 557
>
> That means right after the safepoint completes, a new ICStub is created and the sweeper continues. As a side effect, the
> safe point also performs stack scanning and the nmethod is not found on the stack. During the next iteration of the
> sweeper the nmethod is therefore converted to zombie. Note that there is no safepoint between the actual creation of the
> ICStub and the conversion to zombie!
>
> At the next safepoint, back-patching of ICStubs is performed again and fails because the nmethod is in zombie state:
>
> CodeCache::find_nmethod(void*)
> ICStub::finalize()
> StubQueue::remove_all()
> InlineCacheBuffer::update_inline_caches()
> SafepointSynchronize::do_cleanup_tasks()
>
> Does that answer your question?
>
> Thanks,
> Tobias
>
>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> Tobias
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 9/22/14 6:08 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8058737
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8058737/webrev.00/
>>>>>
>>>>> == Problem ==
>>>>> The test closed/compiler/6440479/VirtualDispatch.java contains a method
>>>>> 'test' with many virtual calls that change the target during execution.
>>>>> Compilation of this method results in a high number of inline caches
>>>>> being created for the virtual calls.
>>>>>
>>>>> List of events that cause the bug:
>>>>> 1) nmethod [1] is converted to non-entrant.
>>>>> 2) The nmethod is processed by the sweeper. It is still active on the
>>>>> stack and can therefore not yet be converted to zombie. However, the
>>>>> sweeper cleans the inline caches of [1] (see line 553 of
>>>>> 'NMethodSweeper::process_nmethod'). CompiledIC::set_to_clean() creates a
>>>>> transition stub because a mt safe transition to the clean state is not
>>>>> possible (see 'InlineCacheBuffer::create_transition_stub'). Because the
>>>>> StubQueue is full, 'InlineCacheBuffer::new_ic_stub' forces a safepoint
>>>>> to back patch IC stubs and free space in the StubQueue.
>>>>> 3) The safepoint also performs stack scanning and [1] is not found on
>>>>> the stack.
>>>>> 4) 'nmethod::cleanup_inline_caches' continues and finishes.
>>>>> 5) The next traversal of the sweeper converts [1] to zombie (it was not
>>>>> found on the stack during the forced safepoint).
>>>>> 6) A safepoint occurs updating the inline caches of [1].
>>>>> 'ICStub::finalize' tries to patch the IC but fails because the nmethod
>>>>> is a zombie (guarantee in 'CodeCache::find_blob'). See [2].
>>>>>
>>>>> Summary:
>>>>> Usually, zombie nmethods do _not_ have an IC stub because the transition
>>>>> from non-entrant to zombie only happens after a safepoint that removes
>>>>> all IC stubs by back patching the corresponding ICs. In this case, a
>>>>> safepoint is triggered _while_ creating an IC stub and this safepoint
>>>>> also performs stack scanning allowing the nmethod to transition to
>>>>> zombie while having an IC stub.
>>>>>
>>>>> == Solution ==
>>>>> Use CodeCache::find_blob_unsafe in ICStub::finalize and only back patch
>>>>> the IC if the nmethod is not a zombie.
>>>>>
>>>>> Maybe we should also think about increasing the size of the StubQueue.
>>>>> Currently it is set to 10*K (see 'InlineCacheBuffer::initialize').
>>>>>
>>>>> == Testing ==
>>>>> - Failing test
>>>>> - JPRT
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> [1] 'VirtualDispatch::test'
>>>>> [2] Stack trace:
>>>>> CodeCache::find_nmethod(void*)
>>>>> ICStub::finalize()
>>>>> StubQueue::remove_all()
>>>>> InlineCacheBuffer::update_inline_caches()
>>>>> SafepointSynchronize::do_cleanup_tasks()
>>>>>
>>>
>