diff options
4 files changed, 130 insertions, 36 deletions
diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/flow/MethodProbesAdapterTest.java b/org.jacoco.core.test/src/org/jacoco/core/internal/flow/MethodProbesAdapterTest.java index 083ec0a3..72e904ed 100644 --- a/org.jacoco.core.test/src/org/jacoco/core/internal/flow/MethodProbesAdapterTest.java +++ b/org.jacoco.core.test/src/org/jacoco/core/internal/flow/MethodProbesAdapterTest.java @@ -320,52 +320,132 @@ public class MethodProbesAdapterTest implements IProbeIdGenerator { adapter.visitTryCatchBlock(start, end, handler, "java/lang/Exception"); adapter.visitLabel(start); + adapter.visitInsn(Opcodes.NOP); + adapter.visitLabel(end); expectedVisitor.visitTryCatchBlock(start, end, handler, "java/lang/Exception"); expectedVisitor.visitLabel(start); + expectedVisitor.visitInsn(Opcodes.NOP); + expectedVisitor.visitLabel(end); } @Test - public void testVisitTryCatchBlockWithProbe() { - Label target = new Label(); - LabelInfo.setSuccessor(target); - LabelInfo.setTarget(target); - Label end = new Label(); - Label handler = new Label(); + public void testVisitTryCatchBlockWithProbeBeforeStart() { Label start = new Label(); + LabelInfo.setSuccessor(start); + LabelInfo.setTarget(start); + Label end = new Label(); + Label handler1 = new Label(); + Label handler2 = new Label(); - adapter.visitTryCatchBlock(target, end, handler, "java/lang/Exception"); - adapter.visitLabel(target); + adapter.visitTryCatchBlock(start, end, handler1, "java/lang/Exception"); + adapter.visitTryCatchBlock(start, end, handler2, "java/lang/Throwable"); + adapter.visitLabel(start); + adapter.visitInsn(Opcodes.NOP); + adapter.visitLabel(end); - expectedVisitor.visitTryCatchBlock(start, end, handler, + Label probe = new Label(); + expectedVisitor.visitTryCatchBlock(probe, end, handler1, "java/lang/Exception"); - expectedVisitor.visitLabel(start); + expectedVisitor.visitTryCatchBlock(probe, end, handler2, + "java/lang/Throwable"); + expectedVisitor.visitLabel(probe); expectedVisitor.visitProbe(1000); - expectedVisitor.visitLabel(target); + expectedVisitor.visitLabel(start); + expectedVisitor.visitInsn(Opcodes.NOP); + expectedVisitor.visitLabel(end); } @Test - public void testVisitMultipleTryCatchBlocksWithProbe() { - Label target = new Label(); - LabelInfo.setSuccessor(target); - LabelInfo.setTarget(target); + public void testVisitTryCatchBlockWithProbeBeforeEnd() { + Label start = new Label(); Label end = new Label(); + LabelInfo.setSuccessor(end); + LabelInfo.setTarget(end); Label handler1 = new Label(); Label handler2 = new Label(); - Label start = new Label(); - adapter.visitTryCatchBlock(target, end, handler1, "java/lang/Exception"); - adapter.visitTryCatchBlock(target, end, handler2, "java/io/IOException"); - adapter.visitLabel(target); + adapter.visitTryCatchBlock(start, end, handler1, "java/lang/Exception"); + adapter.visitTryCatchBlock(start, end, handler2, "java/lang/Throwable"); + adapter.visitLabel(start); + adapter.visitInsn(Opcodes.NOP); + adapter.visitLabel(end); - expectedVisitor.visitTryCatchBlock(start, end, handler1, + Label probe = new Label(); + expectedVisitor.visitTryCatchBlock(start, probe, handler1, "java/lang/Exception"); - expectedVisitor.visitTryCatchBlock(start, end, handler2, - "java/io/IOException"); + expectedVisitor.visitTryCatchBlock(start, probe, handler2, + "java/lang/Throwable"); expectedVisitor.visitLabel(start); + expectedVisitor.visitInsn(Opcodes.NOP); + expectedVisitor.visitLabel(probe); + expectedVisitor.visitProbe(1000); + expectedVisitor.visitLabel(end); + } + + /** + * https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-2.html#jvms-2.11.10 + */ + @Test + public void testStructuredLocking() { + Label start = new Label(); + LabelInfo.setSuccessor(start); + LabelInfo.setTarget(start); + Label end = new Label(); + LabelInfo.setSuccessor(end); + LabelInfo.setTarget(end); + Label handlerStart = new Label(); + Label handlerEnd = new Label(); + Label after = new Label(); + + adapter.visitTryCatchBlock(start, end, handlerStart, null); + adapter.visitTryCatchBlock(handlerStart, handlerEnd, handlerStart, + null); + adapter.visitVarInsn(Opcodes.ALOAD, 1); + adapter.visitInsn(Opcodes.MONITORENTER); + adapter.visitLabel(start); + adapter.visitInsn(Opcodes.NOP); + adapter.visitVarInsn(Opcodes.ALOAD, 1); + adapter.visitInsn(Opcodes.MONITOREXIT); + adapter.visitLabel(end); + adapter.visitJumpInsn(Opcodes.GOTO, after); + adapter.visitLabel(handlerStart); + adapter.visitVarInsn(Opcodes.ALOAD, 1); + adapter.visitInsn(Opcodes.MONITOREXIT); + adapter.visitLabel(handlerEnd); + adapter.visitInsn(Opcodes.ATHROW); + adapter.visitLabel(after); + + Label probe1 = new Label(); + Label probe2 = new Label(); + expectedVisitor.visitTryCatchBlock(probe1, probe2, handlerStart, null); + expectedVisitor.visitTryCatchBlock(handlerStart, handlerEnd, + handlerStart, null); + expectedVisitor.visitVarInsn(Opcodes.ALOAD, 1); + expectedVisitor.visitInsn(Opcodes.MONITORENTER); + // next probe must be INSIDE range of instructions covered by handler, + // otherwise monitorexit won't be executed + // in case if probe causes exception + expectedVisitor.visitLabel(probe1); expectedVisitor.visitProbe(1000); - expectedVisitor.visitLabel(target); + expectedVisitor.visitLabel(start); + expectedVisitor.visitInsn(Opcodes.NOP); + expectedVisitor.visitVarInsn(Opcodes.ALOAD, 1); + expectedVisitor.visitInsn(Opcodes.MONITOREXIT); + // next probe must be OUTSIDE range of instructions covered by handler, + // otherwise monitorexit will be executed second time + // in case if probe causes exception + expectedVisitor.visitLabel(probe2); + expectedVisitor.visitProbe(1001); + expectedVisitor.visitLabel(end); + expectedVisitor.visitJumpInsn(Opcodes.GOTO, after); + expectedVisitor.visitLabel(handlerStart); + expectedVisitor.visitVarInsn(Opcodes.ALOAD, 1); + expectedVisitor.visitInsn(Opcodes.MONITOREXIT); + expectedVisitor.visitLabel(handlerEnd); + expectedVisitor.visitInsnWithProbe(Opcodes.ATHROW, 1002); + expectedVisitor.visitLabel(after); } // === IProbeIdGenerator === diff --git a/org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java b/org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java index ef95e591..320c06b2 100644 --- a/org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java +++ b/org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java @@ -40,11 +40,16 @@ import org.objectweb.asm.tree.analysis.Frame; import org.objectweb.asm.tree.analysis.Interpreter; /** - * Tests that the invariants specified in chapter 2.11.10 of the JVM Spec do - * also hold for instrumented classes. Note that only some runtimes like Android - * ART do actually check these invariants. + * Tests that the invariants specified in <a href= + * "https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-2.html#jvms-2.11.10">chapter + * 2.11.10 of the JVM Spec</a> do also hold for instrumented classes. * - * https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10 + * This is important because JIT compiler in HotSpot JVM ignores methods with + * unstructured locking, so that they executed by interpreter. Android Runtime + * also doesn't optimize such methods. + * + * TODO verification implemented here is incomplete - in particular it is unable + * to catch problem described in https://github.com/jacoco/jacoco/issues/626 */ public class StructuredLockingTest { diff --git a/org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesAdapter.java b/org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesAdapter.java index cff89112..5b3cb02c 100644 --- a/org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesAdapter.java +++ b/org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesAdapter.java @@ -62,19 +62,24 @@ public final class MethodProbesAdapter extends MethodVisitor { } @Override - public void visitTryCatchBlock(Label start, final Label end, + public void visitTryCatchBlock(final Label start, final Label end, final Label handler, final String type) { - // If a probe will be inserted before the start label, we'll need to use - // a different label for the try-catch block. - if (tryCatchProbeLabels.containsKey(start)) { - start = tryCatchProbeLabels.get(start); - } else if (LabelInfo.needsProbe(start)) { + probesVisitor.visitTryCatchBlock(getTryCatchLabel(start), getTryCatchLabel(end), + handler, type); + } + + private Label getTryCatchLabel(Label label) { + if (tryCatchProbeLabels.containsKey(label)) { + label = tryCatchProbeLabels.get(label); + } else if (LabelInfo.needsProbe(label)) { + // If a probe will be inserted before the label, we'll need to use a + // different label to define the range of the try-catch block. final Label probeLabel = new Label(); LabelInfo.setSuccessor(probeLabel); - tryCatchProbeLabels.put(start, probeLabel); - start = probeLabel; + tryCatchProbeLabels.put(label, probeLabel); + label = probeLabel; } - probesVisitor.visitTryCatchBlock(start, end, handler, type); + return label; } @Override diff --git a/org.jacoco.doc/docroot/doc/changes.html b/org.jacoco.doc/docroot/doc/changes.html index ee78110f..2a498a43 100644 --- a/org.jacoco.doc/docroot/doc/changes.html +++ b/org.jacoco.doc/docroot/doc/changes.html @@ -61,6 +61,10 @@ <h3>Fixed Bugs</h3> <ul> + <li>Fixed bug in instrumentation of exception handlers, which was causing damage + of structured locking in certain situations and as consequence poor + performance of instrumented methods, analysis and fix contributed by Allen Hair + (GitHub <a href="https://github.com/jacoco/jacoco/issues/627">#627</a>).</li> <li><code>dump</code> commands now report error when server unexpectedly closes connection without sending response (GitHub <a href="https://github.com/jacoco/jacoco/issues/538">#538</a>).</li> |