infernap12/jdk
1
0
Fork 0
mirror of https://github.com/openjdk/jdk.git synced 2026-01-28 12:09:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

8367949: JFR: MethodTrace double-counts methods that catch their own exceptions

Browse Source 
Reviewed-by: mgronlun
This commit is contained in:
Erik Gahlin 2026-01-08 16:34:39 +00:00
parent 677572b42d
commit fa2eb62647
5 changed files with 363 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Instrumentation.java
View File

@ -58,7 +58,13 @@ final class Instrumentation {
}
public void addMethod(long methodId, String name, String signature, int modification) {
modificationMap.put(name + signature, new Method(methodId, Modification.valueOf(modification), className + "::" + name));
Method method = new Method(
methodId,
Modification.valueOf(modification),
name.equals("<init>"),
className + "::" + name
);
modificationMap.put(name + signature, method);
}
public List<Method> getMethods() {
@ -71,7 +77,7 @@ final class Instrumentation {
ClassModel classModel = classFile.parse(bytecode);
byte[] generated = classFile.build(classModel.thisClass().asSymbol(), classBuilder -> {
for (var ce : classModel) {
if (modifyClassElement(classBuilder, ce)) {
if (modifyClassElement(classModel, classBuilder, ce)) {
modified[0] = true;
} else {
classBuilder.with(ce);
@ -93,7 +99,7 @@ final class Instrumentation {
}
}
private boolean modifyClassElement(ClassBuilder classBuilder, ClassElement ce) {
private boolean modifyClassElement(ClassModel classModel, ClassBuilder classBuilder, ClassElement ce) {
if (ce instanceof MethodModel mm) {
String method = mm.methodName().stringValue();
String signature = mm.methodType().stringValue();
@ -102,15 +108,15 @@ final class Instrumentation {
if (tm != null) {
Modification m = tm.modification();
if (m.tracing() || m.timing()) {
return modifyMethod(classBuilder, mm, tm);
return modifyMethod(classModel, classBuilder, mm, tm);
}
}
}
return false;
}
private boolean modifyMethod(ClassBuilder classBuilder, MethodModel m, Method method) {
var code = m.code();
private boolean modifyMethod(ClassModel classModel, ClassBuilder classBuilder, MethodModel methodModel, Method method) {
var code = methodModel.code();
if (code.isPresent()) {
if (classLoader == null && ExcludeList.containsMethod(method.name())) {
String msg = "Risk of recursion, skipping bytecode generation of " + method.name();
@ -118,9 +124,9 @@ final class Instrumentation {
return false;
}
MethodTransform s = MethodTransform.ofStateful(
() -> MethodTransform.transformingCode(new Transform(method))
() -> MethodTransform.transformingCode(new Transform(classModel, code.get(), method))
);
classBuilder.transformMethod(m, s);
classBuilder.transformMethod(methodModel, s);
return true;
}
return false;

2
src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Method.java
View File

@ -31,7 +31,7 @@ import jdk.jfr.internal.Logger;
/**
* Class that holds information about an instrumented method.
*/
record Method(long methodId, Modification modification, String name) {
record Method(long methodId, Modification modification, boolean constructor, String name) {
@Override
public String toString() {
return name + (modification.timing() ? " +timing" : " -timing") + (modification.tracing() ? " +tracing" : " -tracing") + " (Method ID: " + String.format("0x%08X)", methodId);

199
src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Transform.java
View File

@ -24,13 +24,19 @@
*/
package jdk.jfr.internal.tracing;
import java.lang.classfile.ClassModel;
import java.lang.classfile.CodeBuilder;
import java.lang.classfile.CodeElement;
import java.lang.classfile.CodeModel;
import java.lang.classfile.CodeTransform;
import java.lang.classfile.Label;
import java.lang.classfile.TypeKind;
import java.lang.classfile.instruction.InvokeInstruction;
import java.lang.classfile.instruction.ReturnInstruction;
import java.lang.classfile.instruction.ThrowInstruction;
import java.lang.constant.ClassDesc;
import java.util.ArrayList;
import java.util.List;
import jdk.jfr.internal.util.Bytecode;
import jdk.jfr.internal.util.Bytecode.MethodDesc;
@ -43,59 +49,208 @@ import jdk.jfr.tracing.MethodTracer;
* The method ID is determined by native code.
*/
final class Transform implements CodeTransform {
private static class TryBlock {
Label start;
Label end;
}
private static final ClassDesc METHOD_TRACER_CLASS = ClassDesc.of(MethodTracer.class.getName());
private static final MethodDesc TRACE_METHOD = MethodDesc.of("trace", "(JJ)V");
private static final MethodDesc TIMING_METHOD = MethodDesc.of("timing", "(JJ)V");
private static final MethodDesc TRACE_TIMING_METHOD = MethodDesc.of("traceTiming", "(JJ)V");
private static final MethodDesc TIMESTAMP_METHOD = MethodDesc.of("timestamp", "()J");
private final List<TryBlock> tryBlocks = new ArrayList<>();
private final boolean simplifiedInstrumentation;
private final ClassModel classModel;
private final Method method;
private int timestampSlot = -1;
Transform(Method method) {
Transform(ClassModel classModel, CodeModel model, Method method) {
this.method = method;
this.classModel = classModel;
// The JVMS (not the JLS) allows multiple mutually exclusive super/this.<init>
// invocations in a constructor body as long as only one lies on any given
// execution path. For example, this is valid bytecode:
//
// Foo(boolean value) {
// if (value) {
// staticMethodThatMayThrow();
// super();
// } else {
// try {
// if (value == 0) {
// throw new Exception("");
// }
// } catch (Throwable t) {
// throw t;
// }
// super();
// }
// }
//
// If such a method is found, instrumentation falls back to instrumenting only
// RET and ATHROW. This can cause exceptions to be missed or counted twice.
//
// An effect of this heuristic is that constructors like the one below
// will also trigger simplified instrumentation.
//
// class Bar {
// }
//
// class Foo extends Bar {
// Foo() {
// new Bar();
// }
// }
//
// java.lang.Object::<init> with zero constructor invocations should use simplified instrumentation
this.simplifiedInstrumentation = method.constructor() && constructorInvocations(model.elementList()) != 1;
}
private int constructorInvocations(List<CodeElement> elementList) {
int count = 0;
for (CodeElement e : elementList) {
if (isConstructorInvocation(e)) {
count++;
}
}
return count;
}
private boolean isConstructorInvocation(CodeElement element) {
if (element instanceof InvokeInstruction inv && inv.name().equalsString("<init>")) {
if (classModel.thisClass().equals(inv.owner())) {
return true;
}
if (classModel.superclass().isPresent()) {
return classModel.superclass().get().equals(inv.owner());
}
}
return false;
}
@Override
public final void accept(CodeBuilder builder, CodeElement element) {
public void accept(CodeBuilder builder, CodeElement element) {
if (simplifiedInstrumentation) {
acceptSimplifiedInstrumentation(builder, element);
return;
}
if (method.constructor()) {
acceptConstructor(builder, element, isConstructorInvocation(element));
} else {
acceptMethod(builder, element);
}
}
@Override
public void atEnd(CodeBuilder builder) {
endTryBlock(builder);
for (TryBlock block : tryBlocks) {
addCatchHandler(block, builder);
}
}
private void acceptConstructor(CodeBuilder builder, CodeElement element, boolean isConstructorInvocation) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
if (!isConstructorInvocation) {
beginTryBlock(builder);
}
}
if (isConstructorInvocation) {
endTryBlock(builder);
builder.with(element);
beginTryBlock(builder);
return;
}
if (element instanceof ReturnInstruction) {
addTracing(builder);
}
builder.with(element);
}
private void endTryBlock(CodeBuilder builder) {
if (tryBlocks.isEmpty()) {
return;
}
TryBlock last = tryBlocks.getLast();
if (last.end == null) {
last.end = builder.newBoundLabel();
}
}
private void beginTryBlock(CodeBuilder builder) {
TryBlock block = new TryBlock();
block.start = builder.newBoundLabel();
tryBlocks.add(block);
}
private void acceptSimplifiedInstrumentation(CodeBuilder builder, CodeElement element) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
}
if (element instanceof ReturnInstruction || element instanceof ThrowInstruction) {
builder.lload(timestampSlot);
builder.ldc(method.methodId());
Modification modification = method.modification();
boolean objectInit = method.name().equals("java.lang.Object::<init>");
String suffix = objectInit ? "ObjectInit" : "";
if (modification.timing()) {
if (modification.tracing()) {
invokeTraceTiming(builder, suffix);
} else {
invokeTiming(builder, suffix);
}
} else {
if (modification.tracing()) {
invokeTrace(builder, suffix);
}
}
addTracing(builder);
}
builder.with(element);
}
public static void invokeTiming(CodeBuilder builder, String suffix) {
private void acceptMethod(CodeBuilder builder, CodeElement element) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
beginTryBlock(builder);
}
if (element instanceof ReturnInstruction) {
addTracing(builder);
}
builder.with(element);
}
private void addCatchHandler(TryBlock block, CodeBuilder builder) {
Label catchHandler = builder.newBoundLabel();
int exceptionSlot = builder.allocateLocal(TypeKind.REFERENCE);
builder.astore(exceptionSlot);
addTracing(builder);
builder.aload(exceptionSlot);
builder.athrow();
builder.exceptionCatchAll(block.start, block.end, catchHandler);
}
private void addTracing(CodeBuilder builder) {
builder.lload(timestampSlot);
builder.ldc(method.methodId());
Modification modification = method.modification();
boolean objectInit = method.name().equals("java.lang.Object::<init>");
String suffix = objectInit ? "ObjectInit" : "";
if (modification.timing()) {
if (modification.tracing()) {
invokeTraceTiming(builder, suffix);
} else {
invokeTiming(builder, suffix);
}
} else {
if (modification.tracing()) {
invokeTrace(builder, suffix);
}
}
}
private static void invokeTiming(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TIMING_METHOD.name() + suffix, TIMING_METHOD.descriptor());
}
public static void invokeTrace(CodeBuilder builder, String suffix) {
private static void invokeTrace(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TRACE_METHOD.name() + suffix, TRACE_METHOD.descriptor());
}
public static void invokeTraceTiming(CodeBuilder builder, String suffix) {
private static void invokeTraceTiming(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TRACE_TIMING_METHOD.name() + suffix, TRACE_TIMING_METHOD.descriptor());
}
public static int invokeTimestamp(CodeBuilder builder) {
private static int invokeTimestamp(CodeBuilder builder) {
Bytecode.invokestatic(builder, METHOD_TRACER_CLASS, TIMESTAMP_METHOD);
return builder.allocateLocal(TypeKind.LONG);
}

167
test/jdk/jdk/jfr/event/tracing/TestConstructors.java Normal file
View File

@ -0,0 +1,167 @@
/*
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package jdk.jfr.event.tracing;
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
import jdk.jfr.Recording;
import jdk.jfr.consumer.RecordedEvent;
import jdk.jfr.consumer.RecordedMethod;
import jdk.jfr.consumer.RecordingFile;
import jdk.test.lib.jfr.Events;
/**
* @test
* @summary Tests that constructors are instrumented correctly.
* @requires vm.flagless
* @requires vm.hasJFR
* @library /test/lib
* @run main/othervm -Xlog:jfr+methodtrace=debug
* jdk.jfr.event.tracing.TestConstructors
**/
public class TestConstructors {
static private void methodThatThrows() {
throw new RuntimeException();
}
public static class Cat {
Cat() {
new String();
methodThatThrows();
super();
methodThatThrows();
}
}
public static class Dog {
Dog() {
super();
methodThatThrows();
}
}
public static class Tiger {
Tiger() {
methodThatThrows();
super();
}
}
public static class Zebra {
Zebra(boolean shouldThrow) {
this(shouldThrow ? 1 : 0);
}
Zebra(int shouldThrow) {
if (shouldThrow == 1) {
throw new RuntimeException();
}
}
}
public static class Snake {
Snake() {
try {
throw new RuntimeException();
} catch (Exception e) {
// Ignore
}
super();
}
}
public static void main(String... args) throws Exception {
try (Recording r = new Recording()) {
r.enable("jdk.MethodTrace").with("filter", Dog.class.getName() + ";" + Cat.class.getName() + ";" + Tiger.class.getName() + ";" + Zebra.class.getName() + ";" + Snake.class.getName());
r.start();
try {
new Cat();
} catch (Exception e) {
// ignore
}
try {
new Dog();
} catch (Exception e) {
// ignore
}
try {
new Tiger();
} catch (Exception e) {
// ignore
}
try {
new Zebra(true);
} catch (Exception e) {
// ignore
}
try {
new Zebra(false);
} catch (Exception e) {
// ignore
}
try {
new Snake();
} catch (Exception e) {
// ignore
}
r.stop();
List<RecordedEvent> events = Events.fromRecording(r);
var methods = buildMethodMap(events);
if (methods.size() != 5) {
throw new Exception("Expected 5 different methods");
}
assertMethodCount(methods, "Cat", 1);
assertMethodCount(methods, "Dog", 1);
assertMethodCount(methods, "Snake", 1);
assertMethodCount(methods, "Tiger", 1);
assertMethodCount(methods, "Zebra", 3);
}
}
private static void assertMethodCount(Map<String, Long> methods, String className, int expectedCount) throws Exception {
String name = TestConstructors.class.getName() + "$" + className + "::<init>";
Long count = methods.get(name);
if (count == null) {
throw new Exception("Could not find traced method " + name);
}
if (count != expectedCount) {
throw new Exception("Expected " + expectedCount + " trace event for " + name);
}
}
private static Map<String, Long> buildMethodMap(List<RecordedEvent> events) {
Map<String, Long> map = new TreeMap<>();
for (RecordedEvent e : events) {
RecordedMethod m = e.getValue("method");
String name = m.getType().getName() + "::" + m.getName();
map.compute(name, (_, value) -> (value == null) ? 1 : value + 1);
}
for (var e : map.entrySet()) {
System.out.println(e.getKey() + " " + e.getValue());
}
return map;
}
}

4
test/jdk/jdk/jfr/event/tracing/TestInstrumentation.java
View File

@ -93,6 +93,8 @@ public class TestInstrumentation {
assertMethod(map, "exception", 2);
assertMethod(map, "switchExpression", 3);
assertMethod(map, "recursive", 4);
assertMethod(map, "deepException", 1);
assertMethod(map, "whileTrue", 1);
assertMethod(map, "multipleReturns", 5);
if (!map.isEmpty()) {
throw new Exception("Found unexpected methods " + map.keySet());
@ -105,6 +107,8 @@ public class TestInstrumentation {
assertMethod(map, "exception", 2);
assertMethod(map, "switchExpression", 3);
assertMethod(map, "recursive", 4);
assertMethod(map, "deepException", 1);
assertMethod(map, "whileTrue", 1);
assertMethod(map, "multipleReturns", 5);
for (var entry : map.entrySet()) {
long invocations = entry.getValue();