Skip to content

Commit

Permalink
Fix range for debug symbols of method parameters (#1246)
Browse files Browse the repository at this point in the history 
By inserting the probe array access code at the beginning of the method
we accidently moved the range of the debug symbols of the method
parameters after the probe array access code. Actually the method
parameters are still valid from the very beginning of the method.

This offset in the method parameter's debug symbols may confuse tools
like Jandex.
  • Loading branch information
@marchof
marchof committed Nov 8, 2021
1 parent 472be4e commit 70d5b98
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 22 deletions.
48 changes: 27 additions & 21 deletions org.jacoco.core.test/src/org/jacoco/core/internal/instr/ProbeInserterTest.java
View file
Expand Up @@ -59,7 +59,7 @@ public void verify() {
}

@Test
public void testVariableStatic() {
public void probevar_should_be_at_position_0_for_static_method_without_parameters() {
ProbeInserter pi = new ProbeInserter(Opcodes.ACC_STATIC, "m", "()V",
actualVisitor, arrayStrategy);
pi.insertProbe(0);
Expand All @@ -71,7 +71,7 @@ public void testVariableStatic() {
}

@Test
public void testVariableNonStatic() {
public void probevar_should_be_at_position_1_for_instance_method_without_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);
pi.insertProbe(0);
Expand All @@ -83,7 +83,7 @@ public void testVariableNonStatic() {
}

@Test
public void testVariableNonStatic_IZObject() {
public void probevar_should_be_at_position_4_for_instance_method_with_3_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "(IZLjava/lang/Object;)V",
actualVisitor, arrayStrategy);
pi.insertProbe(0);
Expand All @@ -95,7 +95,7 @@ public void testVariableNonStatic_IZObject() {
}

@Test
public void testVariableNonStatic_JD() {
public void probevar_should_be_at_position_5_for_instance_method_with_2_wide_parameters() {
ProbeInserter pi = new ProbeInserter(0, "m", "(JD)V", actualVisitor,
arrayStrategy);
pi.insertProbe(0);
Expand All @@ -107,25 +107,27 @@ public void testVariableNonStatic_JD() {
}

@Test
public void testVisitCode() {
public void visitCode_should_call_IProbeArrayStrategy_for_any_methods() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);
pi.visitCode();

expectedVisitor.visitLabel(new Label());
expectedVisitor.visitLdcInsn("init");
}

@Test
public void testVisitClinit() {
public void visitCode_should_call_IProbeArrayStrategy_for_static_initializers() {
ProbeInserter pi = new ProbeInserter(0, "<clinit>", "()V",
actualVisitor, arrayStrategy);
pi.visitCode();

expectedVisitor.visitLabel(new Label());
expectedVisitor.visitLdcInsn("clinit");
}

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

Expand All @@ -146,7 +148,7 @@ public void testVisitVarIns() {
}

@Test
public void testVisitIincInsn() {
public void visitIincInsn_should_be_called_with_adjusted_variable_positions() {
ProbeInserter pi = new ProbeInserter(0, "m", "(II)V", actualVisitor,
arrayStrategy);
pi.visitIincInsn(0, 100);
Expand All @@ -166,7 +168,7 @@ public void testVisitIincInsn() {
}

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

Expand All @@ -176,10 +178,12 @@ public void testVisitLocalVariable() {
pi.visitLocalVariable(null, null, null, null, null, 3);
pi.visitLocalVariable(null, null, null, null, null, 4);

Label begin = new Label();

// Argument variables stay at the same position:
expectedVisitor.visitLocalVariable(null, null, null, null, null, 0);
expectedVisitor.visitLocalVariable(null, null, null, null, null, 1);
expectedVisitor.visitLocalVariable(null, null, null, null, null, 2);
expectedVisitor.visitLocalVariable(null, null, null, begin, null, 0);
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);
Expand Down Expand Up @@ -209,29 +213,31 @@ public void should_remap_LocalVariableAnnotation() {
}

@Test
public void testVisitMaxs1() {
public void new_stack_size_should_be_big_enought_to_store_probe_array() {
ProbeInserter pi = new ProbeInserter(0, "m", "(II)V", actualVisitor,
arrayStrategy);
pi.visitCode();
pi.visitMaxs(0, 8);

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

@Test
public void testVisitMaxs2() {
public void new_stack_size_should_be_increased_for_probes() {
ProbeInserter pi = new ProbeInserter(0, "m", "(II)V", actualVisitor,
arrayStrategy);
pi.visitCode();
pi.visitMaxs(10, 8);

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

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

Expand All @@ -245,7 +251,7 @@ public void testVisitFrame() {
}

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

Expand All @@ -256,7 +262,7 @@ public void testVisitFrameNoLocals() {
}

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

Expand All @@ -268,7 +274,7 @@ public void testVisitFrameProbeAt0() {
}

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

Expand All @@ -280,7 +286,7 @@ public void testFillOneWord() {
}

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

Expand All @@ -293,7 +299,7 @@ public void testFillTwoWord() {
}

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

Expand All @@ -309,7 +315,7 @@ public void testFillPartly() {
}

@Test(expected = IllegalArgumentException.class)
public void testVisitFrame_invalidType() {
public void visitFrame_must_only_support_resolved_frames() {
ProbeInserter pi = new ProbeInserter(0, "m", "()V", actualVisitor,
arrayStrategy);
pi.visitFrame(Opcodes.F_SAME, 0, null, 0, null);
Expand Down
14 changes: 13 additions & 1 deletion org.jacoco.core/src/org/jacoco/core/internal/instr/ProbeInserter.java
View file
Expand Up @@ -38,6 +38,9 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
/** Position of the inserted variable. */
private final int variable;

/** Label for the new beginning of the method */
private final Label beginLabel;

/** Maximum stack usage of the code to access the probe array. */
private int accessorStackSize;

Expand Down Expand Up @@ -66,6 +69,7 @@ class ProbeInserter extends MethodVisitor implements IProbeInserter {
pos += t.getSize();
}
variable = pos;
beginLabel = new Label();
}

public void insertProbe(final int id) {
Expand Down Expand Up @@ -93,6 +97,7 @@ public void insertProbe(final int id) {

@Override
public void visitCode() {
mv.visitLabel(beginLabel);
accessorStackSize = arrayStrategy.storeInstance(mv, clinit, variable);
mv.visitCode();
}
Expand All @@ -111,7 +116,14 @@ public final void visitIincInsn(final int var, final int increment) {
public final void visitLocalVariable(final String name, final String desc,
final String signature, final Label start, final Label end,
final int index) {
mv.visitLocalVariable(name, desc, signature, start, end, map(index));
if (index < variable) {
// Method parameters are still valid from the very beginning
mv.visitLocalVariable(name, desc, signature, beginLabel, end,
index);
} else {
mv.visitLocalVariable(name, desc, signature, start, end,
map(index));
}
}

@Override
Expand Down
2 changes: 2 additions & 0 deletions org.jacoco.doc/docroot/doc/changes.html
View file
Expand Up @@ -33,6 +33,8 @@ <h3>Fixed bugs</h3>
<ul>
<li>Fixed <code>NullPointerException</code> during filtering
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1189">#1189</a>).</li>
<li>Fix range for debug symbols of method parameters
(GitHub <a href="https://github.com/jacoco/jacoco/issues/1246">#1246</a>).</li>
</ul>

<h3>Non-functional Changes</h3>
Expand Down

0 comments on commit 70d5b98

Please sign in to comment.