6354181 and the SystemDictionary

[Tom Rodriguez]

On Jan 26, 2011, at 1:47 PM, Karen Kinnear wrote:

> Tom,
> 
> So, yes, a class is not added to the SystemDictionary until it is loaded (define_instance_class calls
> add_to_hierarchy which sets_init_state to loaded before update_dictionary. So a find() call will return
> with a state >= loaded. This is all done at define class time by the defining loader.
> 
> And yes, a klass is added to the loader constraints potentially long before it is loaded. Loader constraints
> allow us to lazily ensure that any referenced class loaded by any of its initiating loaders will always
> reference the same underlying class object as if it were loaded by its defining loader. Basically if
> class A method X calls class B method Y, A's class loader and B's class loader must agree on
> Y's parameters and return value. So this can happen when linking A which can be long before
> B gets loaded.
> 
> So I have only walked through the find_constrained_instance_or_array_klass once and I
> believe it is only called in ciEnv::get_klass_by_name_impl, and only if "require_local" is false.
> I was unable to figure out the real meaning behind require_local.

It controls whether to restrict the lookup to klasses which can be reached directly through the constant pool.  If it's false then we try to find the klass in a symbolic fashion.  This helps deal with types that haven't be resolved yet or types which are never referenced from the constant pool, like the types in the method signature.

> 
> find_constrained_instance_or_array_klass first does the find_instance_or_array_klass that is
> a SystemDictionary lookup, so that should cover loaded classes. I'm wondering if the point is
> to find classes for which the actual class is loaded by the defining loader, but you haven't yet
> gone through the exercise of loading the class for the current, i.e. a different initiating loader,
> and the compiler doesn't/can't initiate loading since that might require calling out to java and
> the compiler thread can't do that... So instead we look to see if we already know due to
> constraints what actual instance or array class this has to resolve to.  If that is what we are doing,
> then it makes sense to look in the loader constraints for this initiating loader/class name and
> if the class is loaded go ahead and use it.
> 
> So I think the modification would be in find_constrained_instance_or_array_klass to only return
> classes in the loaded state, or arrays for which the element class is in the loaded state,
> as you suggest.
> 
> hope this helps,
> Karen

Yes that was helpful.  I'll go with the fix in find_constrained_instance_or_array_klass.

tom

> 
> On Jan 26, 2011, at 4:09 PM, Tom Rodriguez wrote:
> 
>> I've been debugging 6354181 where we attempt to initialize a class which is only in the allocated state and die.  After heading off into the weeds for a while I discovered that the problem is that the compiler is finding an instanceKlass which is only in the allocated state instead of in the loaded state and generating code using that klass.  We manage to run the code before the klass has been moved to the loaded state and we end up dying.
>> 
>> I can see several ways to guard against this but I can't decide whether this is something that should be guarded against within SystemDictionary itself or whether users of SystemDictionary should be guarding against it.  It appears that we're finding the class through   SystemDictionary::find_constrained_instance_or_array_klass by going through the loader constraints.  I'm assuming that the klass gets added to the loader constraints early and then gets added into the dictionary proper and marked as loaded which creates the window where things can go wrong.  I also assume that other interfaces that perform lookups would never return a class which was still in the allocated state.
>> 
>> It isn't that hard to reproduce this failure but it's very hard to capture a snap shot of exactly how the dictionary is being manipulated at the time we find the klass which leaves me a little unsure how many places really need to guard against this.  Does this seem plausible to anyone who understands the SystemDictionary?  I believe the proper fix is to add a  guard to SystemDictionary::find_constrained_instance_or_array_klass to only return classes which are really in the loaded state.
>> 
>> Part of the problem here is that ciObject has a base method is_loaded() which returns true if the ciObject is backed by a real Java object.  This means that is_loaded on a ciInstanceKlass doesn't mean the same thing as instanceKlass::is_loaded unless we disallow the allocated state completely.  I could simply guard against it in the CI but that doesn't really feel right.  I will definitely put some asserts about in there though since that would have made the bug much more obvious.
>> 
>> tom
>