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

[Vladimir Ivanov]

Vladimir, Coleen, Roland, Mikael, thanks for reviews!

On 11/6/14, 1:51 AM, Vladimir Kozlov wrote:
> I am fine with targeted fix only.
>
> One comment env->get_instance_klass() checks for NULL. Your new code in
> create_new_metadata() does not:
>
> ciInstanceKlass* holder =
> get_metadata(h_m()->method_holder())->as_instance_klass();
Good catch. I reverted to ciEnv::get_instance_klass().

FTR updated webrev:
http://cr.openjdk.java.net/~vlivanov/8060147/webrev.02

Best regards,
Vladimir Ivanov

>
> Thanks,
> Vladimir K
>
> On 11/5/14 9:02 AM, Vladimir Ivanov wrote:
>>
>> On 11/5/14, 9:33 PM, Coleen Phillimore wrote:
>>>
>>> On 10/30/14, 4:32 PM, Vladimir Ivanov wrote:
>>>> Coleen,
>>>>
>>>> I implemented 2 approaches of the fix.
>>>>
>>>> The fix with a special case for VM anon classes is:
>>>> http://cr.openjdk.java.net/~vlivanov/8060147/webrev.anon.00/
>>>>
>>>> Both fix the bug, but have different properties.
>>>>
>>>> (1) Special case for VM anon class is very focused on the actual
>>>> cause, but more fragile - all the logic which keeps metadata from
>>>> being deallocated is non-trivial and scattered around the whole
>>>> ciMetadata hierarchy.
>>>>
>>>> (2) On the other hand, initial version, which forcibly creates
>>>> klass_holder ciObject for each ciMetadata, is much cleaner and
>>>> localized, but does unnecessary work.
>>>>
>>>> Am I right that you prefer (1) as a fix?
>>>
>>> Yes, I think this version does less unnecessary work and creates less
>>> ciObjects.   And the comment is useful for finding how we keep
>>> ciMetadata alive for anonymous classes.   You still have a UseNewCode in
>>> the webrev thought that you want to take out.
>>
>> Thanks, Coleen.
>>
>> VladimirK, Roland, what do you think about (1)?
>>
>> Best regards,
>> Vladimir Ivanov