diff --git a/.gitignore b/.gitignore index eeedd4c33..734f07610 100644 --- a/.gitignore +++ b/.gitignore @@ -47,3 +47,10 @@ gradle-wrapper.properties /.java.versions compile.log .worktrees/ + +# OS / editor / tool noise +.DS_Store +.cachebro/ +gradle/gradle-daemon-jvm.properties +# JVM thread-dump artifacts from stalled integration tests +javacore.*.txt diff --git a/btrace-agent/src/main/java/io/btrace/instr/ClassFilter.java b/btrace-agent/src/main/java/io/btrace/instr/ClassFilter.java index 77bf8fab3..1a07171f4 100644 --- a/btrace-agent/src/main/java/io/btrace/instr/ClassFilter.java +++ b/btrace-agent/src/main/java/io/btrace/instr/ClassFilter.java @@ -174,15 +174,13 @@ public static boolean isSubTypeOf(String typeA, ClassLoader loader, String... ty return true; } ClassCache cache = ClassCache.getInstance(); - if (cache == null) { - // Reentrant class loading can observe the holder before INSTANCE assignment completes. - return false; - } - ClassInfo ci = cache.get(loader, typeA); + // ClassCache.getInstance() uses the holder idiom (static final in a nested class), so + // cache is never null under normal JLS class-initialization semantics. Fold the + // defensive cache-unavailable case together with the not-cached case: either way we + // cannot resolve the supertypes, so conservatively report "not a subtype" and let the + // caller fall back to a no-op transform or a different check. + ClassInfo ci = cache != null ? cache.get(loader, typeA) : null; if (ci == null) { - // ClassInfo not present in the cache (or cache temporarily unavailable due to a - // classloader-init race). Conservatively report "not a subtype" rather than NPE; - // the caller can fall back to a no-op transform or a different check. return false; } Collection sTypesInfo = ci.getSupertypes(false); diff --git a/btrace-agent/src/main/java24/io/btrace/instr/ClassFileApiBackend.java b/btrace-agent/src/main/java24/io/btrace/instr/ClassFileApiBackend.java index 488ffd050..58c65e15c 100644 --- a/btrace-agent/src/main/java24/io/btrace/instr/ClassFileApiBackend.java +++ b/btrace-agent/src/main/java24/io/btrace/instr/ClassFileApiBackend.java @@ -113,6 +113,7 @@ public final class ClassFileApiBackend implements InstrumentationBackend { ClassDesc.of("java.lang.String")); private static final ClassDesc INDY_DISPATCHER = ClassDesc.of("io.btrace.runtime.IndyDispatcher"); + private static final ClassDesc CD_OBJECT = ClassDesc.of("java.lang.Object"); private static final ClassDesc CD_METHOD_TRACKER = ClassDesc.ofInternalName("io/btrace/instr/MethodTracker"); private static final MethodTypeDesc HIT_DESC = MethodTypeDesc.ofDescriptor("(I)Z"); @@ -705,6 +706,8 @@ private static CodeTransform buildCodeTransform( Map> catchHandlerTypes, boolean[] anyMatch) { + Type[] methodArgTypes = Type.getArgumentTypes(methodDesc); + // hasDurationReturn: RETURN probes need duration → compute nanoTime on each RETURN path boolean hasDurationReturn = returnHandlers.stream().anyMatch(ph -> ph.om.getDurationParameter() != -1); @@ -819,12 +822,12 @@ public void accept(CodeBuilder cb, CodeElement ce) { Label skipLabel = cb.newLabel(); cb.iload(sHitSlot[0]); cb.ifeq(skipLabel); - emitProbeCall(cb, ph, javaClassName, methodName, isStatic, true, -1, TypeKind.VOID, -1); + emitProbeCall(cb, ph, javaClassName, methodName, isStatic, true, methodArgTypes, -1, TypeKind.VOID, -1); cb.labelBinding(skipLabel); } else { emitWithSamplingGuard( cb, ph, samplingMethodId, - () -> emitProbeCall(cb, ph, javaClassName, methodName, isStatic, true, -1, TypeKind.VOID, -1)); + () -> emitProbeCall(cb, ph, javaClassName, methodName, isStatic, true, methodArgTypes, -1, TypeKind.VOID, -1)); } } if (isSynchronizedMethod) { @@ -893,8 +896,8 @@ public void accept(CodeBuilder cb, CodeElement ce) { emitWithSamplingGuard( cb, ph, samplingMethodId, () -> emitProbeCall( - cb, ph, javaClassName, methodName, isStatic, false, - fLocalRetValSlot, methodReturnType, fReturnKind, fLocalDurationSlot, null)); + cb, ph, javaClassName, methodName, isStatic, false, methodArgTypes, + fLocalRetValSlot, methodReturnType, fReturnKind, fLocalDurationSlot)); } } if (adaptiveReturnHandlers != null) { @@ -905,8 +908,8 @@ public void accept(CodeBuilder cb, CodeElement ce) { cb.invokestatic(CD_METHOD_TRACKER, "updateEndTs", UPDATE_END_TS_DESC); for (ProbeHandler ph : adaptiveReturnHandlers) { emitProbeCall( - cb, ph, javaClassName, methodName, isStatic, false, - fLocalRetValSlot, methodReturnType, fReturnKind, fLocalDurationSlot, null); + cb, ph, javaClassName, methodName, isStatic, false, methodArgTypes, + fLocalRetValSlot, methodReturnType, fReturnKind, fLocalDurationSlot); } cb.labelBinding(skipAdaptive); } @@ -2655,6 +2658,7 @@ private static boolean emitNewArrayProbe( methodName, isStatic, false, + null, ctx.returnSlot, ctx.arrayType, TypeKind.REFERENCE, @@ -2675,8 +2679,10 @@ private static boolean canEmitCallProbe(ProbeHandler ph, InvokeInstruction ii, C String rawDesc = om.getTargetDescriptor().replace(Constants.ANYTYPE_DESC, Constants.OBJECT_DESC); Type[] handlerArgTypes = Type.getArgumentTypes(rawDesc); + // Original (pre-replacement) descriptor: needed to recognise AnyType[] aggregate params, + // which the replaced descriptor flattens to Object[]. + Type[] originalHandlerArgTypes = Type.getArgumentTypes(om.getTargetDescriptor()); Type[] callArgTypes = Type.getArgumentTypes(ii.type().stringValue()); - boolean staticCall = ii.opcode() == Opcode.INVOKESTATIC; Type ownerType = Type.getObjectType(ii.owner().asInternalName()); for (int i = 0; i < handlerArgTypes.length; i++) { if (i == om.getSelfParameter() @@ -2725,6 +2731,11 @@ private static boolean canEmitCallProbe(ProbeHandler ph, InvokeInstruction ii, C } continue; } + // AnyType[] aggregate: packages all call args into Object[] regardless of their + // individual types (mirrors ASM MethodCallInstrumentor + AnyTypeArgProvider). + if (TypeUtils.isAnyTypeArray(originalHandlerArgTypes[i])) { + continue; + } int callArgIndex = callArgumentIndex(om, i); if (callArgIndex < 0 || callArgIndex >= callArgTypes.length) { return false; @@ -3300,6 +3311,7 @@ private static boolean emitProbeCall( methodName, isStatic, isEntry, + null, retValSlot, returnType, returnKind, @@ -3315,6 +3327,77 @@ private static boolean emitProbeCall( null); } + // ENTRY convenience: enclosing-method args available, no return value, no call context. + private static boolean emitProbeCall( + CodeBuilder cb, + ProbeHandler ph, + String javaClassName, + String methodName, + boolean isStatic, + boolean isEntry, + Type[] methodArgTypes, + int retValSlot, + TypeKind returnKind, + int durationSlot) { + return emitProbeCall( + cb, + ph, + javaClassName, + methodName, + isStatic, + isEntry, + methodArgTypes, + retValSlot, + null, + returnKind, + durationSlot, + null, + -1, + null, + null, + null, + null, + null, + null, + null); + } + + // RETURN convenience: enclosing-method args available, with return value + duration slots. + private static boolean emitProbeCall( + CodeBuilder cb, + ProbeHandler ph, + String javaClassName, + String methodName, + boolean isStatic, + boolean isEntry, + Type[] methodArgTypes, + int retValSlot, + Type returnType, + TypeKind returnKind, + int durationSlot) { + return emitProbeCall( + cb, + ph, + javaClassName, + methodName, + isStatic, + isEntry, + methodArgTypes, + retValSlot, + returnType, + returnKind, + durationSlot, + null, + -1, + null, + null, + null, + null, + null, + null, + null); + } + private static boolean emitProbeCall( CodeBuilder cb, ProbeHandler ph, @@ -3322,6 +3405,7 @@ private static boolean emitProbeCall( String methodName, boolean isStatic, boolean isEntry, + Type[] methodArgTypes, int retValSlot, Type returnType, TypeKind returnKind, @@ -3340,6 +3424,10 @@ private static boolean emitProbeCall( String rawDesc = om.getTargetDescriptor().replace(Constants.ANYTYPE_DESC, Constants.OBJECT_DESC); Type[] argTypes = Type.getArgumentTypes(rawDesc); + // Original (pre-replacement) handler arg types: needed to detect AnyType[] aggregate + // params, which the replaced descriptor flattens to Object[]. ENTRY/RETURN ordinary method + // args and CALL ordinary call args are only loaded when methodArgTypes / callContext is set. + Type[] originalArgTypes = Type.getArgumentTypes(om.getTargetDescriptor()); // Pre-validate: every argument must be satisfiable before we push anything onto the stack. // An early return mid-loop would leave orphaned stack values, causing a VerifyError. @@ -3373,7 +3461,15 @@ private static boolean emitProbeCall( || (typeCheckContext != null && ordinaryArgumentIndex(om, i) == 0) || (newArrayContext != null && ordinaryArgumentIndex(om, i) == 0) || (newArrayContext != null && ordinaryArgumentIndex(om, i) == 1) - || (callArgIndex >= 0 && callArgIndex < callContext.argumentTypes.length); + || (callArgIndex >= 0 && callArgIndex < callContext.argumentTypes.length) + // ENTRY/RETURN ordinary enclosing-method args (typed or AnyType[] aggregate). + || (methodArgTypes != null + && TypeUtils.isAnyTypeArray(originalArgTypes[i])) + || (methodArgTypes != null + && ordinaryArgumentIndex(om, i) >= 0 + && ordinaryArgumentIndex(om, i) < methodArgTypes.length) + // CALL AnyType[] aggregate packages all call args into Object[]. + || (callContext != null && TypeUtils.isAnyTypeArray(originalArgTypes[i])); if (!satisfiable) { log.debug( "ClassFileApiBackend: skipping handler {}.{} — arg {} cannot be satisfied", @@ -3467,6 +3563,17 @@ && typeKind(argTypes[i]) == TypeKind.REFERENCE) { cb.ldc(newArrayContext.extName); } else if (newArrayContext != null && ordinaryArgumentIndex(om, i) == 1) { cb.ldc(newArrayContext.dims); + } else if (TypeUtils.isAnyTypeArray(originalArgTypes[i]) + && (methodArgTypes != null || callContext != null)) { + // AnyType[] aggregate: package all enclosing-method args (ENTRY/RETURN) or all call + // args (CALL) into an Object[], boxing primitives. Mirrors ASM AnyTypeArgProvider. + emitAnyTypeArray(cb, isStatic, methodArgTypes, callContext); + } else if (methodArgTypes != null) { + // Typed ordinary enclosing-method argument (ENTRY/RETURN). Loaded from its fixed + // method-local slot; no boxing — the handler type must match the method arg type + // (validated upstream), matching the existing CALL typed-arg path. + int ordIdx = ordinaryArgumentIndex(om, i); + cb.loadLocal(typeKind(methodArgTypes[ordIdx]), methodArgSlot(isStatic, methodArgTypes, ordIdx)); } else if (callContext != null) { int callArgIndex = callArgumentIndex(om, i); cb.loadLocal( @@ -3490,6 +3597,47 @@ && typeKind(argTypes[i]) == TypeKind.REFERENCE) { return true; } + // Fixed local-variable slot for the ordinary enclosing-method argument at + // ordinaryIndex (0-based). `this` occupies slot 0 for instance methods. + private static int methodArgSlot(boolean isStatic, Type[] methodArgTypes, int ordinaryIndex) { + int slot = isStatic ? 0 : 1; + for (int j = 0; j < ordinaryIndex; j++) { + slot += methodArgTypes[j].getSize(); + } + return slot; + } + + // Package all enclosing-method args (ENTRY/RETURN, methodArgTypes != null) or all call args + // (CALL, callContext != null) into an Object[], boxing primitives. Mirrors ASM + // MethodInstrumentor.AnyTypeArgProvider.doProvide. + private static void emitAnyTypeArray( + CodeBuilder cb, boolean isStatic, Type[] methodArgTypes, CallContext callContext) { + boolean useMethod = methodArgTypes != null; + Type[] srcTypes = useMethod ? methodArgTypes : callContext.argumentTypes; + cb.ldc(srcTypes.length); + cb.anewarray(CD_OBJECT); + for (int j = 0; j < srcTypes.length; j++) { + TypeKind tk = typeKind(srcTypes[j]); + int slot = useMethod ? methodArgSlot(isStatic, methodArgTypes, j) : callContext.argumentSlots[j]; + cb.dup(); + cb.ldc(j); + cb.loadLocal(tk, slot); + boxIfPrimitive(cb, srcTypes[j], tk); + cb.arrayStore(TypeKind.REFERENCE); + } + } + + // Box a primitive on the stack to its wrapper; no-op for references. The JVMS verifier + // collapses byte/short/char/boolean to int, so valueOf(B/S/C/Z) accepts the int on stack. + private static void boxIfPrimitive(CodeBuilder cb, Type t, TypeKind tk) { + if (tk == TypeKind.INT + || tk == TypeKind.LONG + || tk == TypeKind.FLOAT + || tk == TypeKind.DOUBLE) { + boxPrimitiveReturn(cb, t, tk); + } + } + private static void boxPrimitiveReturn(CodeBuilder cb, Type returnType, TypeKind returnKind) { String primitiveDesc = returnType != null ? returnType.getDescriptor() : primitiveDesc(returnKind); String wrapperInternalName = wrapperInternalName(primitiveDesc); diff --git a/btrace-agent/src/test/java/io/btrace/instr/ClassFileApiBackendTest.java b/btrace-agent/src/test/java/io/btrace/instr/ClassFileApiBackendTest.java index 687b8d720..bc61cd6c3 100644 --- a/btrace-agent/src/test/java/io/btrace/instr/ClassFileApiBackendTest.java +++ b/btrace-agent/src/test/java/io/btrace/instr/ClassFileApiBackendTest.java @@ -149,6 +149,189 @@ void entryProbeSkippedWhenTypeConstraintMismatches() { assertNull(result, "Expected mismatched type-constrained ENTRY probe to be skipped"); } + // --- Task 1: ENTRY ordinary enclosing-method arguments --- + + @Test + void entryProbePassesTypedMethodArgs() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithInstanceCall(70, "com/example/Target", "callTopLevel"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTopLevel", + Kind.ENTRY, + "(Ljava/lang/String;J)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected ENTRY probe capturing typed method args to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTopLevel", "$btrace$"); + assertEquals("(Ljava/lang/String;J)V", desc); + } + + @Test + void entryProbePassesTypedMethodArgsStatic() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithStaticCall(70, "com/example/Target", "callTopLevel"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTopLevel", + Kind.ENTRY, + "(Ljava/lang/String;J)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected ENTRY probe on static method with typed args to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTopLevel", "$btrace$"); + assertEquals("(Ljava/lang/String;J)V", desc); + } + + // --- Task 3 (ENTRY/RETURN share): AnyType[] aggregate packaging --- + + @Test + void entryProbePackagesAnyTypeArrayArgs() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithInstanceCall(70, "com/example/Target", "callTopLevel"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTopLevel", + Kind.ENTRY, + "([Lio/btrace/core/types/AnyType;)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected ENTRY AnyType[] probe to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTopLevel", "$btrace$"); + assertEquals("([Ljava/lang/Object;)V", desc); + assertTrue( + countOpcode(readable, "callTopLevel", Opcodes.ANEWARRAY) >= 1, + "Expected anewarray for AnyType[] aggregate packaging"); + } + + // --- Task 2: RETURN ordinary enclosing-method arguments --- + + @Test + void returnProbePassesTypedMethodArgs() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithInstanceCall(70, "com/example/Target", "callTopLevel"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTarget", + Kind.RETURN, + "(Ljava/lang/String;J)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected RETURN probe capturing typed method args to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTarget", "$btrace$"); + assertEquals("(Ljava/lang/String;J)V", desc); + } + + @Test + void returnProbePackagesAnyTypeArrayArgs() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithInstanceCall(70, "com/example/Target", "callTopLevel"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTarget", + Kind.RETURN, + "([Lio/btrace/core/types/AnyType;)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected RETURN AnyType[] probe to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTarget", "$btrace$"); + assertEquals("([Ljava/lang/Object;)V", desc); + assertTrue( + countOpcode(readable, "callTarget", Opcodes.ANEWARRAY) >= 1, + "Expected anewarray for AnyType[] aggregate packaging"); + } + + // --- Task 3 (CALL): AnyType[] aggregate packaging of call arguments --- + + @Test + void callProbePackagesAnyTypeArrayArgs() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithInstanceCall(70, "com/example/Target", "callTopLevel"); + Location location = new Location(); + location.setValue(Kind.CALL); + location.setWhere(Where.BEFORE); + location.setClazz("com.example.Target"); + location.setMethod("callTarget"); + location.setType("(Ljava/lang/String;J)J"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTopLevel", + location, + "([Lio/btrace/core/types/AnyType;)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected CALL AnyType[] probe to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTopLevel", "$btrace$"); + assertEquals("([Ljava/lang/Object;)V", desc); + assertTrue( + countOpcode(readable, "callTopLevel", Opcodes.ANEWARRAY) >= 1, + "Expected anewarray for AnyType[] aggregate packaging of call args"); + } + + @Test + void callProbePackagesAnyTypeArrayArgsEmpty() { + requireJdk26ForVersion70(); + byte[] classBytes = buildClassWithVoidCall(70, "com/example/Target", "callTopLevel"); + Location location = new Location(); + location.setValue(Kind.CALL); + location.setWhere(Where.BEFORE); + location.setClazz("com.example.Target"); + location.setMethod("callTargetVoid"); + location.setType("()V"); + BTraceProbe probe = + buildStubProbe( + "com/example/MyTrace", + "com.example.Target", + "callTopLevel", + location, + "([Lio/btrace/core/types/AnyType;)V"); + + byte[] result = + BackendSelector.select(70).instrument(null, classBytes, Collections.singletonList(probe)); + + assertNotNull(result, "Expected CALL AnyType[] probe on a no-arg call to be injected"); + byte[] readable = patchVersion(result, 65); + assertLoadsWithoutVerifyError(readable, "com.example.Target"); + String desc = getInvokeDynamicDescriptor(readable, "callTopLevel", "$btrace$"); + assertEquals("([Ljava/lang/Object;)V", desc); + } + @Test void returnProbeInjectedBeforeReturn() { requireJdk26ForVersion70(); @@ -2526,6 +2709,28 @@ private static byte[] patchVersion(byte[] bytes, int majorVersion) { return copy; } + // Define the (version-65-patched) instrumented bytes via an isolated ClassLoader so the + // JVM verifier runs over the generated bytecode — catches stack/frame corruption that + // descriptor/opcode presence checks miss. The invokedynamic bootstrap (IndyDispatcher) is + // not resolved during verification, so it need not be on the null-parent classloader. + private static void assertLoadsWithoutVerifyError(byte[] readable, String className) { + assertDoesNotThrow( + () -> { + ClassLoader cl = + new ClassLoader(null) { + @Override + protected Class findClass(String name) throws ClassNotFoundException { + if (name.equals(className)) { + return defineClass(name, readable, 0, readable.length); + } + throw new ClassNotFoundException(name); + } + }; + cl.loadClass(className); + }, + "Instrumented class must load without VerifyError"); + } + /** * Returns the running JDK's major version using only Java 8-compatible APIs. {@code * java.specification.version} is {@code "1.8"} on Java 8 and {@code "9"}, {@code "10"}, … {@code diff --git a/docs/superpowers/plans/2026-07-06-classfileapi-remaining-gaps.md b/docs/superpowers/plans/2026-07-06-classfileapi-remaining-gaps.md new file mode 100644 index 000000000..94782a104 --- /dev/null +++ b/docs/superpowers/plans/2026-07-06-classfileapi-remaining-gaps.md @@ -0,0 +1,378 @@ +# ClassFile API Backend — Remaining Gaps Plan + +> **Status:** Post-merge follow-up to PR #843 (squash-merged into `develop` as +> `9f39a250`). PR #843 implemented ClassFile API parity for every `@OnMethod` location +> `Kind` (ENTRY, RETURN, CALL, LINE, FIELD_GET/SET, ARRAY_GET/SET, CHECKCAST, +> INSTANCEOF, THROW, CATCH, ERROR, NEWARRAY, NEW, SYNC_ENTRY/EXIT, plus sampled/level +> guards). This plan covers the pieces that work left open **plus** a parity gap it +> under-scoped: ordinary method-argument capture on ENTRY/RETURN and `AnyType[]` +> aggregate packaging. +> +> Note: the planning docs produced during PR #843 (`2026-06-04-…`, `2026-06-05-…`) +> were removed from `develop` after merge; this document is self-contained. + +**Goal:** Close the remaining ClassFile API backend gaps versus the ASM backend, sweep +the open bot review findings on PR #843, and finish the Phase-1/14 housekeeping the +implementation plan deferred. + +**Primary files:** +- `btrace-agent/src/main/java24/io/btrace/instr/ClassFileApiBackend.java` +- `btrace-agent/src/test/java/io/btrace/instr/ClassFileApiBackendTest.java` +- `btrace-agent/src/main/java/io/btrace/instr/ClassCache.java` +- `integration-tests/src/test/btrace/ClassFileApiFeatureSmokeTest.java` +- ASM parity reference: `btrace-agent/src/main/java/io/btrace/instr/Instrumentor.java` + and `btrace-agent/src/main/java/io/btrace/instr/MethodInstrumentor.java` + (`AnyTypeArgProvider`, `loadArguments`, `anytypeArg`). + +**Test environment:** + +```sh +export JAVA_HOME=$HOME/.sdkman/candidates/java/26-tem +export GRADLE_USER_HOME=$(pwd)/.gradle-user +``` + +Do not stream Gradle logs. Redirect to `/tmp/...` and summarize with `rg` +(prefer the build-summarize skill). + +--- + +## Evidence Summary (what is actually missing, with references) + +### Gap A — Ordinary method arguments on ENTRY are not loaded + +`ClassFileApiBackend.emitProbeCall` (around line 3320–3490) loads handler parameters in +a dispatch loop. For ENTRY probes it is called with `callContext == null` and +`lineNumber == -1` (line 822). The load loop has branches only for special params and +for context-specific ordinary params (line/field/array/typecheck/newarray/call). There +is **no branch that loads an ordinary method argument for an ENTRY handler**. + +The pre-validation loop (line 3340–3380) therefore marks any ordinary ENTRY arg as +`!satisfiable` (callArgIndex=-1, lineArgIndex=-1, no context matches) and the handler is +silently skipped: + +```java +log.debug("ClassFileApiBackend: skipping handler {}.{} — arg {} cannot be satisfied", ...); +return false; +``` + +ASM parity: `Instrumentor` ENTRY case calls +`loadArguments(vr, actionArgTypes, isStatic(), actionArgs)` (Instrumentor.java ~line 129), +which loads each ordinary arg from its local slot (`MethodInstrumentor.loadArguments`, +line 180–200). ENTRY probes capturing method arguments (`void onEntry(String a, int b)`) +are a standard, long-supported ASM feature. + +Test coverage: every ENTRY test uses a `()V` handler descriptor +(`ClassFileApiBackendTest` lines 88, 105, 297; `ClassFileApiEntryTest.java` uses only +`@ProbeMethodName`). The gap is latent and untested. + +**Verdict:** Real parity gap. ENTRY handlers declaring ordinary method args (typed or +`AnyType[]`) are silently dropped on Java 26+ class files. + +### Gap B — Ordinary method arguments on RETURN are not loaded + +Same mechanism as Gap A. `emitProbeCall` for RETURN is called with `callContext == null` +(line 895, 907). Ordinary RETURN args hit no load branch and fail pre-validation → skip. + +ASM parity: `Instrumentor` RETURN case validates against +`Type.getArgumentTypes(getDescriptor())` (Instrumentor.java ~line 83) and calls +`loadArguments(vr, actionArgTypes, isStatic(), actionArgs)` (~line 943), so RETURN +handlers may capture enclosing-method arguments. + +Test coverage: all RETURN tests use `()V` (line 157, 300). Latent gap. + +**Verdict:** Real parity gap, same shape as Gap A. + +### Gap C — `AnyType[]` aggregate is not packaged for CALL (or ENTRY/RETURN) + +ASM packages all call/method args into an `Object[]` when a handler declares +`AnyType[]` (`MethodInstrumentor.AnyTypeArgProvider`, lines 636–665): + +```java +asm.push(myArgTypes.length); +asm.newArray(Constants.OBJECT_TYPE); +for (int j = 0; j < myArgTypes.length; j++) { + asm.dup().push(j).loadLocal(argType, argPtr).box(argType).arrayStore(OBJECT_TYPE); + argPtr += argType.getSize(); +} +``` + +The CALL path in `emitProbeCall` (line 3481–3484) loads a single call arg per ordinary +param: + +```java +int callArgIndex = callArgumentIndex(om, i); +cb.loadLocal(typeKind(callContext.argumentTypes[callArgIndex]), + callContext.argumentSlots[callArgIndex]); +``` + +There is no `AnyType[]` aggregate branch. Because the backend computes `argTypes` from +`om.getTargetDescriptor().replace(ANYTYPE_DESC, OBJECT_DESC)` (line 3327), an `AnyType[]` +param appears as `Object[]` and is indistinguishable from a literal `Object[]` param — +so the backend cannot currently tell it should emit the packaging loop. + +Detection is available: `om.getTargetDescriptor()` returns the **original** descriptor +(the `.replace(...)` call the backend itself performs proves this), so an `AnyType[]` +param is `[` + `Constants.ANYTYPE_DESC` in the raw descriptor and can be detected before +replacement. `TypeUtils.isAnyTypeArray` / `anyTypeArray` (TypeUtils.java line 29) give the +canonical check used on the ASM side. + +Pre-validation today: an `AnyType[]` ordinary CALL arg gets `callArgIndex=0`, passes the +`callArgIndex < callContext.argumentTypes.length` check, then at load time one call arg +is pushed where an `Object[]` is expected → **verifier error or wrong values**. + +Test coverage: zero `AnyType[]` references in `ClassFileApiBackendTest.java`; the +integration `ClassFileApiFeatureSmokeTest.onCall` uses a single typed ordinary arg +(`Object key`), which works, but no `AnyType[]` handler is exercised. + +**Verdict:** Real parity gap; actively produces bad bytecode when a CALL handler uses +`AnyType[]`. ENTRY/RETURN `AnyType[]` is folded into Gaps A/B. + +### Gap D — Phase 1 shared validation helpers (deferred) + +The implementation plan left Phase 1 unchecked: location-family grouping for +`collectHandlers` (today a ~30-arm if/else, line 190–224) and extraction of common +handler validation for `@Self`/`@ProbeClassName`/`@ProbeMethodName`/`@TargetInstance`/ +`@TargetMethodOrField`/`@Return`/`@Duration`, which is currently duplicated across the +per-kind emit paths. This is a refactor, not a behavior gap, but it is the documented +reason Gaps A/B/C were easy to miss: ordinary-arg handling has no single home. + +### Gap E — Dead "unsupported kind" debug branch + +`collectHandlers` (line 206–224) handles every `Kind` value explicitly; the trailing + +```java +else log.debug("Skipping unsupported probe kind {} for class {}", kind, javaClassName); +``` + +is now unreachable. Phase 14 asks for its removal (keep explicit debug skips only for +intentionally-unsupported edge cases — e.g. `@Duration` on SYNC, BEFORE on synchronized +methods, NEW AFTER+constructor safety skips, which are documented elsewhere). + +### Gap F — Open bot review findings on PR #843 + +Unresolved `github-code-quality` comments: + +1. **Unread local `boolean staticCall`** in `ClassFileApiBackend.canEmitCallProbe` + (line 2679). Confirmed: the local is assigned but never read in that method (only + `ownerType` is used). The field `CallContext.staticCall` (line 287) is genuinely + read (line 1917, 3434) — the bot finding is about the dead *local* in + `canEmitCallProbe`, not the field. +2. **~~Two "useless null check" findings in `ClassCache.java`~~ — STALE/MOOT.** + Recheck (post-rebase) found the 66e2bfd3 `ClassCache.getInstance()` DCL fix was + **dropped at merge**: the squash `9f39a250` never touched `ClassCache.java`, so those + findings reference code absent from `develop`. What landed from 66e2bfd3 is the + `ClassFilter.isSubTypeOf` defensive guard; its residual `if (cache == null)` arm is + JLS-dead (holder idiom) and not bot-flagged. Resolved in Task 7 by folding it into the + `ci == null` no-match path. +3. **~8 "useless parameter" findings in `ClassFileApiFeatureSmokeTest.java`**: unused + handler params (`value`, `array`, `lock`, `key`, `set`, `exception` across + `onArrayGet`, `onNewArray`, `onSyncEntry`, `onSyncExit`, `onCall`, `onNewObject`, + `onCatch`, `onError`). For BTrace scripts the param declaration **drives + instrumentation** (tells the backend what to capture), so an unused-in-body param is + not actually useless. Commit `4340ffb4` fixed one (`onArraySet`'s `array`) by + referencing it in the `println`. The right fix for the rest is the same: reference + each captured value in the assertion `println`, which both satisfies the bot and + strengthens the smoke test (it verifies the captured value, not just that the probe + fired). + +--- + +## Task Breakdown + +### Task 1 — Ordinary method-argument loading for ENTRY (Gap A) — DONE + +**Approach:** +- In `emitProbeCall`, add an ENTRY ordinary-arg path. ENTRY args live in fixed locals: + slot `0` is `this` (unless static), then each arg in declaration order at slot + `1 + offset` where offset accumulates by `Type.getSize()`. The ClassFile API + `CodeModel` gives `thisSlot`/arg slots via `methodParamTypes` already available in the + surrounding `instrumentMethod` context — reuse the slot computation used for CALL + (`callContext.argumentSlots` is the model to mirror for method locals). +- Detect `AnyType[]` ordinary args (Gap C detection, shared helper) and emit the + `Object[]` packaging loop instead of a single load. +- Add the satisfiability condition to pre-validation so typed ENTRY args are no longer + rejected (mirror the CALL condition: ordinary index `< methodArgCount`). + +**Acceptance:** +- New unit tests: ENTRY with one typed arg; ENTRY with multiple typed args (category-1 + and category-2); ENTRY with `AnyType[]`; ENTRY `@Self` + typed args; static vs + instance method; mismatched descriptor rejected. +- Integration: a `ClassFileApiEntryArgsTest` btrace script capturing + `Math.max(int,int)` args and printing them. + +**Risk:** Medium. Slot math for category-2 args and static-vs-instance `this` offset. +Verify with `ClassFileApiBackendTest` bytecode assertions on local load opcodes. + +### Task 2 — Ordinary method-argument loading for RETURN (Gap B) — DONE + +**Approach:** Same as Task 1 but the RETURN emit path (line 895/907). Method args are +still in scope at every return point (locals are valid for the whole method body), so +the same slot computation applies. `@Return` and `@Duration` already have slots; ordinary +args reuse the method-local slots. + +**Acceptance:** +- Unit tests: RETURN with typed method args; RETURN with `AnyType[]`; RETURN with + `@Return` + typed args; multiple return instructions in one method; void return + + args rejected or skipped per ASM semantics. +- Integration: extend the entry-args smoke script or add a return-args one. + +**Risk:** Medium. Confirm ASM behavior for ordinary args on void-return methods +(ASM RETURN validates against `Type.getArgumentTypes(getDescriptor())`; args are +independent of return type, so they should be loadable even for void methods — verify). + +### Task 3 — `AnyType[]` aggregate packaging (Gap C, CALL + shared) — DONE + +**Approach:** +- Add a helper `isAnyTypeArrayParam(om, i)` that inspects the **original** + `om.getTargetDescriptor()` (pre-replace) for `[` + `ANYTYPE_DESC` at param `i`. Use + `Type.getArgumentTypes` on the original descriptor and `TypeUtils.isAnyTypeArray`. +- In `emitProbeCall`, for an ordinary param flagged `AnyType[]`: + - ENTRY/RETURN: package all method args from their locals into `Object[]` (box + primitives via the existing `boxPrimitiveReturn` helper or an equivalent + `box(TypeKind)`). + - CALL: package all call args from `callContext.argumentSlots` into `Object[]`. + - Mirror `AnyTypeArgProvider.doProvide`: `push(len); newArray(OBJECT); for each: + dup; push(j); loadLocal(type, slot); box; arrayStore`. +- Single-`AnyType` (non-array) ordinary arg: verify ASM behavior and either match it or + document the deviation. (ASM `loadArguments` only special-cases `isAnyTypeArray`; a + single `AnyType` ordinary arg is treated as a typed `Object` local load + box — + confirm before implementing.) + +**Acceptance:** +- Unit tests: CALL `AnyType[]` with 0/1/2/3 call args incl. long/double; CALL `AnyType[]` + + `@TargetInstance`; ENTRY/RETURN `AnyType[]` (shared with Tasks 1/2). +- Bytecode assertion: the packaging `newarray` + `arraystore` loop is present and the + invokedynamic descriptor ends with `Object[]`. + +**Risk:** Medium-high. Boxing for category-2 primitives and the slot-accumulation order +must match `AnyTypeArgProvider` exactly or the array will contain shifted values. + +### Task 4 — Phase 1 shared validation + location-family grouping (Gap D) + +**Approach:** +- Replace the `collectHandlers` if/else chain with a `Map>` + keyed by an enum grouping (ENTRY/RETURN/CALL family vs instruction-site families). +- Extract the duplicated special-param validation (`@Self`, `@ProbeClassName`, etc.) + used in `canEmitCallProbe`, `canEmitFieldProbe`, `canEmitSyncProbe`, etc. into a single + `validateSpecialParams(om, handlerArgTypes, loader)` returning a boolean. +- Do this **after** Tasks 1–3 so the new ordinary-arg logic lands in one place and is + not immediately re-duplicated. + +**Acceptance:** No behavior change; full `ClassFileApiBackendTest` green; the +instruction-site `canEmit*` methods shrink. + +**Risk:** Low-medium. Pure refactor; guard with the existing regression suite and a +before/after bytecode diff on a representative fixture. + +### Task 5 — Dead-code cleanup (Gap E) — NO CHANGE (re-evaluated) + +Re-checked against the ASM precedent: `Instrumentor`'s `switch (loc.getValue())` has +**no `default`** — an unknown `Kind` silently skips (returns the unmodified visitor). The +`else log.debug("Skipping unsupported probe kind ...")` arm in `ClassFileApiBackend.collectHandlers` +is the matching fallback for future `Kind` values; it is not dead in principle (only +unreachable for the current enum) and removing it would make a future `Kind` vanish with no +log, diverging from ASM. Keeping the arm is the correct, lower-risk choice. No change made. + +### Task 6 — Bot finding: unread `staticCall` local (Gap F.1) — DONE + +Removed the unused `boolean staticCall = ii.opcode() == Opcode.INVOKESTATIC;` local in +`canEmitCallProbe` (it was never read; `@TargetInstance` assignability uses `ownerType`). +`:btrace-agent:classFileApiBackendTest` (121 tests) green. + +### Task 7 — ~~Bot finding: `ClassCache` null checks (Gap F.2)~~ RESOLVED + +**Recheck correction:** The 66e2bfd3 `ClassCache.getInstance()` DCL null-check fix was +**dropped at merge** — the squash `9f39a250` did not touch `ClassCache.java`, so the two +bot findings (both on `ClassCache.java`) reference code that is not on `develop`. Gap +F.2 is moot. + +What landed from 66e2bfd3 is the `ClassFilter.isSubTypeOf` defensive guard. The residual +`if (cache == null)` guard there is JLS-dead (`getInstance()` uses the holder idiom) but +not bot-flagged. Resolved by folding it into the `ci == null` conservative no-match path +(Option 3 from the pros/cons discussion): + +```java +ClassCache cache = ClassCache.getInstance(); +ClassInfo ci = cache != null ? cache.get(loader, typeA) : null; +if (ci == null) return false; +``` + +**Done:** `btrace-agent/.../ClassFilter.java` edited; `:btrace-agent:spotlessApply` + +`:btrace-agent:compileJava` + `:btrace-agent:test` green. + +**Outstanding before commit:** run the JDK 8/11/17/21 integration suite to prove the +race fix (66e2bfd3) regression does not recur — unit tests do not exercise the +classloader-init race window. + +### Task 8 — Bot finding: smoke-test unused params (Gap F.3) — DONE + +Referenced each previously-unused captured param in its handler `println` via +`BTraceUtils.str(...)` for `Object` params (BTrace forbids `+ Object`; the existing code +only concats `String`/primitives) and `array.length` for the array param: +`onArrayGet.value`, `onNewArray.array`, `onSyncEntry.lock`, `onSyncExit.lock`, +`onCall.key`, `onNewObject.set`, `onCatch.exception`, `onError.exception`. The integration +driver (`ClassFileApiTests`) asserts via `stdout.contains(marker)` (substring), so the +appended values do not break the markers. `:integration-tests:spotlessApply` clean. + +**Outstanding:** the integration run on JDK 26 (deferred) must confirm the script still +compiles under the btrace-compiler and the smoke markers still fire — `str()` on the +`SynchronizedMap` mutex / `SynchronizedSet` calls `toString`, which iterates the backing +collection (low recursion risk — `*Printed` guards prevent probe re-entry, and the backing +collections are not instrumented). + +--- + +## Execution Order + +1. **Tasks 1–3** (ordinary args + `AnyType[]`) — DONE. Implemented in + `ClassFileApiBackend.emitProbeCall`: threaded `methodArgTypes` (computed from `methodDesc`) + through new ENTRY/RETURN convenience overloads; added typed ordinary method-arg loading + from fixed local slots (`methodArgSlot`), `AnyType[]` aggregate packaging + (`emitAnyTypeArray`: anewarray + per-element load/box/arraystore, mirroring ASM + `AnyTypeArgProvider`), and an `AnyType[]` short-circuit in `canEmitCallProbe` (without it + `sameStackType` rejected the aggregate param and the probe was skipped). Tests added: + typed ENTRY (instance+static), typed RETURN, ENTRY/RETURN/CALL `AnyType[]`, CALL + `AnyType[]` on a 0-arg call — each with a JVM-verifier load check + (`assertLoadsWithoutVerifyError`). `:btrace-agent:classFileApiBackendTest` (121 tests) and + `:btrace-agent:test` green; spotless clean. + - Known edge case (out of scope): sub-int (`byte`/`short`/`char`/`boolean`) `AnyType[]` + elements box via `boxPrimitiveReturn`, consistent with the existing `@Return` boxing and + the JVMS verifier's int-collapse; not exercised by tests (ASM `AnytypeArgs` also covers + only String/long/String[]/int[]). +2. **Task 4** (shared validation refactor) — after 1–3 so the new code lands once. +3. **Tasks 5, 6, 8** (dead code + low-risk bot nits) — Task 6 DONE (removed unread + `staticCall` local); Task 8 DONE (smoke-test params referenced via `str()`); Task 5 + NO CHANGE (the `else` arm is the correct fallback matching ASM's silent-skip). +4. **Task 7** — DONE (folded the residual `ClassFilter` `cache == null` guard into the + `ci == null` path); JDK 8/11/17/21 integration run still recommended pre-commit. + +## Verification Commands + +```sh +# Targeted ClassFile API backend tests (JDK 26) +JAVA_HOME=$HOME/.sdkman/candidates/java/26-tem GRADLE_USER_HOME=$(pwd)/.gradle-user \ + ./gradlew :btrace-agent:classFileApiBackendTest > /tmp/cfapi-gaps-test.log 2>&1 +rg -n "BUILD SUCCESSFUL|BUILD FAILED|FAILED|ERROR|ClassFileApiBackendTest" /tmp/cfapi-gaps-test.log + +# Formatting +JAVA_HOME=$HOME/.sdkman/candidates/java/26-tem GRADLE_USER_HOME=$(pwd)/.gradle-user \ + ./gradlew :btrace-agent:spotlessCheck > /tmp/cfapi-gaps-spotless.log 2>&1 +rg -n "BUILD SUCCESSFUL|BUILD FAILED|FAILED|ERROR|spotless" /tmp/cfapi-gaps-spotless.log + +# Full agent tests +JAVA_HOME=$HOME/.sdkman/candidates/java/26-tem GRADLE_USER_HOME=$(pwd)/.gradle-user \ + ./gradlew :btrace-agent:test > /tmp/cfapi-gaps-agent.log 2>&1 +rg -n "BUILD SUCCESSFUL|BUILD FAILED|FAILED|ERROR|tests" /tmp/cfapi-gaps-agent.log + +# Integration tests (build dist first; requires TEST_JAVA_HOME for older JDKs) +JAVA_HOME=$HOME/.sdkman/candidates/java/26-tem GRADLE_USER_HOME=$(pwd)/.gradle-user \ + ./gradlew -Pintegration test > /tmp/cfapi-gaps-integ.log 2>&1 +rg -n "BUILD SUCCESSFUL|BUILD FAILED|FAILED|ERROR|ClassFileApi" /tmp/cfapi-gaps-integ.log +``` + +## Out of Scope + +- Compiler/verifier changes outside `ClassFileApiBackend` unless a parity gap cannot be + closed locally (single-`AnyType` ordinary arg is the likely edge case to revisit). +- `IndyDispatcher` / runtime changes. +- Re-opening PR #843; follow-ups land as new PRs against `develop`. \ No newline at end of file diff --git a/integration-tests/src/test/btrace/ClassFileApiFeatureSmokeTest.java b/integration-tests/src/test/btrace/ClassFileApiFeatureSmokeTest.java index 2e7567885..5dc1a074a 100644 --- a/integration-tests/src/test/btrace/ClassFileApiFeatureSmokeTest.java +++ b/integration-tests/src/test/btrace/ClassFileApiFeatureSmokeTest.java @@ -17,6 +17,7 @@ package btrace; import static io.btrace.core.BTraceUtils.println; +import static io.btrace.core.BTraceUtils.str; import io.btrace.core.annotations.BTrace; import io.btrace.core.annotations.Duration; @@ -85,7 +86,7 @@ public static void onFieldSet(@TargetMethodOrField(fqn = true) String field, lon public static void onArrayGet(@Return Object value, int index) { if (!arrayGetPrinted) { arrayGetPrinted = true; - println("cfapi ARRAY_GET index=" + index); + println("cfapi ARRAY_GET index=" + index + ", value=" + str(value)); } } @@ -113,7 +114,13 @@ public static void onArraySet(@TargetInstance int[] array, int index, int value) public static void onNewArray(String type, int dimensions, @Return int[] array) { if (!newArrayPrinted) { newArrayPrinted = true; - println("cfapi NEWARRAY type=" + type + ", dimensions=" + dimensions); + println( + "cfapi NEWARRAY type=" + + type + + ", dimensions=" + + dimensions + + ", arrayLength=" + + array.length); } } @@ -124,7 +131,7 @@ public static void onNewArray(String type, int dimensions, @Return int[] array) public static void onSyncEntry(@TargetInstance Object lock) { if (!syncEntryPrinted) { syncEntryPrinted = true; - println("cfapi SYNC_ENTRY"); + println("cfapi SYNC_ENTRY lock=" + str(lock)); } } @@ -135,7 +142,7 @@ public static void onSyncEntry(@TargetInstance Object lock) { public static void onSyncExit(@TargetInstance Object lock) { if (!syncExitPrinted) { syncExitPrinted = true; - println("cfapi SYNC_EXIT"); + println("cfapi SYNC_EXIT lock=" + str(lock)); } } @@ -146,7 +153,7 @@ public static void onSyncExit(@TargetInstance Object lock) { public static void onCall(@TargetMethodOrField(fqn = true) String target, Object key) { if (!callPrinted) { callPrinted = true; - println("cfapi CALL " + target); + println("cfapi CALL " + target + ", key=" + str(key)); } } @@ -161,7 +168,7 @@ public static void onCall(@TargetMethodOrField(fqn = true) String target, Object public static void onNewObject(String type, @Return Object set) { if (!newObjectPrinted) { newObjectPrinted = true; - println("cfapi NEW " + type); + println("cfapi NEW " + type + ", set=" + str(set)); } } @@ -172,7 +179,7 @@ public static void onNewObject(String type, @Return Object set) { public static void onCatch(@TargetInstance IllegalArgumentException exception) { if (!catchPrinted) { catchPrinted = true; - println("cfapi CATCH"); + println("cfapi CATCH exception=" + str(exception)); } } @@ -183,7 +190,7 @@ public static void onCatch(@TargetInstance IllegalArgumentException exception) { public static void onError(@Duration long duration, @TargetInstance Throwable exception) { if (!errorPrinted) { errorPrinted = true; - println("cfapi ERROR duration=" + duration); + println("cfapi ERROR duration=" + duration + ", exception=" + str(exception)); } } }