Re: [RFA] JVMTI/JDWP GetAllLoadedClasses

[Kyle Galloway <kgallowa at redhat dot com>]

Tom Tromey wrote:
> > > > > > "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:
> > > > > >             
>
> Kyle> +  java::util::HashSet *loaded_classes = VMClassLoader::getAllLoadedClasses ();
> [...]
> Kyle> +  JvSynchronize list (VMClassLoader::loaded_classes);	
> Kyle> +  ::java::util::Iterator *it = loaded_classes->iterator ();
> Kyle> +  for (int i = 0; it->hasNext () && i < *num_classes; i++)
> Kyle> +    {
> Kyle> +      (*classes)[i] = reinterpret_cast<Class *> 
> Kyle> +                        (((ref::WeakReference *)it->next ())->get ());
> Kyle> +    }
>
> Unless we are stopping other threads, I think there is a race here
> where one of the WeakReferences can be collected between the time
> loaded_classes is allocated and this loop.  So, an element of *classes
> could be NULL.
>
> You can guard against this by checking for NULL and keeping a second
> int for the output slot to use, and then setting *num_classes
> afterward.
>   
You're totally right, if there are multiple JVMTI agents at work it 
could cause trouble here.
> Kyle>  gnu::classpath::jdwp::VMVirtualMachine::getAllLoadedClasses (void)
> Kyle>  {
>
> It seems a bit weird for this method to call GetLoadedClasses to turn
> the VMClassLoader HashSet into an array, and then create a new
> ArrayList out of it.
>   
There is stuff like this all over the place.  Since we started to go 
through JVMTI to implement JDWP, there are lots of instances where we 
call JVMTI methods to massage the data one way, then take that data and 
repackage it again for JDWP.  The feeling seemed to be that we should 
try to use JVMTI as much as possible so that the approach would be more 
portable.
> Kyle> +// Element in the temporary loaded classes linked list for JVMTI.
>
> Will this list ever differ from the system_class_list?
>   
From what I've seen, the list of classes that is copied over in 
_Jv_CopyClassesToSystemLoader is often empty.
> Also, _Jv_PushClass seems like the wrong place to put this
> registration.  I'm not positive but I think it probably is never
> called for array classes, for instance.
>
> You may need to hook in to multiple places.  Bleah.  Sorry about this,
> this code is still very complicated.  Be glad you didn't see it before
> the big cleanup :-)
>
> Array classes are all defined via _Jv_NewArrayClass.  Interpreted
> classes are all handled through VMClassLoader::defineClass.  CNI
> classes all go via _Jv_RegisterClasses or _Jv_RegisterClasses_Counted
> or _Jv_RegisterClass (the last may be deprecated, I am not sure).  BC
> classes go through _Jv_RegisterNewClasses, I think.  (Actually I don't
> remember this part of the code.)
>   
Ah.  I was working on the assumption that all classes had to go through 
_Jv_PushClass.  I'm going to need to take another look at this then.

Thanks,
Kyle