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

[Mikael Gerdin]

Vladimir,

On 2014-11-10 14:01, Vladimir Ivanov wrote:
> 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

Looks good.

/Mikael

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