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

Wed Sep 24 20:21:49 UTC 2014

On 9/24/14 10:13 AM, Vladimir Kozlov wrote:
> 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.

Tobias, your changes are good so you can push them. What I am asking is to get clear picture of this code for future 
reference.

Thanks,
Vladimir

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