Skip to content

Commit

Permalink
Instrumentation should not damage structured locking (#627)
Browse files Browse the repository at this point in the history
  • Loading branch information
@allenhair @Godin
allenhair authored and Godin committed Dec 20, 2017
1 parent 4c0b93e commit e050f19
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 36 deletions.
126 changes: 103 additions & 23 deletions org.jacoco.core.test/src/org/jacoco/core/internal/flow/MethodProbesAdapterTest.java
View file
Expand Up @@ -320,52 +320,132 @@ public void testVisitTryCatchBlockNoProbe() {

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 ===
Expand Down
13 changes: 9 additions & 4 deletions org.jacoco.core.test/src/org/jacoco/core/test/validation/StructuredLockingTest.java
View file
Expand Up @@ -40,11 +40,16 @@
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 {

Expand Down
23 changes: 14 additions & 9 deletions org.jacoco.core/src/org/jacoco/core/internal/flow/MethodProbesAdapter.java
View file
Expand Up @@ -62,19 +62,24 @@ public void setAnalyzer(final AnalyzerAdapter analyzer) {
}

@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
Expand Down
4 changes: 4 additions & 0 deletions org.jacoco.doc/docroot/doc/changes.html
View file
Expand Up @@ -61,6 +61,10 @@ <h3>New Features</h3>

<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>
Expand Down

0 comments on commit e050f19

Please sign in to comment.