From 46993be9e6ddd0d912e4f6ca586a5382dbe35444 Mon Sep 17 00:00:00 2001 From: Mark Roberts Date: Fri, 4 Sep 2026 14:21:08 -0700 Subject: [PATCH 1/4] Do not let an abstract interface method outrank a real implementation (Suggested by Michael Ernst in review of PR 685) getDefiningInterface matched any declaration, and interface methods are abstract unless default, so an abstract declaration in a JDK interface could beat the implementation in an application superclass -- losing comparability through that call. JVMS 5.4.3.3 resolves against the superclass chain first, so an abstract declaration is now held aside and used only if the walk finds nothing. Renamed to getDeclaringInterface: it finds a declaration, not an implementation. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A1De2wQi77Zz4pnapvnFJz --- java/daikon/dcomp/DCInstrument.java | 77 ++++++++++++++++++++------ java/daikon/dcomp/DCInstrument24.java | 80 ++++++++++++++++++++------- 2 files changed, 120 insertions(+), 37 deletions(-) diff --git a/java/daikon/dcomp/DCInstrument.java b/java/daikon/dcomp/DCInstrument.java index 91dd052c6..3728bab39 100644 --- a/java/daikon/dcomp/DCInstrument.java +++ b/java/daikon/dcomp/DCInstrument.java @@ -463,8 +463,8 @@ public class DCInstrument extends InstructionListUtils { /** 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; @@ -2275,22 +2275,32 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { } /** - * Returns the interface class containing the implementation of the given method. The interfaces - * of {@code startClass} are recursively searched. + * 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. * * @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; @@ -2303,16 +2313,20 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { throw new Error("Unable to find class: " + interfaceName); } 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. A static method is never the target of an INVOKEVIRTUAL, and an + // abstract one declares the method without implementing it. + if (implementationsOnly && (jm.isAbstract() || jm.isStatic())) { + continue; + } return interfaceName; } } // 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 +2578,11 @@ private boolean isTargetInstrumented( } @ClassGetName String targetClassname = classname; + // An abstract interface method is only a declaration, so finding one does not settle + // where the method that runs comes from. JVMS 5.4.3.3 resolves a method against the + // superclass chain before the superinterfaces, so hold any such declaration aside and + // use it only if the walk up the superclasses finds nothing. + @ClassGetName @Nullable String declaringInterface = null; // Search this class for the target method. If not found, set targetClassname to // its superclass and try again. mainloop: @@ -2606,10 +2625,11 @@ private boolean isTargetInstrumented( } { - // no methods match - search this class's interfaces + // No declared method matches - search this class's interfaces. A default method is + // an implementation, so finding one settles the question. @ClassGetName String found; try { - found = getDefiningInterface(targetClass, methodName, paramTypes); + found = getDeclaringInterface(targetClass, methodName, paramTypes, true); } catch (Throwable e) { // We cannot locate or read the .class file, better assume it is not instrumented. targetInstrumented = false; @@ -2625,17 +2645,38 @@ private boolean isTargetInstrumented( } break; } + // Otherwise remember the first abstract declaration and keep walking; see the + // comment on declaringInterface above. + if (declaringInterface == null) { + try { + declaringInterface = + getDeclaringInterface(targetClass, methodName, paramTypes, false); + } catch (Throwable e) { + 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); + // The target class is Object; the search completed without finding an + // implementation. Fall back to any abstract declaration seen along the way. + if (declaringInterface != null) { + if (debugHandleInvoke) { + System.out.printf("only a declaration, in %s%n%n", declaringInterface); + } + if (BcelUtil.inJdk(declaringInterface)) { + targetInstrumented = false; + } + } else { + if (debugHandleInvoke) { + System.out.printf("Unable to locate method: %s%n%n", methodName); + } + targetInstrumented = false; } - targetInstrumented = false; break; } // Recurse looking in the superclass. diff --git a/java/daikon/dcomp/DCInstrument24.java b/java/daikon/dcomp/DCInstrument24.java index 07b2e5cf0..9899695a0 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,33 @@ 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 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. * * @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; @@ -2917,16 +2927,22 @@ private void add_exit( 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. A static method is never the target of an INVOKEVIRTUAL, and an + // abstract one declares the method without implementing it. + AccessFlags jmFlags = jm.flags(); + if (implementationsOnly + && (jmFlags.has(AccessFlag.ABSTRACT) || jmFlags.has(AccessFlag.STATIC))) { + continue; + } return interfaceName; } } // 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; @@ -3215,6 +3231,11 @@ private boolean isTargetInstrumented( } @BinaryName String targetClassname = classname; + // An abstract interface method is only a declaration, so finding one does not settle + // where the method that runs comes from. JVMS 5.4.3.3 resolves a method against the + // superclass chain before the superinterfaces, so hold any such declaration aside and + // use it only if the walk up the superclasses finds nothing. + @BinaryName @Nullable String declaringInterface = null; // Search this class for the target method. If not found, set targetClassname to // its superclass and try again. mainloop: @@ -3257,10 +3278,11 @@ private boolean isTargetInstrumented( } { - // no methods match - search this class's interfaces + // No declared method matches - search this class's interfaces. A default method is + // an implementation, so finding one settles the question. @BinaryName String found; try { - found = getDefiningInterface(targetClass, methodName, paramTypes); + found = getDeclaringInterface(targetClass, methodName, paramTypes, true); } catch (Throwable e) { // We cannot locate or read the .class file, better assume it is not instrumented. targetInstrumented = false; @@ -3276,16 +3298,36 @@ private boolean isTargetInstrumented( } break; } + // Otherwise remember the first abstract declaration and keep walking; see the + // comment on declaringInterface above. + if (declaringInterface == null) { + try { + declaringInterface = + getDeclaringInterface(targetClass, methodName, paramTypes, false); + } catch (Throwable e) { + 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); + // The target class was Object; the search completed without finding an + // implementation. Fall back to any abstract declaration seen along the way. + if (declaringInterface != null) { + if (debugHandleInvoke) { + System.out.printf("only a declaration, in %s%n%n", declaringInterface); + } + if (BcelUtil.inJdk(declaringInterface)) { + targetInstrumented = false; + } + } else { + if (debugHandleInvoke) { + System.out.printf("Unable to locate method: %s%n%n", methodName); + } + targetInstrumented = false; } - targetInstrumented = false; break; } From 2a9206e7e64873947fbe482f6926dd0ddca8126f Mon Sep 17 00:00:00 2001 From: Mark Roberts Date: Thu, 10 Sep 2026 16:41:35 -0700 Subject: [PATCH 2/4] Respond to CodeRabbit comments Search the whole superclass chain before any interface, per JVMS 5.4.3.3; previously a default method preempted it. Stop the interface search at an interface that reabstracts an inherited default, which hides it. Removing the provisional-declaration local also removes the redundant @Nullable that the Checker Framework flagged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A1De2wQi77Zz4pnapvnFJz --- java/daikon/dcomp/DCInstrument.java | 96 +++++++------ java/daikon/dcomp/DCInstrument24.java | 93 ++++++------ java/daikon/dcomp/DCInstrumentTest24.java | 165 ++++++++++++++++++++++ 3 files changed, 260 insertions(+), 94 deletions(-) diff --git a/java/daikon/dcomp/DCInstrument.java b/java/daikon/dcomp/DCInstrument.java index 3728bab39..3ea7805f6 100644 --- a/java/daikon/dcomp/DCInstrument.java +++ b/java/daikon/dcomp/DCInstrument.java @@ -2274,6 +2274,18 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { } } + /** + * 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 @ClassGetName String firstNonNull( + @Nullable @ClassGetName String first, @Nullable @ClassGetName 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. @@ -2312,19 +2324,29 @@ 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 (debugGetDeclaringInterface) { System.out.println(" " + jm.getName() + Arrays.toString(jm.getArgumentTypes())); } if (jm.getName().equals(methodName) && Arrays.equals(jm.getArgumentTypes(), paramTypes)) { - // We have a match. A static method is never the target of an INVOKEVIRTUAL, and an - // abstract one declares the method without implementing it. - if (implementationsOnly && (jm.isAbstract() || jm.isStatic())) { + // We have a match. A static method is never the target of an INVOKEVIRTUAL. + if (jm.isStatic()) { 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 = getDeclaringInterface(ji, methodName, paramTypes, implementationsOnly); if (foundAbove != null) { @@ -2578,11 +2600,10 @@ private boolean isTargetInstrumented( } @ClassGetName String targetClassname = classname; - // An abstract interface method is only a declaration, so finding one does not settle - // where the method that runs comes from. JVMS 5.4.3.3 resolves a method against the - // superclass chain before the superinterfaces, so hold any such declaration aside and - // use it only if the walk up the superclasses finds nothing. - @ClassGetName @Nullable String declaringInterface = null; + // 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: @@ -2624,58 +2645,39 @@ private boolean isTargetInstrumented( } } - { - // No declared method matches - search this class's interfaces. A default method is - // an implementation, so finding one settles the question. + // 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 = getDeclaringInterface(targetClass, methodName, paramTypes, true); + 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"); - } - if (BcelUtil.inJdk(found)) { - targetInstrumented = false; - } - break; - } - // Otherwise remember the first abstract declaration and keep walking; see the - // comment on declaringInterface above. - if (declaringInterface == null) { - try { - declaringInterface = - getDeclaringInterface(targetClass, methodName, paramTypes, false); - } catch (Throwable e) { - targetInstrumented = false; - break; + System.out.printf("Unable to locate method: %s%n%n", methodName); } - } - } - - // 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 an - // implementation. Fall back to any abstract declaration seen along the way. - if (declaringInterface != null) { + targetInstrumented = false; + } else { if (debugHandleInvoke) { - System.out.printf("only a declaration, in %s%n%n", declaringInterface); + System.out.printf("declared by interface %s%n%n", found); } - if (BcelUtil.inJdk(declaringInterface)) { + if (BcelUtil.inJdk(found)) { targetInstrumented = false; } - } else { - if (debugHandleInvoke) { - System.out.printf("Unable to locate method: %s%n%n", methodName); - } - targetInstrumented = false; } break; } diff --git a/java/daikon/dcomp/DCInstrument24.java b/java/daikon/dcomp/DCInstrument24.java index 9899695a0..33a48150d 100644 --- a/java/daikon/dcomp/DCInstrument24.java +++ b/java/daikon/dcomp/DCInstrument24.java @@ -2886,6 +2886,18 @@ private void add_exit( } } + /** + * 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. @@ -2924,6 +2936,7 @@ 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(); @@ -2931,16 +2944,24 @@ private void add_exit( System.out.println(" " + jmName + Arrays.toString(mtd.parameterArray())); } if (jmName.equals(methodName) && Arrays.equals(mtd.parameterArray(), paramTypes)) { - // We have a match. A static method is never the target of an INVOKEVIRTUAL, and an - // abstract one declares the method without implementing it. + // We have a match. A static method is never the target of an INVOKEVIRTUAL. AccessFlags jmFlags = jm.flags(); - if (implementationsOnly - && (jmFlags.has(AccessFlag.ABSTRACT) || jmFlags.has(AccessFlag.STATIC))) { + if (jmFlags.has(AccessFlag.STATIC)) { 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 = getDeclaringInterface(cm, methodName, paramTypes, implementationsOnly); if (foundAbove != null) { @@ -3231,13 +3252,11 @@ private boolean isTargetInstrumented( } @BinaryName String targetClassname = classname; - // An abstract interface method is only a declaration, so finding one does not settle - // where the method that runs comes from. JVMS 5.4.3.3 resolves a method against the - // superclass chain before the superinterfaces, so hold any such declaration aside and - // use it only if the walk up the superclasses finds nothing. - @BinaryName @Nullable String declaringInterface = null; // 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 @@ -3277,56 +3296,36 @@ private boolean isTargetInstrumented( } } - { - // No declared method matches - search this class's interfaces. A default method is - // an implementation, so finding one settles the question. + // 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 = getDeclaringInterface(targetClass, methodName, paramTypes, true); + 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"); - } - if (BcelUtil.inJdk(found)) { - targetInstrumented = false; - } - break; - } - // Otherwise remember the first abstract declaration and keep walking; see the - // comment on declaringInterface above. - if (declaringInterface == null) { - try { - declaringInterface = - getDeclaringInterface(targetClass, methodName, paramTypes, false); - } catch (Throwable e) { - targetInstrumented = false; - break; + System.out.printf("Unable to locate method: %s%n%n", methodName); } - } - } - - // Method not found; perhaps inherited from superclass. - if (targetClassname.equals("java.lang.Object")) { - // The target class was Object; the search completed without finding an - // implementation. Fall back to any abstract declaration seen along the way. - if (declaringInterface != null) { + targetInstrumented = false; + } else { if (debugHandleInvoke) { - System.out.printf("only a declaration, in %s%n%n", declaringInterface); + System.out.printf("declared by interface %s%n%n", found); } - if (BcelUtil.inJdk(declaringInterface)) { + if (BcelUtil.inJdk(found)) { targetInstrumented = false; } - } else { - 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..58c8fb6ae 100644 --- a/java/daikon/dcomp/DCInstrumentTest24.java +++ b/java/daikon/dcomp/DCInstrumentTest24.java @@ -153,6 +153,101 @@ 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.of() + : method.code().orElseThrow()) { + if (element instanceof InvokeInstruction invoke + && invoke.name().stringValue().equals(methodName)) { + List params = invoke.typeSymbol().parameterList(); + if (!params.isEmpty() + && params.get(params.size() - 1).displayName().equals("DCompMarker")) { + return true; + } + } + } + } + return false; + } + + /** + * Tests that a method declared by a superclass outranks a {@code default} method of an interface. + * JVMS 5.4.3.3 resolves a method against the class's own declaration, then the superclass chain, + * and only then the superinterfaces. + * + *

{@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 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 +388,76 @@ 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 { + @Override + public boolean hasNext() { + return false; + } + + @Override + public Object next() { + throw new java.util.NoSuchElementException(); + } + } + + /** + * An interface that reabstracts {@code Iterator.remove}. An implementor must define the method, + * so the interface's default no longer applies; see {@link + * #testReabstractionHidesInheritedDefault}. + */ + public interface ReabstractsRemove extends java.util.Iterator { + @Override + void remove(); + } + + /** An interface that inherits the reabstraction and declares nothing of its own. */ + public interface InheritsReabstraction extends ReabstractsRemove {} + + /** + * An abstract class that declares no {@code remove} of its own, so resolving a call to it reaches + * the interfaces. The call site is typed as this class rather than as the interface, because the + * resolution being tested runs only for INVOKEVIRTUAL. + */ + public abstract static class AbstractReabstracted implements InheritsReabstraction {} + + /** Calls {@code remove} on a class that inherits it from an application superclass. */ + public static class CallsSuperclassRemove { + /** + * Calls {@code remove}. + * + * @param it the receiver + */ + public void call(IteratorWithSuperclassRemove it) { + it.remove(); + } + } + + /** Calls {@code remove} through an interface that reabstracts the JDK's default. */ + public static class CallsReabstractedRemove { + /** + * Calls {@code remove}. + * + * @param it the receiver + */ + public void call(AbstractReabstracted it) { + it.remove(); + } + } + /** A small class that the tests instrument. */ public static class Sample { From 9beec99d050be5fc7dd3c46606e30917548c1171 Mon Sep 17 00:00:00 2001 From: Mark Roberts Date: Fri, 11 Sep 2026 08:22:27 -0700 Subject: [PATCH 3/4] Fix Checker Framework lock errors Iterator's methods are annotated with @GuardSatisfied receivers, so an override must match. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A1De2wQi77Zz4pnapvnFJz --- java/daikon/dcomp/DCInstrumentTest24.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/java/daikon/dcomp/DCInstrumentTest24.java b/java/daikon/dcomp/DCInstrumentTest24.java index 58c8fb6ae..3d15f228f 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; @@ -404,12 +405,12 @@ public void remove() {} public static class IteratorWithSuperclassRemove extends RemoveInSuperclass implements java.util.Iterator { @Override - public boolean hasNext() { + public boolean hasNext(@GuardSatisfied IteratorWithSuperclassRemove this) { return false; } @Override - public Object next() { + public Object next(@GuardSatisfied IteratorWithSuperclassRemove this) { throw new java.util.NoSuchElementException(); } } @@ -421,7 +422,7 @@ public Object next() { */ public interface ReabstractsRemove extends java.util.Iterator { @Override - void remove(); + void remove(@GuardSatisfied ReabstractsRemove this); } /** An interface that inherits the reabstraction and declares nothing of its own. */ From ee9c80e72b2df1ee8bb713d1abfcd52ff8b959a4 Mon Sep 17 00:00:00 2001 From: Mark Roberts Date: Fri, 11 Sep 2026 09:38:40 -0700 Subject: [PATCH 4/4] Respond to CodeRabbit comments Skip private interface methods when resolving a virtual call: they are not inherited and are never the target of an INVOKEVIRTUAL. Document, rather than fix, that the search returns the first matching interface rather than the maximally specific one. The caller uses the answer only to decide whether the target is instrumented, so the effect is lost precision rather than a broken call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A1De2wQi77Zz4pnapvnFJz --- java/daikon/dcomp/DCInstrument.java | 13 +++++- java/daikon/dcomp/DCInstrument24.java | 13 +++++- java/daikon/dcomp/DCInstrumentTest24.java | 54 +++++++++++++++++++++++ 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/java/daikon/dcomp/DCInstrument.java b/java/daikon/dcomp/DCInstrument.java index 3ea7805f6..15094436b 100644 --- a/java/daikon/dcomp/DCInstrument.java +++ b/java/daikon/dcomp/DCInstrument.java @@ -2295,6 +2295,14 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { * 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 @@ -2330,8 +2338,9 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) { System.out.println(" " + jm.getName() + Arrays.toString(jm.getArgumentTypes())); } if (jm.getName().equals(methodName) && Arrays.equals(jm.getArgumentTypes(), paramTypes)) { - // We have a match. A static method is never the target of an INVOKEVIRTUAL. - if (jm.isStatic()) { + // 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()) { diff --git a/java/daikon/dcomp/DCInstrument24.java b/java/daikon/dcomp/DCInstrument24.java index 33a48150d..2197fa01a 100644 --- a/java/daikon/dcomp/DCInstrument24.java +++ b/java/daikon/dcomp/DCInstrument24.java @@ -2907,6 +2907,14 @@ private void add_exit( * 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 @@ -2944,9 +2952,10 @@ private void add_exit( System.out.println(" " + jmName + Arrays.toString(mtd.parameterArray())); } if (jmName.equals(methodName) && Arrays.equals(mtd.parameterArray(), paramTypes)) { - // We have a match. A static method is never the target of an INVOKEVIRTUAL. + // 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)) { + if (jmFlags.has(AccessFlag.STATIC) || jmFlags.has(AccessFlag.PRIVATE)) { continue; } if (implementationsOnly && jmFlags.has(AccessFlag.ABSTRACT)) { diff --git a/java/daikon/dcomp/DCInstrumentTest24.java b/java/daikon/dcomp/DCInstrumentTest24.java index 3d15f228f..3284511d4 100644 --- a/java/daikon/dcomp/DCInstrumentTest24.java +++ b/java/daikon/dcomp/DCInstrumentTest24.java @@ -231,6 +231,24 @@ public void testSuperclassOutranksInterfaceDefault() throws IOException { 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 @@ -459,6 +477,42 @@ public void call(AbstractReabstracted it) { } } + /** + * An interface with a private {@code remove}, matching the name and descriptor of the {@code + * default} that {@code java.util.Iterator} supplies. A private interface method is not inherited + * and is never the target of an INVOKEVIRTUAL, so resolution must skip it and go on to the JDK's + * default; see {@link #testPrivateInterfaceMethodIsNotADeclaration}. + */ + public interface PrivateRemove { + /** Unrelated to any call being resolved; it exists only to occupy the name. */ + private void remove() {} + + /** Uses the private method, so that it is not flagged as unused. */ + default void usePrivateRemove() { + remove(); + } + } + + /** + * Implements the private-method interface first and {@code java.util.Iterator} second, so a + * search that does not skip private methods finds the wrong one. Abstract, so that no class in + * the chain declares {@code remove} and the search reaches the interfaces. + */ + public abstract static class PrivateThenJdkDefault + implements PrivateRemove, java.util.Iterator {} + + /** Calls {@code remove} on a class whose first interface declares it private. */ + public static class CallsRemoveThroughPrivate { + /** + * Calls {@code remove}. + * + * @param x the receiver + */ + public void call(PrivateThenJdkDefault x) { + x.remove(); + } + } + /** A small class that the tests instrument. */ public static class Sample {