reflection work

[David Lichteblau]

Hi,

Grzegorz Prokopski provided a patch (merged from SableVM, I believe)
implementing some tricky parts of the reflection methods in Java.

The patch seems interesting to me in that it handles security checks
portably, so that virtual machines do not have to worry about them.

However, below some comments on other parts of the patch.

Part of the issue here is the handling of native methods.  Reflection
does not currently use VM* classes (neither before nor with the patch),
though the patch tries to go into this direction, I believe.


Regards,
David

>  public final class Constructor
>  extends AccessibleObject implements Member
>  {
> -  private Class clazz;
> -  private int slot;
> +
> +  private Class declaringClass;

Renaming `clazz' to `declaringClass' in Constructor is probably a good
idea given that other reflection classes use the latter name.

> -  private Constructor(Class declaringClass,int slot)
> -  {
> -    this.clazz = declaringClass;
> -    this.slot = slot;
> -  }
>  
> -  private Constructor()
> +  byte[] vmData;
> +  private Constructor(byte[] vmData)
>    {
> +    this.vmData = vmData;
>    }

As discussed for the vmdata slot in java.lang.Class, the slot (if any)
must be typed java.lang.Object (not byte[]) for some virtual machines.

However, the reflection classes in Classpath currently use the pair of
class object and slot number to identify class members.  While it might
be possible to replace this with a vmdata slot, I do not see a clear
benefit of this change.

There needs to be _some_ way of getting from reflection objects to
virtual machine data, of course.  But java.lang.Class with its vmdata
already solves this problem in a central place.

Keeping this problem centralized instead of introducing vmdata slots all
over the place seems vaguely preferable to me.

>     */
>    public Class getDeclaringClass()
>    {
> -    return clazz;
> +    if (declaringClass == null)
> +      {
> +     declaringClass = nativeGetDeclaringClass(vmData);
> +      }
> +
> +    return declaringClass;
> +
>    }
>  
> +  public static native Class nativeGetDeclaringClass(byte[] vmData);

This native method would not be necessary.  The class is already known
at instantiation time.

> -  public native int getModifiers();
> +  public int getModifiers()
> +  {
> +    return nativeGetModifiers(vmData);
> +  }
> +  public native int nativeGetModifiers(byte[] vmData);
> +

The are many such changes in the patch.  (I note that the method was
probably intended to be static.)

Following previous decisions in classpath, the native methods should
probably be moved into VM* classes.

(Personally I am not entirely convinced that making all these methods
static is necessary, except that the VM* class design enforces it.)

>    public Class[] getParameterTypes()
>    {
>      if (parameterTypes == null)
> -      return new Class[0];
> +    {
> +      parameterTypes =
> +        ReflectUtil.getParameterTypes(nativeGetDescriptor(vmData));
> +
> +    }
>      return parameterTypes;
>    }

Sounds OK to me, except that the virtual machine should know how to do
this anyway, so there is perhaps no pressing need to do this in Java.
It does not save a native method in the end.

> @@ -251,11 +273,174 @@
>      throws InstantiationException, IllegalAccessException,
>             InvocationTargetException
>    {
> -    return constructNative(args, clazz, slot);
> +    /* The following code is more of a hack, than real
> +     * implementation, as it lacks checking and widening. To be
> +     * fixed...*/
[...]

This Java code needs to be fixed before it can replace existing native
implementations.

Also, this and other methods are redundant with similar methods in the
Method class.

> +  private void checkField(Class type, Object o, Class[] acceptTypes) {
> +    if (!Modifier.isStatic(getModifiers())) {
> +      // instance field checks
> +      if (o == null) {
> +     throw new NullPointerException();
> +      }
> +
> +      if (!(getDeclaringClass().isInstance(o))) {
> +     throw new IllegalArgumentException();
> +      }
> +    }
> +
> +
> +    // access check
> +  
> +    // *** TODO ***

Should be completed before commit.

pgp6ldlOeSpb3.pgp
Description: PGP signature