[RFA] JVMTI/JDWP GetAllLoadedClasses

[Tom Tromey]

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

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.

Kyle> +// Element in the temporary loaded classes linked list for JVMTI.

Will this list ever differ from the system_class_list?

Kyle> +      for (struct _Jv_TempClassListElement *ptr = jvmti_class_list_head;
Kyle> +           ptr;
Kyle> +           ptr = ptr->next)
Kyle> +        {
Kyle> +          VMClassLoader::loaded_classes->add (
Kyle> +                                          new ref::WeakReference (ptr->klass));
Kyle> +          _Jv_Free (jvmti_class_list_head);
Kyle> +          jvmti_class_list_head = ptr;
Kyle> +        }

This loop looks strange.  I think the first time through, we will
dereference a freed pointer, which we shouldn't do.  The usual
approach in this sort of situation is a new temporary:

for (_Jv_TempClassListElement *ptr = jvmti_class_list_head;
     ptr; )
  {
    ...
    _Jv_TempClassListElement *next = ptr->next;
    _Jv_Free (ptr);
    ptr = next;
  }

But maybe this loop isn't needed at all, depending on what exactly is
ending up on this list.

Kyle> +          if (!jvmti_class_list_head)
Kyle> +            {
Kyle> +              jvmti_class_list_head 
Kyle> +                = reinterpret_cast<struct _Jv_TempClassListElement *> 
Kyle> +                    (_Jv_MallocUnchecked (
Kyle> +                       sizeof (struct _Jv_TempClassListElement)));
Kyle> +              jvmti_class_list_head->next = NULL;
Kyle> +            }
Kyle> +          else
Kyle> +            {
Kyle> +              struct _Jv_TempClassListElement *tmp 
Kyle> +                = reinterpret_cast<struct _Jv_TempClassListElement *> 
Kyle> +                    (_Jv_MallocUnchecked (
Kyle> +                       sizeof (struct _Jv_TempClassListElement)));
Kyle> +              tmp->next = jvmti_class_list_head;
Kyle> +              jvmti_class_list_head = tmp;
Kyle> +            }
Kyle> +            jvmti_class_list_head->klass = k;

Here you don't need separate cases.  The 'else' clause is generic and
works fine when jvmti_class_list_head == NULL.

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

So as you can see there's little consistency here.

Tom