[9] RFR(S): 8029443: 'assert(klass->is_loader_alive(_is_alive)) failed: must be alive' during VM_CollectForMetadataAllocation

[Tobias Hartmann]

Hi,

I spend some more days and was finally able to implement a test that 
deterministically triggers the bug:

http://cr.openjdk.java.net/~thartmann/8029443/webrev.02/

@Vladimir: The test shows why we should only clean the ICs but not 
unload the nmethod if possible. The method ' doWork' is still valid 
after WorkerClass was unloaded and depending on the complexity of the 
method we should avoid unloading it.

On Sparc my patch fixes the bug and leads to the nmethod not being 
unloaded. The compiled version is therefore used even after WorkerClass 
is unloaded.

On x86 the nmethod is unloaded anyway because of a dead oop. This is 
probably due to a slightly different implementation of the ICs. I'll 
have a closer look to see if we can improve that.

Thanks,
Tobias

On 16.07.2014 10:36, Tobias Hartmann wrote:
> Sorry, forgot to answer this question:
>> Were you able to create a small test case for it that would be useful 
>> to add?
> Unfortunately I was not able to create a test. The bug only reproduces 
> on a particular system with a > 30 minute run of runThese.
>
> Best,
> Tobias
>
> On 16.07.2014 09:54, Tobias Hartmann wrote:
>> Hi Coleen,
>>
>> thanks for the review.
>>> *+           if (csc->is_call_to_interpreted() && 
>>> stub_contains_dead_metadata(is_alive, csc->destination())) {*
>>> *+             csc->set_to_clean();*
>>> *+           }*
>>>
>>> This appears in each case.  Can you fold it and the new function 
>>> into a function like clean_call_to_interpreted_stub(is_alive, csc)?
>>
>> I folded it into the function clean_call_to_interpreter_stub(..).
>>
>> New webrev: http://cr.openjdk.java.net/~thartmann/8029443/webrev.01/
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>>
>>>> So before the permgen removal embedded method* were oops and they 
>>>> were processed in relocInfo::oop_type loop.
>>>>
>>>> May be instead of specializing opt_virtual_call_type and 
>>>> static_call_type call site you can simple add a loop for 
>>>> relocInfo::metadata_type (similar to oop_type loop)?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/14/14 4:56 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>>
>>>>> please review the following patch for JDK-8029443.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8029443
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8029443/webrev.00/
>>>>>
>>>>> *Problem*
>>>>> After the tracing/marking phase of GC, nmethod::do_unloading(..) 
>>>>> checks
>>>>> if a nmethod can be unloaded because it contains dead oops. If class
>>>>> unloading occurred we additionally clear all ICs where the cached
>>>>> metadata refers to an unloaded klass or method. If the nmethod is not
>>>>> unloaded, nmethod::verify_metadata_loaders(..) finally checks if all
>>>>> metadata is alive. The assert in CheckClass::check_class fails 
>>>>> because
>>>>> the nmethod contains Method* metadata corresponding to a dead Klass.
>>>>> The Method* belongs to a to-interpreter stub [1] of an optimized
>>>>> compiled IC. Normally we clear those stubs prior to verification to
>>>>> avoid dangling references to Method* [2], but only if the stub is 
>>>>> not in
>>>>> use, i.e. if the IC is not in to-interpreted mode. In this case the
>>>>> to-interpreter stub may be executed and hand a stale Method* to the
>>>>> interpreter.
>>>>>
>>>>> *Solution
>>>>> *The implementation of nmethod::do_unloading(..) is changed to clean
>>>>> compiled ICs and compiled static calls if they call into a
>>>>> to-interpreter stub that references dead Method* metadata.
>>>>>
>>>>> The patch was affected by the G1 class unloading changes 
>>>>> (JDK-8048248)
>>>>> because the method nmethod::do_unloading_parallel(..) was added. I
>>>>> adapted the implementation as well.
>>>>> *
>>>>> Testing
>>>>> *Failing test (runThese)
>>>>> JPRT
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> [1] see CompiledStaticCall::emit_to_interp_stub(..)
>>>>> [2] see nmethod::verify_metadata_loaders(..),
>>>>> static_stub_reloc()->clear_inline_cache() clears the stub
>>>
>>
>