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

[Vladimir Ivanov]

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.

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