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

[Vladimir Kozlov]

Looks like nmethod::verify_metadata_loaders(..) does check embedded metadata - CheckClass::do_check_class() call. So we 
will catch dead metadata in debug VM.

Okay, your latest version looks fine.

Thanks,
Vladimir

On 7/16/14 4:04 AM, Tobias Hartmann wrote:
> Hi Vladimir,
>
> On 15.07.2014 19:59, Vladimir Kozlov wrote:
>> On 7/15/14 12:48 AM, Tobias Hartmann wrote:
>>> Hi Vladimir,
>>>
>>>> Impressive work, Tobias!
>>>
>>> Thanks! Took me a while to figure out what's happening.
>>>
>>>> So before the permgen removal embedded method* were oops and they were
>>>> processed in relocInfo::oop_type loop.
>>>
>>> Okay, good to know. That explains why the terms oops and metadata are
>>> used interchangeably at some points in the code.
>>>
>>>> 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)?
>>>
>>> The problem with iterating over relocInfo::metadata_type is that we
>>> don't know to which stub, i.e., to which IC the Method* pointer belongs.
>>> Since we don't want to unload the entire method but only clear the
>>> corresponding IC, we need this information.
>>
>> Got it: you are cleaning call site IC: ic->set_to_clean().
>>
>> My point was these to_interp stubs are part of a nmethod (they are in stubs section) and contain dead metadata. Should
>> we unload this nmethod then? We do unloading nmethods if any embedded oops are dead (see can_unload()). Should we do
>> the same if a nmethod (and its stubs) have dead metadata? Note, embedded metadata could be Method* and MethodData*.
>
> I'm not sure but I think except for the to-interpreter stub the nmethod is still valid and could be executed. In
> nmethod::verify_metadata_loaders(..) we explicitly check for this case and clean such stubs prior to verification.
> Assuming that this code was added for a reason, it means that nmethods with a to-interpreter stub referencing stale
> metadata do not necessarily have to be unloaded.
>
> But maybe someone else knows better.
>
> Thanks,
> Tobias
>
>>
>> Vladimir
>>
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>
>>>> 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
>>>
>