From 866c057f0d5c4da366158e174110f64598bf0c16 Mon Sep 17 00:00:00 2001 From: Joel Dice Date: Mon, 3 Mar 2014 09:56:26 -0700 Subject: [PATCH] fix Class.getDeclaredMethods getDeclaredMethods was returning methods which were inherited from interfaces but not (re)declared in the class itself, due to the VM's internal use of VMClass.methodTable differing from its role in reflection. For reflection, we must only include the declared methods, not the inherited but un-redeclared ones. Previously, we saved the original method table in ClassAddendum.methodTable before creating a new one which contains both declared and inherited methods. That wasted space, so this patch replaces ClassAddendum.methodTable with ClassAddendum.declaredMethodCount, which specifies how many of the methods in VMClass.methodTable were declared in that class. Alternatively, we could ensure that undeclared methods always have their VMMethod.class_ field set to the declaring class instead of the inheriting class. I tried this, but it led to subtle crashes in interface method lookup. The rest of the VM relies not only on VMClass.methodTable containing all inherited interface methods but also that those methods point to the inheriting class, not the declaring class. Changing those assumptions would be a much bigger (and more dangerous in terms of regression potential) effort than I care to take on right now. The solution I chose is a bit ugly, but it's safe. --- classpath/avian/ClassAddendum.java | 9 ++++- classpath/avian/Classes.java | 53 +++++++++++++++++++----------- classpath/avian/VMClass.java | 9 +++++ src/classpath-openjdk.cpp | 33 +++++++++++-------- src/machine.cpp | 10 +++--- test/Reflection.java | 8 +++++ 6 files changed, 84 insertions(+), 38 deletions(-) diff --git a/classpath/avian/ClassAddendum.java b/classpath/avian/ClassAddendum.java index c445b86dbd..ded1b1879b 100644 --- a/classpath/avian/ClassAddendum.java +++ b/classpath/avian/ClassAddendum.java @@ -13,7 +13,14 @@ package avian; public class ClassAddendum extends Addendum { public Object[] interfaceTable; public InnerClassReference[] innerClassTable; - public Object[] methodTable; + /** + * If this value is negative, all the methods in VMClass.methodTable + * were declared in that class. Otherwise, only the first + * declaredMethodCount methods in that table were declared in that + * class, while the rest were declared in interfaces implemented or + * extended by that class. + */ + public int declaredMethodCount; public Object enclosingClass; public Object enclosingMethod; } diff --git a/classpath/avian/Classes.java b/classpath/avian/Classes.java index 3db5a067ab..018c0c83d9 100644 --- a/classpath/avian/Classes.java +++ b/classpath/avian/Classes.java @@ -221,6 +221,18 @@ public class Classes { return result; } + private static int declaredMethodCount(VMClass c) { + ClassAddendum a = c.addendum; + if (a != null) { + int count = a.declaredMethodCount; + if (count >= 0) { + return count; + } + } + VMMethod[] table = c.methodTable; + return table == null ? 0 : table.length; + } + public static void link(VMClass c, ClassLoader loader) { acquireClassLock(); try { @@ -238,9 +250,10 @@ public class Classes { } } - if (c.methodTable != null) { - for (int i = 0; i < c.methodTable.length; ++i) { - VMMethod m = c.methodTable[i]; + VMMethod[] methodTable = c.methodTable; + if (methodTable != null) { + for (int i = 0; i < methodTable.length; ++i) { + VMMethod m = methodTable[i]; for (int j = 1; j < m.spec.length;) { j = resolveSpec(loader, m.spec, j); @@ -394,17 +407,18 @@ public class Classes { public static int findMethod(VMClass vmClass, String name, Class[] parameterTypes) { - if (vmClass.methodTable != null) { + VMMethod[] methodTable = vmClass.methodTable; + if (methodTable != null) { Classes.link(vmClass); if (parameterTypes == null) { parameterTypes = new Class[0]; } - for (int i = 0; i < vmClass.methodTable.length; ++i) { - if (toString(vmClass.methodTable[i].name).equals(name) - && match(parameterTypes, - getParameterTypes(vmClass.methodTable[i]))) + for (int i = 0; i < methodTable.length; ++i) { + VMMethod m = methodTable[i]; + if (toString(m.name).equals(name) + && match(parameterTypes, getParameterTypes(m))) { return i; } @@ -415,12 +429,12 @@ public class Classes { public static int countMethods(VMClass vmClass, boolean publicOnly) { int count = 0; - if (vmClass.methodTable != null) { - for (int i = 0; i < vmClass.methodTable.length; ++i) { - if (((! publicOnly) - || ((vmClass.methodTable[i].flags & Modifier.PUBLIC)) - != 0) - && (! toString(vmClass.methodTable[i].name).startsWith("<"))) + VMMethod[] methodTable = vmClass.methodTable; + if (methodTable != null) { + for (int i = 0, j = declaredMethodCount(vmClass); i < j; ++i) { + VMMethod m = methodTable[i]; + if (((! publicOnly) || ((m.flags & Modifier.PUBLIC)) != 0) + && (! toString(m.name).startsWith("<"))) { ++ count; } @@ -431,14 +445,15 @@ public class Classes { public static Method[] getMethods(VMClass vmClass, boolean publicOnly) { Method[] array = new Method[countMethods(vmClass, publicOnly)]; - if (vmClass.methodTable != null) { + VMMethod[] methodTable = vmClass.methodTable; + if (methodTable != null) { Classes.link(vmClass); int ai = 0; - for (int i = 0; i < vmClass.methodTable.length; ++i) { - if ((((vmClass.methodTable[i].flags & Modifier.PUBLIC) != 0) - || (! publicOnly)) - && ! toString(vmClass.methodTable[i].name).startsWith("<")) + for (int i = 0, j = declaredMethodCount(vmClass); i < j; ++i) { + VMMethod m = methodTable[i]; + if (((! publicOnly) || ((m.flags & Modifier.PUBLIC) != 0)) + && ! toString(m.name).startsWith("<")) { array[ai++] = makeMethod(SystemClassLoader.getClass(vmClass), i); } diff --git a/classpath/avian/VMClass.java b/classpath/avian/VMClass.java index 6b5a052c39..a370e02924 100644 --- a/classpath/avian/VMClass.java +++ b/classpath/avian/VMClass.java @@ -24,6 +24,15 @@ public class VMClass { public Object[] interfaceTable; public VMMethod[] virtualTable; public VMField[] fieldTable; + /** + * Methods declared in this class, plus any abstract virtual methods + * inherited from implemented or extended interfaces. If addendum + * is non-null and addendum.declaredMethodCount is non-negative, + * then the first addendum.declaredMethodCount methods are declared + * methods, while the rest are abstract virtual methods. If + * addendum is null or addendum.declaredMethodCount is negative, all + * are declared methods. + */ public VMMethod[] methodTable; public ClassAddendum addendum; public Object staticTable; diff --git a/src/classpath-openjdk.cpp b/src/classpath-openjdk.cpp index 76e797de10..d51a11eb84 100644 --- a/src/classpath-openjdk.cpp +++ b/src/classpath-openjdk.cpp @@ -2121,25 +2121,26 @@ interceptFileOperations(Thread* t, bool updateRuntimeData) } #endif // AVIAN_OPENJDK_SRC -object -getClassMethodTable(Thread* t, object c) +unsigned +classDeclaredMethodCount(Thread* t, object c) { object addendum = classAddendum(t, c); if (addendum) { - object table = classAddendumMethodTable(t, addendum); - if (table) { - return table; + int count = classAddendumDeclaredMethodCount(t, addendum); + if (count >= 0) { + return count; } } - return classMethodTable(t, c); + object table = classMethodTable(t, c); + return table == 0 ? 0 : arrayLength(t, table); } unsigned countMethods(Thread* t, object c, bool publicOnly) { - object table = getClassMethodTable(t, c); + object table = classMethodTable(t, c); unsigned count = 0; - for (unsigned i = 0; i < arrayLength(t, table); ++i) { + for (unsigned i = 0, j = classDeclaredMethodCount(t, c); i < j; ++i) { object vmMethod = arrayBody(t, table, i); if (((not publicOnly) or (methodFlags(t, vmMethod) & ACC_PUBLIC)) and byteArrayBody(t, methodName(t, vmMethod), 0) != '<') @@ -2171,9 +2172,9 @@ countFields(Thread* t, object c, bool publicOnly) unsigned countConstructors(Thread* t, object c, bool publicOnly) { - object table = getClassMethodTable(t, c); + object table = classMethodTable(t, c); unsigned count = 0; - for (unsigned i = 0; i < arrayLength(t, table); ++i) { + for (unsigned i = 0, j = classDeclaredMethodCount(t, c); i < j; ++i) { object vmMethod = arrayBody(t, table, i); if (((not publicOnly) or (methodFlags(t, vmMethod) & ACC_PUBLIC)) and strcmp(reinterpret_cast @@ -4214,7 +4215,7 @@ jvmGetClassDeclaredMethods(Thread* t, uintptr_t* arguments) jclass c = reinterpret_cast(arguments[0]); jboolean publicOnly = arguments[1]; - object table = getClassMethodTable(t, jclassVmClass(t, *c)); + object table = classMethodTable(t, jclassVmClass(t, *c)); if (table) { PROTECT(t, table); @@ -4224,7 +4225,9 @@ jvmGetClassDeclaredMethods(Thread* t, uintptr_t* arguments) PROTECT(t, array); unsigned ai = 0; - for (unsigned i = 0; i < arrayLength(t, table); ++i) { + for (unsigned i = 0, j = classDeclaredMethodCount(t, jclassVmClass(t, *c)); + i < j; ++i) + { object vmMethod = arrayBody(t, table, i); PROTECT(t, vmMethod); @@ -4308,7 +4311,7 @@ jvmGetClassDeclaredConstructors(Thread* t, uintptr_t* arguments) jclass c = reinterpret_cast(arguments[0]); jboolean publicOnly = arguments[1]; - object table = getClassMethodTable(t, jclassVmClass(t, *c)); + object table = classMethodTable(t, jclassVmClass(t, *c)); if (table) { PROTECT(t, table); @@ -4318,7 +4321,9 @@ jvmGetClassDeclaredConstructors(Thread* t, uintptr_t* arguments) PROTECT(t, array); unsigned ai = 0; - for (unsigned i = 0; i < arrayLength(t, table); ++i) { + for (unsigned i = 0, j = classDeclaredMethodCount(t, jclassVmClass(t, *c)); + i < j; ++i) + { object vmMethod = arrayBody(t, table, i); PROTECT(t, vmMethod); diff --git a/src/machine.cpp b/src/machine.cpp index ca99b330e0..7c62617d09 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -1123,7 +1123,7 @@ getClassAddendum(Thread* t, object class_, object pool) if (addendum == 0) { PROTECT(t, class_); - addendum = makeClassAddendum(t, pool, 0, 0, 0, 0, 0, 0, 0); + addendum = makeClassAddendum(t, pool, 0, 0, 0, 0, -1, 0, 0); set(t, class_, ClassAddendum, addendum); } return addendum; @@ -2206,13 +2206,15 @@ parseMethodTable(Thread* t, Stream& s, object class_, object pool) if (abstractVirtuals) { PROTECT(t, vtable); - object addendum = getClassAddendum(t, class_, pool); - set(t, addendum, ClassAddendumMethodTable, - classMethodTable(t, class_)); + object originalMethodTable = classMethodTable(t, class_); + PROTECT(t, originalMethodTable); unsigned oldLength = classMethodTable(t, class_) ? arrayLength(t, classMethodTable(t, class_)) : 0; + object addendum = getClassAddendum(t, class_, pool); + classAddendumDeclaredMethodCount(t, addendum) = oldLength; + object newMethodTable = makeArray (t, oldLength + listSize(t, abstractVirtuals)); diff --git a/test/Reflection.java b/test/Reflection.java index 8607cacc45..bd054f9ef9 100644 --- a/test/Reflection.java +++ b/test/Reflection.java @@ -241,6 +241,8 @@ public class Reflection { expect(avian.TestReflection.get(Baz.class.getField("foo"), new Baz()) .equals(42)); expect((Baz.class.getModifiers() & Modifier.PUBLIC) == 0); + + expect(B.class.getDeclaredMethods().length == 0); } protected static class Baz { @@ -263,3 +265,9 @@ class Foo { } class MyException extends RuntimeException { } + +interface A { + void foo(); +} + +interface B extends A { }