RFR (S) 8197844: JVMTI GetLoadedClasses should use the Access API

[Erik Österlund]

Hi Coleen,

I for one would like to see your version of the fix, plus some suitable 
assert to catch bad use of these APIs.
The motivation is that it seems unfortunate to expose APIs for getting 
Metadata that when retrieved is unstable and unsafe to use unless you 
have deep knowledge about GC races and class unloading, and hope all 
callsites know that they can't just use the things they retrieved 
because they are dangerously explosive. The only problem with the more 
conservative API is if the GC itself calls into the code and 
accidentally keeps everything alive preventing class unloading. But I 
think a suitable assert could catch that.

I'm not 100% sure what the assert condition should be. But I'm thinking 
something along the lines of:
Thread::current()->is_Java_thread() || Thread::current()->is_VM_thread() 
&& (!SafepointSynchronize::is_at_safepoint() || !HeapLock->is_locked())
Other suggestions for a good condition are welcome - it is just an 
example that may or may not work.

Thanks,
/Erik

On 2018-03-16 16:01, coleen.phillimore at oracle.com wrote:
>
> Hi Roman, thank you for your comments.
>
> On 3/15/18 9:46 AM, Roman Kennke wrote:
>> Am 14.03.2018 um 18:37 schriebcoleen.phillimore at oracle.com:
>>> Summary: Make sure the holder of a class loader is accessed during
>>> iteration of CLDG
>>>
>>> This is where we should have put the GC barrier.  This can be cleaned
>>> somewhat when we have a WeakHandle holder in the ClassLoaderData, then
>>> the code in ensure_loader_alive() becomes _holder.resolve();
>>>
>>> Tested with tier1-5.
>>>
>>> open webrev athttp://cr.openjdk.java.net/~coleenp/8197844.01/webrev
>>> bug linkhttps://bugs.openjdk.java.net/browse/JDK-8197844
>>>
>>> Thanks,
>>> Coleen
>> I am not sure at all that this wouldn't have adverse effects. Suppose a
>> GC is iterating all CLDs and marks all of them alive, we'd probably
>> never unload it? I probably wouldn't do the keep-alive stuff wholesale
>> in the iterator methods.
>>
>> I've made (and later withdraw) a (IMO) more straightforward patch to
>> address the same:
>>
>> http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/
>>
>> It has the advantage that it doesn't to a bogus load, just to keep
>> something alive. It loads the mirror and at the same time communicates
>> the GC to keep it alive. Maybe better?
>
> So when I made this change, I was being conservative.  Anytime we 
> iterate over the CLDG, and take out oops to put somewhere else, we 
> need to mark that the CLD needs to stay alive for that GC, until the 
> somewhere else is walked.  We also have to worry about these classes 
> being unloaded if we are still holding metadata after walking the CLDG.
>
> I've gone through all the CLDG::classes_do, loaded_classes_do, 
> dictionary_all_entries_do, methods_do, modules_do and packages_do at 
> least 100 times.  (also oops_do variants and cld_do).   All these 
> calls do something with metadata, and it appears that these places are 
> unaffected by GC or class unloading, except the instance in 
> jvmtiGetLoadedClasses that Poonam found.
>
> Also if the closure or function passed to the CLDG iterator functions 
> stop for a safepoint and unload, the walk will be broken.  I didn't 
> find any instances of this in the code on my 100 traversals.  My 
> change is trying to make it so I don't have to do this analysis anymore.
>
> Poonams, Erik's change and your change puts the responsibility on the 
> caller of ClassLoaderDataGraph to keep the class alive that it is 
> processing through the walk.   I think I can accept that as a fix, and 
> withdraw mine, since we seem to have no instances of bugs in other 
> walks.  If it turns out to be something that becomes buggy, I will 
> suggest my fix again as an alternative.
>
> This fix doesn't affect the walks done during GC.  As of this morning, 
> there are no KlassClosures in GC anymore.   Your concern is well 
> placed though because it could help us introduce bugs which could 
> negatively affect class unloading.
>
> So on the balance, there's risk in both versions of the fix, so I'll 
> withdraw mine.  I think the best fix is yours:
>
> http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp.udiff.html
>
> with Erik's function java_mirror_phantom() that does the RootAccess load.
>
> http://cr.openjdk.java.net/~eosterlund/8197844/webrev.00/src/hotspot/share/oops/klass.cpp.udiff.html
>
> but not have it mark the class_loader, only do below.
>
> +oop Klass::java_mirror_phantom() {
> + // Loading the klass_holder as a phantom oop ref keeps the class alive.
> + // After the holder has been kept alive, it is safe to return the 
> mirror.
> + oop mirror = 
> RootAccess<ON_PHANTOM_OOP_REF>::oop_load(k->java_mirror_handle().ptr_raw());
> + return mirror;
> +}
>
> I didn't like that Klass was a friend of ClassLoaderData and could get 
> to the class_loader field (in case I move it).
>
> Your change here is covered by Kim's work to accessorize jni.
>
> http://cr.openjdk.java.net/~rkennke/8199612/webrev.00/src/hotspot/share/runtime/jniHandles.cpp.udiff.html
>
> So I withdraw and I'll unassign myself and you and/or Erik can take 
> this bug back again.
>
> Thanks!
> Coleen
>
>
>> Roman
>>
>