Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add safety slot before variable for probe array #893

Merged
merged 10 commits into from
May 15, 2023
Merged

Add safety slot before variable for probe array #893

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
73206c8
(WIP) add test
Godin May 3, 2023
05c0064
(WIP) update existing unit tests
Godin May 3, 2023
9f5f574
(WIP) add new unit tests
Godin May 3, 2023
f9dfa07
(WIP) fix
Godin May 3, 2023
0b7cf75
Apply suggestions from code review
Godin May 10, 2023
6b19a20
Merge branch 'master' into issue-767
Godin May 11, 2023
b584d23
(WIP) remove unused import
Godin May 11, 2023
78a4869
(WIP) add comments
Godin May 11, 2023
7f8c17e
(WIP) update changelog
Godin May 11, 2023
06b0688
Merge branch 'master' into issue-767
marchof May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
138 changes: 95 additions & 43 deletions org.jacoco.core.test/src/org/jacoco/core/internal/instr/ProbeInserterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,48 +59,48 @@ public void verify() {
}

@Test
public void probevar_should_be_at_position_0_for_static_method_without_parameters() {
public void probevar_should_be_at_position_1_for_static_method_without_parameters() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "()V",
actualVisitor, arrayStrategy);
pi.insertProbe(0);

expectedVisitor.visitVarInsn(Opcodes.ALOAD, 0);
expectedVisitor.visitVarInsn(Opcodes.ALOAD, 1);
expectedVisitor.visitInsn(Opcodes.ICONST_0);
expectedVisitor.visitInsn(Opcodes.ICONST_1);
expectedVisitor.visitInsn(Opcodes.BASTORE);
}

@Test
public void probevar_should_be_at_position_1_for_instance_method_without_parameters() {
public void probevar_should_be_at_position_2_for_instance_method_without_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);
pi.insertProbe(0);

expectedVisitor.visitVarInsn(Opcodes.ALOAD, 1);
expectedVisitor.visitVarInsn(Opcodes.ALOAD, 2);
expectedVisitor.visitInsn(Opcodes.ICONST_0);
expectedVisitor.visitInsn(Opcodes.ICONST_1);
expectedVisitor.visitInsn(Opcodes.BASTORE);
}

@Test
public void probevar_should_be_at_position_4_for_instance_method_with_3_parameters() {
public void probevar_should_be_at_position_5_for_instance_method_with_3_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "(IZLjava/lang/Object;)V",
actualVisitor, arrayStrategy);
pi.insertProbe(0);

expectedVisitor.visitVarInsn(Opcodes.ALOAD, 4);
expectedVisitor.visitVarInsn(Opcodes.ALOAD, 5);
expectedVisitor.visitInsn(Opcodes.ICONST_0);
expectedVisitor.visitInsn(Opcodes.ICONST_1);
expectedVisitor.visitInsn(Opcodes.BASTORE);
}

@Test
public void probevar_should_be_at_position_5_for_instance_method_with_2_wide_parameters() {
public void probevar_should_be_at_position_6_for_instance_method_with_2_wide_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "(JD)V", actualVisitor,
arrayStrategy);
pi.insertProbe(0);

expectedVisitor.visitVarInsn(Opcodes.ALOAD, 5);
expectedVisitor.visitVarInsn(Opcodes.ALOAD, 6);
expectedVisitor.visitInsn(Opcodes.ICONST_0);
expectedVisitor.visitInsn(Opcodes.ICONST_1);
expectedVisitor.visitInsn(Opcodes.BASTORE);
Expand Down Expand Up @@ -142,9 +142,9 @@ public void visitVarInsn_should_be_called_with_adjusted_variable_positions() {
expectedVisitor.visitVarInsn(Opcodes.ILOAD, 1);
expectedVisitor.visitVarInsn(Opcodes.ILOAD, 2);

// Local variables are shifted by one:
expectedVisitor.visitVarInsn(Opcodes.ISTORE, 4);
expectedVisitor.visitVarInsn(Opcodes.FSTORE, 5);
// Local variables are shifted by two:
expectedVisitor.visitVarInsn(Opcodes.ISTORE, 5);
expectedVisitor.visitVarInsn(Opcodes.FSTORE, 6);
}

@Test
Expand All @@ -162,9 +162,9 @@ public void visitIincInsn_should_be_called_with_adjusted_variable_positions() {
expectedVisitor.visitIincInsn(1, 101);
expectedVisitor.visitIincInsn(2, 102);

// Local variables are shifted by one:
expectedVisitor.visitIincInsn(4, 103);
expectedVisitor.visitIincInsn(5, 104);
// Local variables are shifted by two:
expectedVisitor.visitIincInsn(5, 103);
expectedVisitor.visitIincInsn(6, 104);
}

@Test
Expand All @@ -185,9 +185,9 @@ public void visitLocalVariable_should_be_called_with_adjusted_variable_positions
expectedVisitor.visitLocalVariable(null, null, null, begin, null, 1);
expectedVisitor.visitLocalVariable(null, null, null, begin, null, 2);

// Local variables are shifted by one:
expectedVisitor.visitLocalVariable(null, null, null, null, null, 4);
// Local variables are shifted by two:
expectedVisitor.visitLocalVariable(null, null, null, null, null, 5);
expectedVisitor.visitLocalVariable(null, null, null, null, null, 6);
}

@Test
Expand All @@ -206,10 +206,10 @@ public void should_remap_LocalVariableAnnotation() {

expectedVisitor.visitLabel(start);
expectedVisitor.visitLabel(end);
// Local variables are shifted by one:
// Local variables are shifted by two:
expectedVisitor.visitLocalVariableAnnotation(
TypeReference.LOCAL_VARIABLE, null, new Label[] { start },
new Label[] { end }, new int[] { 3 }, "LNonNull;", false);
new Label[] { end }, new int[] { 4 }, "LNonNull;", false);
}

@Test
Expand All @@ -221,7 +221,7 @@ public void new_stack_size_should_be_big_enought_to_store_probe_array() {

expectedVisitor.visitLabel(new Label());
expectedVisitor.visitLdcInsn("init");
expectedVisitor.visitMaxs(5, 9);
expectedVisitor.visitMaxs(5, 10);
}

@Test
Expand All @@ -233,85 +233,137 @@ public void new_stack_size_should_be_increased_for_probes() {

expectedVisitor.visitLabel(new Label());
expectedVisitor.visitLdcInsn("init");
expectedVisitor.visitMaxs(13, 9);
expectedVisitor.visitMaxs(13, 10);
}

@Test
public void visitFrame_should_insert_probe_variable_between_arguments_and_local_variables() {
public void visitFrame_should_insert_safety_slot_and_probe_variable_between_arguments_and_local_variables() {
ProbeInserter pi = new ProbeInserter(0, "m", "(J)V", actualVisitor,
arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 3,
new Object[] { "Foo", Opcodes.LONG, "java/lang/String" }, 0,
new Object[0]);

expectedVisitor.visitFrame(Opcodes.F_NEW, 4,
new Object[] { "Foo", Opcodes.LONG, "[Z", "java/lang/String" },
0, new Object[0]);
expectedVisitor.visitFrame(Opcodes.F_NEW, 5, new Object[] { //
"Foo", //
Opcodes.LONG, //
Opcodes.TOP, // safety slot
"[Z", // probe array
"java/lang/String" //
}, 0, new Object[0]);
}

@Test
public void visitFrame_should_only_insert_probe_variable_when_no_other_local_variables_exist() {
public void visitFrame_should_only_insert_safety_slot_and_probe_variable_when_no_other_local_variables_exist() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "()V",
actualVisitor, arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 0, new Object[] {}, 0, new Object[0]);

expectedVisitor.visitFrame(Opcodes.F_NEW, 1, new Object[] { "[Z" }, 0,
new Object[0]);
expectedVisitor.visitFrame(Opcodes.F_NEW, 2, new Object[] { //
Opcodes.TOP, // safety slot
"[Z", // probe array
}, 0, new Object[0]);
}

@Test
public void visitFrame_should_insert_probe_variable_first_when_no_parameters_exist() {
public void visitFrame_should_insert_safety_slot_and_probe_variable_first_when_no_parameters_exist() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "()V",
actualVisitor, arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 2, new Object[] { Opcodes.DOUBLE, "Foo" },
0, new Object[0]);

expectedVisitor.visitFrame(Opcodes.F_NEW, 3,
new Object[] { "[Z", Opcodes.DOUBLE, "Foo" }, 0, new Object[0]);
expectedVisitor.visitFrame(Opcodes.F_NEW, 4, new Object[] { //
Opcodes.TOP, // safety slot
"[Z", // probe array
Opcodes.DOUBLE, //
"Foo" //
}, 0, new Object[0]);
}

@Test
public void visitFrame_should_fill_one_unused_slots_before_probe_variable_with_TOP() {
public void visitFrame_should_fill_2_unused_slots_before_probe_variable_with_TOP_TOP() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "(I)V",
actualVisitor, arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 0, new Object[] {}, 0, new Object[] {});

// The locals in this frame are filled with TOP up to the probe variable
expectedVisitor.visitFrame(Opcodes.F_NEW, 2,
new Object[] { Opcodes.TOP, "[Z", }, 0, new Object[] {});
expectedVisitor.visitFrame(Opcodes.F_NEW, 3, new Object[] { //
Opcodes.TOP, //
Opcodes.TOP, // safety slot
"[Z", // probe array
}, 0, new Object[] {});
}

@Test
public void visitFrame_should_fill_two_unused_slots_before_probe_variable_with_TOP_TOP() {
public void visitFrame_should_fill_3_unused_slots_before_probe_variable_with_TOP_TOP_TOP() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "(J)V",
actualVisitor, arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 0, new Object[] {}, 0, new Object[] {});

// The locals in this frame are filled with TOP up to the probe variable
expectedVisitor.visitFrame(Opcodes.F_NEW, 3,
new Object[] { Opcodes.TOP, Opcodes.TOP, "[Z", }, 0,
new Object[] {});
expectedVisitor.visitFrame(Opcodes.F_NEW, 4, new Object[] { //
Opcodes.TOP, //
Opcodes.TOP, //
Opcodes.TOP, // safety slot
"[Z", // probe array
}, 0, new Object[] {});
}

@Test
public void visitFrame_should_fill_three_unused_slots_before_probe_variable_with_TOP_TOP_TOP() {
public void visitFrame_should_fill_4_unused_slots_before_probe_variable_with_TOP_TOP_TOP_TOP() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "(DIJ)V",
actualVisitor, arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 1, new Object[] { Opcodes.DOUBLE }, 0,
new Object[] {});

// The locals in this frame are filled with TOP up to the probe variable
expectedVisitor
.visitFrame(
Opcodes.F_NEW, 5, new Object[] { Opcodes.DOUBLE,
Opcodes.TOP, Opcodes.TOP, Opcodes.TOP, "[Z", },
0, new Object[] {});
expectedVisitor.visitFrame(Opcodes.F_NEW, 6, new Object[] { //
Opcodes.DOUBLE, //
Opcodes.TOP, //
Opcodes.TOP, //
Opcodes.TOP, //
Opcodes.TOP, // safety slot
"[Z", // probe array
}, 0, new Object[] {});
}

@Test
public void visitFrame_should_not_insert_safety_slot_when_it_is_the_last_occupied_slot() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 1, new Object[] { //
Opcodes.DOUBLE //
}, 0, new Object[] {});

expectedVisitor.visitFrame(Opcodes.F_NEW, 2, new Object[] { //
Opcodes.DOUBLE, //
"[Z" // probe array
}, 0, new Object[] {});
}

@Test
public void visitFrame_should_insert_TOP_after_probe_variable_when_safety_slot_occupied_but_not_the_last() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);

pi.visitFrame(Opcodes.F_NEW, 2, new Object[] { //
Opcodes.DOUBLE, //
Opcodes.INTEGER //
}, 0, new Object[] {});

expectedVisitor.visitFrame(Opcodes.F_NEW, 4, new Object[] { //
Opcodes.DOUBLE, //
"[Z", // probe array
Opcodes.TOP, //
Opcodes.INTEGER, //
}, 0, new Object[] {});
}

@Test(expected = IllegalArgumentException.class)
Expand Down
118 changes: 118 additions & 0 deletions org.jacoco.core.test/src/org/jacoco/core/internal/instr/SafetySlotTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*******************************************************************************
* Copyright (c) 2009, 2023 Mountainminds GmbH & Co. KG and Contributors
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Evgeny Mandrikov - initial API and implementation
*
*******************************************************************************/
package org.jacoco.core.internal.instr;

import org.jacoco.core.instr.Instrumenter;
import org.jacoco.core.runtime.IRuntime;
import org.jacoco.core.runtime.RuntimeData;
import org.jacoco.core.runtime.SystemPropertiesRuntime;
import org.jacoco.core.test.TargetLoader;
import org.jacoco.core.test.validation.JavaVersion;
import org.junit.Test;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.ClassWriter;
import org.objectweb.asm.Label;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;

public class SafetySlotTest {

@Test
public void original() throws Exception {
Godin marked this conversation as resolved.
Show resolved Hide resolved
final byte[] original = createClass();

new TargetLoader().add("Sample", original).newInstance();
}

@Test
public void instrumented() throws Exception {
Godin marked this conversation as resolved.
Show resolved Hide resolved
final IRuntime runtime = new SystemPropertiesRuntime();
runtime.startup(new RuntimeData());

final byte[] original = createClass();
final byte[] instrumented = new Instrumenter(runtime)
.instrument(original, "Sample");

new TargetLoader().add("Sample", instrumented).newInstance();
}

private static byte[] createClass() throws Exception {
Godin marked this conversation as resolved.
Show resolved Hide resolved
final ClassWriter writer = new ClassWriter(0);
writer.visit(bytecodeVersion(), Opcodes.ACC_PUBLIC, "Sample", null,
"java/lang/Object", new String[0]);

MethodVisitor mv = writer.visitMethod(Opcodes.ACC_PUBLIC, "<init>",
"()V", null, new String[0]);
mv.visitCode();
mv.visitVarInsn(Opcodes.ALOAD, 0);
mv.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/Object", "<init>",
"()V", false);

// Put a long value (2 slots) on position 0, overwriting 'this'
mv.visitLdcInsn(Long.valueOf(42));
mv.visitVarInsn(Opcodes.LSTORE, 0);

mv.visitInsn(Opcodes.ICONST_0);
final Label label1 = new Label();
mv.visitJumpInsn(Opcodes.IFEQ, label1);
mv.visitJumpInsn(Opcodes.GOTO, label1);
mv.visitLabel(label1);
mv.visitFrame(Opcodes.F_NEW, 1, new Object[] { Opcodes.LONG }, 0,
new Object[] {});

mv.visitLdcInsn(Integer.valueOf(13));
mv.visitVarInsn(Opcodes.ISTORE, 2);
mv.visitInsn(Opcodes.ICONST_0);
final Label label2 = new Label();
mv.visitJumpInsn(Opcodes.IFEQ, label2);
mv.visitJumpInsn(Opcodes.GOTO, label2);
mv.visitLabel(label2);
mv.visitFrame(Opcodes.F_NEW, 2,
new Object[] { Opcodes.LONG, Opcodes.INTEGER }, 0,
new Object[] {});

mv.visitInsn(Opcodes.RETURN);
mv.visitMaxs(2, 3);
mv.visitEnd();

writer.visitEnd();

return writer.toByteArray();
}

/**
* According to Java Virtual Machine Specification <a href=
* "https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1">
* §4.10.1</a>:
*
* <blockquote>
* <p>
* A class file whose version number is 50.0 or above (§4.1) must be
* verified using the type checking rules given in this section.
* </p>
* <p>
* If, and only if, a class file's version number equals 50.0, then if the
* type checking fails, a Java Virtual Machine implementation may choose to
* attempt to perform verification by type inference (§4.10.2).
* </p>
* </blockquote>
*
* @return {@link Opcodes#V1_7} if supported by current JVM,
* {@link Opcodes#V1_5} otherwise
*/
private static int bytecodeVersion() {
return JavaVersion.current().isBefore("7") ? Opcodes.V1_5
: Opcodes.V1_7;
}

}