Note that this finds a declaration, which is usually not an implementation: an + * interface method is implicitly abstract unless it is {@code default}, {@code static}, or + * private. Pass true for {@code implementationsOnly} to match only a {@code default} method, + * which is the one case where the interface really does hold the code that will run. + * + *
Limitation: when several interfaces match, this returns the first one reached rather than + * the maximally specific one that JVMS 5.4.3.3 selects. A class that implements both an interface + * and a subinterface that reabstracts the same method gets the first of the two in declaration + * order, which may be the supertype. The consequence is confined to precision: the caller uses + * the answer only to decide whether the target is instrumented, and a wrong answer there loses + * comparability through the call rather than breaking it, because the uninstrumented overload it + * then invokes always exists. * * @param startClass the class whose interfaces are to be searched * @param methodName the target method to search for * @param paramTypes the target method's parameter types - * @return the name of the interface class containing target method, or null if not found + * @param implementationsOnly if true, match only a {@code default} method; if false, match any + * declaration, abstract ones included + * @return the name of the interface that declares the target method, or null if not found */ - private @Nullable @ClassGetName String getDefiningInterface( - JavaClass startClass, @Identifier String methodName, Type[] paramTypes) { + private @Nullable @ClassGetName String getDeclaringInterface( + JavaClass startClass, + @Identifier String methodName, + Type[] paramTypes, + boolean implementationsOnly) { - if (debugGetDefiningInterface) { + if (debugGetDeclaringInterface) { System.out.println("searching interfaces of: " + startClass.getClassName()); } for (@ClassGetName String interfaceName : startClass.getInterfaceNames()) { - if (debugGetDefiningInterface) { + if (debugGetDeclaringInterface) { System.out.println("interface: " + interfaceName); } JavaClass ji; @@ -2302,17 +2332,32 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { if (ji == null) { throw new Error("Unable to find class: " + interfaceName); } + boolean reabstracted = false; for (Method jm : ji.getMethods()) { - if (debugGetDefiningInterface) { + if (debugGetDeclaringInterface) { System.out.println(" " + jm.getName() + Arrays.toString(jm.getArgumentTypes())); } if (jm.getName().equals(methodName) && Arrays.equals(jm.getArgumentTypes(), paramTypes)) { - // We have a match. + // We have a match. Neither a static nor a private interface method is ever the + // target of an INVOKEVIRTUAL: a private one is not even inherited. + if (jm.isStatic() || jm.isPrivate()) { + continue; + } + if (implementationsOnly && jm.isAbstract()) { + // This interface declares the method abstract. An interface may reabstract a default + // it inherits, and an implementor must then define the method, so any default above + // this point is hidden: do not search this branch further. + reabstracted = true; + break; + } return interfaceName; } } + if (reabstracted) { + continue; + } // no match found; does this interface extend other interfaces? - @ClassGetName String foundAbove = getDefiningInterface(ji, methodName, paramTypes); + @ClassGetName String foundAbove = getDeclaringInterface(ji, methodName, paramTypes, implementationsOnly); if (foundAbove != null) { // We have a match. return foundAbove; @@ -2564,6 +2609,10 @@ private boolean isTargetInstrumented( } @ClassGetName String targetClassname = classname; + // Interfaces are not consulted in the loop below: JVMS 5.4.3.3 resolves a method + // against the class's own declaration, then the superclass chain, and only then the + // superinterfaces, so the whole chain is searched first and the interfaces of the + // original target class afterwards. // Search this class for the target method. If not found, set targetClassname to // its superclass and try again. mainloop: @@ -2605,37 +2654,40 @@ private boolean isTargetInstrumented( } } - { - // no methods match - search this class's interfaces + // Method not found; perhaps inherited from superclass. + // Cannot use "targetClass = targetClass.getSuperClass()" because the superclass might + // not have been loaded into BCEL yet. + if (targetClass.getSuperclassNameIndex() == 0) { + // No class in the chain declares the method, so it comes from an interface. Prefer + // a default method, which is an implementation; an abstract declaration only says + // where the method is declared, but that is the best available answer. @ClassGetName String found; try { - found = getDefiningInterface(targetClass, methodName, paramTypes); + JavaClass origin = getJavaClass(classname); + found = + origin == null + ? null + : firstNonNull( + getDeclaringInterface(origin, methodName, paramTypes, true), + getDeclaringInterface(origin, methodName, paramTypes, false)); } catch (Throwable e) { // We cannot locate or read the .class file, better assume it is not instrumented. targetInstrumented = false; break; } - if (found != null) { - // We have a match. + if (found == null) { if (debugHandleInvoke) { - System.out.printf("we have a match%n%n"); + System.out.printf("Unable to locate method: %s%n%n", methodName); + } + targetInstrumented = false; + } else { + if (debugHandleInvoke) { + System.out.printf("declared by interface %s%n%n", found); } if (BcelUtil.inJdk(found)) { targetInstrumented = false; } - break; - } - } - - // Method not found; perhaps inherited from superclass. - // Cannot use "targetClass = targetClass.getSuperClass()" because the superclass might - // not have been loaded into BCEL yet. - if (targetClass.getSuperclassNameIndex() == 0) { - // The target class is Object; the search completed without finding a matching method. - if (debugHandleInvoke) { - System.out.printf("Unable to locate method: %s%n%n", methodName); } - targetInstrumented = false; break; } // Recurse looking in the superclass. diff --git a/java/daikon/dcomp/DCInstrument24.java b/java/daikon/dcomp/DCInstrument24.java index 07b2e5cf0..2197fa01a 100644 --- a/java/daikon/dcomp/DCInstrument24.java +++ b/java/daikon/dcomp/DCInstrument24.java @@ -538,8 +538,8 @@ public record myLocalVariable(int slot, String name, ClassDesc descriptor) {} /** If true, enable JUnit analysis debugging. */ protected static final boolean debugJunitAnalysis = false; - /** If true, enable {@link #getDefiningInterface} debugging. */ - protected static final boolean debugGetDefiningInterface = false; + /** If true, enable {@link #getDeclaringInterface} debugging. */ + protected static final boolean debugGetDeclaringInterface = false; /** If true, enable {@link #handleInvoke} debugging. */ protected static final boolean debugHandleInvoke = false; @@ -2887,23 +2887,53 @@ private void add_exit( } /** - * Returns the interface class name containing the implementation of the given method. The - * interfaces of {@code startClass} are recursively searched. + * Returns the first argument if it is non-null, otherwise the second. + * + * @param first the preferred value + * @param second the fallback value + * @return the first non-null argument, or null if both are null + */ + private static @Nullable @BinaryName String firstNonNull( + @Nullable @BinaryName String first, @Nullable @BinaryName String second) { + return first != null ? first : second; + } + + /** + * Returns the name of the interface that declares the given method. The interfaces of {@code + * startClass} are recursively searched. + * + *
Note that this finds a declaration, which is usually not an implementation: an + * interface method is implicitly abstract unless it is {@code default}, {@code static}, or + * private. Pass true for {@code implementationsOnly} to match only a {@code default} method, + * which is the one case where the interface really does hold the code that will run. + * + *
Limitation: when several interfaces match, this returns the first one reached rather than
+ * the maximally specific one that JVMS 5.4.3.3 selects. A class that implements both an interface
+ * and a subinterface that reabstracts the same method gets the first of the two in declaration
+ * order, which may be the supertype. The consequence is confined to precision: the caller uses
+ * the answer only to decide whether the target is instrumented, and a wrong answer there loses
+ * comparability through the call rather than breaking it, because the uninstrumented overload it
+ * then invokes always exists.
*
* @param startClass the class whose interfaces are to be searched
* @param methodName the target method to search for
* @param paramTypes the target method's parameter types
- * @return the name of the interface class containing target method, or null if not found
+ * @param implementationsOnly if true, match only a {@code default} method; if false, match any
+ * declaration, abstract ones included
+ * @return the name of the interface that declares the target method, or null if not found
*/
- private @Nullable @BinaryName String getDefiningInterface(
- ClassModel startClass, @Identifier String methodName, ClassDesc[] paramTypes) {
+ private @Nullable @BinaryName String getDeclaringInterface(
+ ClassModel startClass,
+ @Identifier String methodName,
+ ClassDesc[] paramTypes,
+ boolean implementationsOnly) {
- if (debugGetDefiningInterface) {
+ if (debugGetDeclaringInterface) {
System.out.println("searching interfaces of: " + ClassGen24.getClassName(startClass));
}
for (ClassEntry classEntry : startClass.interfaces()) {
@BinaryName String interfaceName = Signatures.internalFormToBinaryName(classEntry.asInternalName());
- if (debugGetDefiningInterface) {
+ if (debugGetDeclaringInterface) {
System.out.println("interface: " + interfaceName);
}
ClassModel cm;
@@ -2914,19 +2944,35 @@ private void add_exit(
} catch (Throwable t) {
throw new DynCompError(String.format("Unable to load class: %s", interfaceName), t);
}
+ boolean reabstracted = false;
for (MethodModel jm : cm.methods()) {
String jmName = jm.methodName().stringValue();
MethodTypeDesc mtd = jm.methodTypeSymbol();
- if (debugGetDefiningInterface) {
+ if (debugGetDeclaringInterface) {
System.out.println(" " + jmName + Arrays.toString(mtd.parameterArray()));
}
if (jmName.equals(methodName) && Arrays.equals(mtd.parameterArray(), paramTypes)) {
- // We have a match.
+ // We have a match. Neither a static nor a private interface method is ever the
+ // target of an INVOKEVIRTUAL: a private one is not even inherited.
+ AccessFlags jmFlags = jm.flags();
+ if (jmFlags.has(AccessFlag.STATIC) || jmFlags.has(AccessFlag.PRIVATE)) {
+ continue;
+ }
+ if (implementationsOnly && jmFlags.has(AccessFlag.ABSTRACT)) {
+ // This interface declares the method abstract. An interface may reabstract a default
+ // it inherits, and an implementor must then define the method, so any default above
+ // this point is hidden: do not search this branch further.
+ reabstracted = true;
+ break;
+ }
return interfaceName;
}
}
+ if (reabstracted) {
+ continue;
+ }
// no match found; does this interface extend other interfaces?
- @BinaryName String foundAbove = getDefiningInterface(cm, methodName, paramTypes);
+ @BinaryName String foundAbove = getDeclaringInterface(cm, methodName, paramTypes, implementationsOnly);
if (foundAbove != null) {
// We have a match.
return foundAbove;
@@ -3216,7 +3262,10 @@ private boolean isTargetInstrumented(
@BinaryName String targetClassname = classname;
// Search this class for the target method. If not found, set targetClassname to
- // its superclass and try again.
+ // its superclass and try again. Interfaces are not consulted here: JVMS 5.4.3.3
+ // resolves a method against the class's own declaration, then the superclass chain,
+ // and only then the superinterfaces, so the whole chain is searched first and the
+ // interfaces of the original target class afterwards.
mainloop:
while (true) {
// Check that the class exists
@@ -3256,36 +3305,37 @@ private boolean isTargetInstrumented(
}
}
- {
- // no methods match - search this class's interfaces
+ // No class in the chain declares the method, so it comes from an interface. Prefer
+ // a default method, which is an implementation; an abstract declaration only says
+ // where the method is declared, but that is the best available answer.
+ if (targetClassname.equals("java.lang.Object")) {
@BinaryName String found;
try {
- found = getDefiningInterface(targetClass, methodName, paramTypes);
+ ClassModel origin = getClassModel(classname);
+ found =
+ origin == null
+ ? null
+ : firstNonNull(
+ getDeclaringInterface(origin, methodName, paramTypes, true),
+ getDeclaringInterface(origin, methodName, paramTypes, false));
} catch (Throwable e) {
// We cannot locate or read the .class file, better assume it is not instrumented.
targetInstrumented = false;
break;
}
- if (found != null) {
- // We have a match.
+ if (found == null) {
if (debugHandleInvoke) {
- System.out.printf("we have a match%n%n");
+ System.out.printf("Unable to locate method: %s%n%n", methodName);
+ }
+ targetInstrumented = false;
+ } else {
+ if (debugHandleInvoke) {
+ System.out.printf("declared by interface %s%n%n", found);
}
if (BcelUtil.inJdk(found)) {
targetInstrumented = false;
}
- break;
}
- }
-
- // Method not found; perhaps inherited from superclass.
- if (targetClassname.equals("java.lang.Object")) {
- // The target class was Object; the search completed without finding a matching
- // method.
- if (debugHandleInvoke) {
- System.out.printf("Unable to locate method: %s%n%n", methodName);
- }
- targetInstrumented = false;
break;
}
diff --git a/java/daikon/dcomp/DCInstrumentTest24.java b/java/daikon/dcomp/DCInstrumentTest24.java
index 91ec8b3bf..3284511d4 100644
--- a/java/daikon/dcomp/DCInstrumentTest24.java
+++ b/java/daikon/dcomp/DCInstrumentTest24.java
@@ -34,6 +34,7 @@
import java.util.function.Supplier;
import java.util.regex.Pattern;
import org.checkerframework.checker.interning.qual.Interned;
+import org.checkerframework.checker.lock.qual.GuardSatisfied;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.signature.qual.BinaryName;
import org.junit.Test;
@@ -153,6 +154,119 @@ private static byte[] classBytes(@BinaryName String binaryName) throws IOExcepti
}
}
+ /**
+ * Instruments the named class, setting the runtime state that {@link DCInstrument24#instrument}
+ * requires. Tests must not depend on an earlier test having set it.
+ *
+ * @param binaryName the class to instrument
+ * @return the instrumented class
+ * @throws IOException if the class file cannot be read
+ */
+ private static byte[] instrumentCaller(@BinaryName String binaryName) throws IOException {
+ @BinaryName String saved = DCRuntime.instrumentation_interface;
+ boolean savedJdkInstrumented = Premain.jdk_instrumented;
+ DCRuntime.instrumentation_interface = "daikon.dcomp.DCompInstrumented";
+ // handleInvoke only resolves the target when the JDK is not instrumented; with an
+ // instrumented JDK every JDK method has an instrumented form and there is nothing to decide.
+ Premain.jdk_instrumented = false;
+ byte @Nullable [] result;
+ try {
+ result = instrument(classBytes(binaryName), binaryName);
+ } finally {
+ Premain.jdk_instrumented = savedJdkInstrumented;
+ DCRuntime.instrumentation_interface = saved;
+ }
+ assert result != null : "@AssumeAssertion(nullness)";
+ return result;
+ }
+
+ /**
+ * Returns true if the instrumented form of {@code caller} invokes {@code methodName} with a
+ * DCompMarker argument, that is, if the instrumenter decided the target was instrumented.
+ *
+ * @param classBytes an instrumented class
+ * @param methodName the name of the invoked method
+ * @return true if the call carries a DCompMarker argument
+ */
+ private static boolean invokesInstrumentedForm(byte[] classBytes, String methodName) {
+ ClassModel classModel = ClassFile.of().parse(classBytes);
+ for (MethodModel method : classModel.methods()) {
+ for (CodeElement element :
+ method.code().orElse(null) == null
+ ? List. {@link IteratorWithSuperclassRemove} inherits {@code remove} from an application superclass
+ * and also implements {@code java.util.Iterator}, which declares {@code remove} as a default. The
+ * superclass implementation is what runs, and it is instrumented, so the call must use the
+ * instrumented form. Consulting the interface first would find the JDK's default and wrongly
+ * treat the call as uninstrumented, losing comparability through it.
+ *
+ * @throws IOException if the class file cannot be read
+ */
+ @Test
+ public void testSuperclassOutranksInterfaceDefault() throws IOException {
+ @SuppressWarnings("signature:assignment") // the name of a nested class
+ @BinaryName String callerName = CallsSuperclassRemove.class.getName();
+ byte[] instrumented = instrumentCaller(callerName);
+ assertTrue(
+ "an interface default outranked the superclass implementation",
+ invokesInstrumentedForm(instrumented, "remove"));
+ }
+
+ /**
+ * Tests that a private interface method is not treated as declaring the method being resolved.
+ * {@link PrivateThenJdkDefault} lists {@link PrivateRemove} before {@code java.util.Iterator}, so
+ * a search that matches the private {@code remove} stops at an application interface and
+ * concludes the target is instrumented. Skipping it reaches the JDK's default, which is not.
+ *
+ * @throws IOException if the class file cannot be read
+ */
+ @Test
+ public void testPrivateInterfaceMethodIsNotADeclaration() throws IOException {
+ @SuppressWarnings("signature:assignment") // the name of a nested class
+ @BinaryName String callerName = CallsRemoveThroughPrivate.class.getName();
+ byte[] instrumented = instrumentCaller(callerName);
+ assertFalse(
+ "a private interface method was treated as the declaration",
+ invokesInstrumentedForm(instrumented, "remove"));
+ }
+
+ /**
+ * Tests that an interface which reabstracts an inherited {@code default} hides it. {@link
+ * ReabstractsRemove} redeclares {@code Iterator.remove} as abstract, so an implementor must
+ * define the method and the JDK's default no longer applies. Searching past the reabstraction and
+ * finding that default would wrongly attribute the method to the JDK.
+ *
+ * @throws IOException if the class file cannot be read
+ */
+ @Test
+ public void testReabstractionHidesInheritedDefault() throws IOException {
+ @SuppressWarnings("signature:assignment") // the name of a nested class
+ @BinaryName String callerName = CallsReabstractedRemove.class.getName();
+ byte[] instrumented = instrumentCaller(callerName);
+ assertTrue(
+ "a reabstracted default was still treated as the JDK's implementation",
+ invokesInstrumentedForm(instrumented, "remove"));
+ }
+
/**
* Instruments the given class, as {@code Instrument24.transform} does.
*
@@ -293,6 +407,112 @@ public void trackedMethodPromotionSurvivesLaterUntrackedMethod() throws IOExcept
}
}
+ /**
+ * An application class that implements {@code Iterator.remove}, which the interface supplies as a
+ * default method. Used by {@link #testSuperclassOutranksInterfaceDefault}: JVMS 5.4.3.3 resolves
+ * a method against the superclass chain before the superinterfaces, so a call to {@code remove}
+ * on {@link IteratorWithSuperclassRemove} runs this implementation rather than the interface's
+ * default.
+ */
+ public static class RemoveInSuperclass {
+ /** Does nothing. */
+ public void remove() {}
+ }
+
+ /** A class whose {@code remove} comes from {@link RemoveInSuperclass}, not from the interface. */
+ public static class IteratorWithSuperclassRemove extends RemoveInSuperclass
+ implements java.util.Iterator