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

[Mikael Gerdin]

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
>