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

[Coleen Phillimore]

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.

>
>> I'm sorry that I didn't get to my email today but from the discussion I
>> think changing two occurrences of "class_loader" in
>> ciInstanceKlass::ciInstanceKlass to "klass_holder" would have solved
>> your problem.
> I don't think that's what we want. For VM anon classes, _loader == 
> NULL, but if we place java_mirror there instead, it could cause 
> problems in other parts of VM, since non-NULL _loader value implicates 
> ClassLoader instance. Not sure all these places are guarded against 
> seeing VM anon classes.

I thought that field was only added to hold the class_loader as a holder 
but if you think using mirror would cause problems, it seems like a 
reason to not do this.

I reviewed this code.

Coleen

>
>
>> Unless, you can add a ciMethod or ciMethodData without adding a
>> ciInstanceKlass (which I don't think you can).
> It's not possible right now. But ciObjectFactory doesn't forbid that.
>
>> I think Roland pointed out a flaw though that you can safepoint before
>> adding a ciInstanceKlass though, which you could fix by moving this up
>> in ciMethod::ciMethod to before the safepoint.
>>
>>    _holder = env->get_instance_klass(h_m()->method_holder());
> I simply pass _holder value into the ciMethod ctor.
>
> Best regards,
> Vladimir Ivanov
>
>> I know I suggested adding the ciObject in ciMetadata but that's because
>> this is done somewhere that is hard to find.  A good comment that this
>> is what keeps metadata that ci points to from being unloaded by GC would
>> help a lot with that.
>
>
>>
>> Thanks,
>> Coleen
>>
>>
>> On 10/30/2014 02:06 PM, Vladimir Kozlov wrote:
>>> I would go with webrev.01 (updated initial version).
>>>
>>> Regards,
>>> Vladimir
>>>
>>> On 10/30/14 7:55 AM, Vladimir Ivanov wrote:
>>>>>> As a solution, _holder can be passed into ciMethod::ciMethod as a
>>>>>> parameter. It should fix the problem.
>>>>>
>>>>> The first change you suggested
>>>>> (http://cr.openjdk.java.net/~vlivanov/8060147/webrev.00) would fix 
>>>>> the
>>>>> ciMethod::ciMethod problem, right? The code would be more robust that
>>>>> way and other similar issues could be avoided.
>>>> Yes, initial version fixes ciMethod::ciMethod problem. It's also more
>>>> robust and easier to reason about.
>>>>
>>>> The downside is that for every ciMetadata instantiation we do more 
>>>> work.
>>>>
>>>> I have an alternative version:
>>>> http://cr.openjdk.java.net/~vlivanov/8060147/webrev.anon.00/
>>>>
>>>> Initial version (updated):
>>>> http://cr.openjdk.java.net/~vlivanov/8060147/webrev.01/
>>>>
>>>> I like initial version more, but I don't have strong opinion here.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>