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

[Vladimir Kozlov]

On 7/18/14 4:38 AM, Tobias Hartmann wrote:
> Hi,
>
> I spend some more days and was finally able to implement a test that deterministically triggers the bug:

Why do you need to switch off compressed oops? Do you need to switch off compressed klass pointers too 
(UseCompressedClassPointers)?

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

Very nice!

>
> @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.

Make sense.

>
> 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,
Vladimir

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