[9] RFR (S): 8060147: SIGSEGV in Metadata::mark_on_stack() while marking metadata in ciEnv

[Mikael Gerdin]

Vladimir,

On 2014-10-30 11:27, Vladimir Ivanov wrote:
> Mikael, Coleen,
>
> Thanks for the feedback!
>
> As I wrote to Roland, it seems keeping klass_holder for every
> ciInstanceKlass for VM anonymous classes should solve the problem I
> encountered. It doesn't cover ciMethodData though.

Ok, I think using klass_holder should be fine for normal classes as 
well, since klass_holder returns the class loader oop in that case.

Does it need to cover ciMethodData? Since MethodData is allocated in the 
ClassLoader's (or VM anon class) metaspace it should have the same 
liveness as the Method, and the InstanceKlass and all the other metadata.

Or does a compilation reference MethodData objects for other methods, 
for example when inlining? In that case you need to be sure that we 
create a ciInstanceKlass for the method holder of the inlined method.

/Mikael

>
> I can narrow the logic to that cases only. What are your thoughts?
>
> Best regards,
> Vladimir Ivanov
>
> On 10/30/14, 12:18 PM, Mikael Gerdin wrote:
>>
>> On 2014-10-29 17:35, Coleen Phillimore wrote:
>>>
>>> On 10/29/14, 12:25 PM, Mikael Gerdin wrote:
>>>> Hi Vladimir,
>>>>
>>>> On 2014-10-29 14:57, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/8060147/webrev.00
>>>>> https://bugs.openjdk.java.net/browse/JDK-8060147
>>>>>
>>>>> ciObjectFactory doesn't keep cached Metadata* alive. ciMetadata is
>>>>> different from ciObject - it doesn't store JNI handle, but a raw
>>>>> Metadata* pointer.
>>>>>
>>>>> In order to avoid Metadata* vanishing during compilation, I cache
>>>>> ciObject for it's holder, which should keep corresponding Metadata*
>>>>> alive. Cached objects have the same lifetime as the owning
>>>>> ciObjectFactory (no cache pruning), so Metadata* will be available as
>>>>> long as ciObjectFactory instance is used.
>>>>>
>>>>> Also, cleaned relevant comments and strengthened some asserts (since
>>>>> NULL keys are not allowed).
>>>>>
>>>>> Testing: jprt, stress mode - full Nashorn/Octane with aggressive VM
>>>>> anonymous class unloading (LambdaForm/MethodHandle caching completely
>>>>> turned off).
>>>>
>>>> Have you tried this with G1?
>>>>
>>>> I'm asking because G1's concurrent marking algorithm behaves slightly
>>>> different to the other gcs, and I'm unsure if you need to also need to
>>>> call G1SATBCardTableModRefBS::enqueue (like ensure_metadata_alive
>>>> does).
>>>
>>> ensure_metadata_alive doesn't handle MethodData.   Should it?
>>
>> I don't think so.
>> Currently ensure_metadata_alive is needed because metadata pointers
>> embedded in MDOs are converted from "weak" to "strong" references when a
>> MDO is imported into a compilation and its metadata pointers are wrapped
>> in ci objects.
>>
>> At that point the metadata may in fact be otherwise dead, and G1's SATB
>> marking algorithm will correctly determine these to be not alive. This
>> is similar to other read barrier's we've inserted in different places
>> where weak pointers are promoted to strong when running with G1, see
>> java.lang.ref.Reference#get, StringTable, etc.
>>
>>>>
>>>> When implementing class unloading for G1 we saw some similar crashes
>>>> and I think I redeemed them by calling ensure_metadata_alive for
>>>> Metadata retrieved from the MDO's (since MDO's are in fact "weak"
>>>> Metadata references).
>>>>
>>>> Essentially, since we have a compilation executing I thought that the
>>>> reciever of the methods compiled must all be alive, and if the
>>>> receivers are alive then they should keep all the holders alive.
>>>>
>>>> ciInstanceKlass::ciInstanceKlass creates a JNIHandle to the
>>>> class_loader() of an InstanceKlass being wrapped in the CI, perhaps
>>>> that should be changed to klass_holder() to account for anonymous
>>>> classes?
>>>
>>> This is probably unnecessary now with Vladimir's change and why it
>>> didn't crash sooner.
>>
>> Indeed. I sort of see what Vladimir is suggesting as a sledgehammer
>> approach, picking up the holder for every referenced metadata and
>> "interning" it into the ciObjectFactory.
>>
>> What I'm interested to see is if changing to klass_holder() in the
>> ciInstanceKlass is sufficient to solve the current issue because I want
>> to know exactly in what way the compiler needs to access metadata and
>> why it needs otherwise dead metadata.
>>
>> In the end it may be wise to implement Vladimir's suggested change to be
>> sure that we don't crash because we missed something. It depends on if
>> we want to be exact in keeping track of which objects we actually need
>> or if we wrap all the ones we can think of in handles and hope for the
>> best.
>>
>> /Mikael
>>
>>>
>>> Coleen
>>>>
>>>> /Mikael
>>>>
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>