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

[Vladimir Ivanov]

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?

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

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