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

[Tobias Hartmann]

Vladimir, Coleen, thanks for the reviews!

On 18.07.2014 20:09, Vladimir Kozlov wrote:
> On 7/18/14 11:02 AM, Coleen Phillimore wrote:
>>
>> On 7/18/14, 11:06 AM, Vladimir Kozlov wrote:
>>> 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)?
>>
>> CompressedOops when off turns off CompressedClassPointers.
>
> You are right, I forgot that. Still the question is why switch off coop?

I'm only able to reproduce the bug without compressed oops. The original 
bug also only reproduces with -XX:-UseCompressedOops. I tried to figure 
out why (on Sparc):

With compressed oops enabled, Method* metadata referencing 'WorkerClass' 
is added to 'doWork' in MacroAssembler::set_narrow_klass(..). In 
CodeBuffer::finalize_oop_references(..) the metadata is processed and an 
oop to the class loader 'URLClassLoader' is added. This oop leads to the 
unloading of 'doWork', hence the verification code is never executed.

I'm not sure what set_narrow_klass(..) is used for in this case. I 
assume it stores a 'WorkerClass' Klass* in a register as part of an 
optimization? Because 'doWork' potentially works on any class. 
Apparently this optimization is not performed without compressed oops.

Best,
Tobias

>
> Vladimir
>
>>>
>>>>
>>>> http://cr.openjdk.java.net/~thartmann/8029443/webrev.02/
>>>
>>> Very nice!
>>
>> Yes, I agree.  Impressive.
>>
>> The refactoring in nmethod.cpp looks good to me.  I have no further
>> comments.
>> Thanks!
>> Coleen
>>
>>>
>>>>
>>>> @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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>