From 4741fb65cbebd799fece1c36ebc131cb0945a159 Mon Sep 17 00:00:00 2001 From: Evgeny Mandrikov Date: Fri, 17 Aug 2018 21:37:09 +0200 Subject: Add filter for bytecode that ECJ generates for String in switch (#735) --- .../test/validation/java7/StringSwitchTest.java | 28 ---- .../java7/targets/StringSwitchTarget.java | 6 +- .../core/internal/analysis/MethodAnalyzerTest.java | 40 +++++- .../internal/analysis/filter/FilterTestBase.java | 6 + .../analysis/filter/FinallyFilterTest.java | 5 + .../analysis/filter/StringSwitchEcjFilterTest.java | 141 +++++++++++++++++++++ .../core/internal/analysis/MethodAnalyzer.java | 28 +++- .../core/internal/analysis/filter/Filters.java | 7 +- .../internal/analysis/filter/IFilterOutput.java | 14 ++ .../analysis/filter/StringSwitchEcjFilter.java | 108 ++++++++++++++++ org.jacoco.doc/docroot/doc/changes.html | 4 + 11 files changed, 350 insertions(+), 37 deletions(-) create mode 100644 org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilterTest.java create mode 100644 org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilter.java diff --git a/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/StringSwitchTest.java b/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/StringSwitchTest.java index 242a55f5..0a40c010 100644 --- a/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/StringSwitchTest.java +++ b/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/StringSwitchTest.java @@ -11,7 +11,6 @@ *******************************************************************************/ package org.jacoco.core.test.validation.java7; -import org.jacoco.core.test.validation.Source.Line; import org.jacoco.core.test.validation.ValidationTestBase; import org.jacoco.core.test.validation.java7.targets.StringSwitchTarget; @@ -25,31 +24,4 @@ public class StringSwitchTest extends ValidationTestBase { super(StringSwitchTarget.class); } - public void assertSwitchCovered(final Line line) { - if (isJDKCompiler) { - assertFullyCovered(line, 0, 4); - } else { - // Filtering for ECJ not yet implemented: - assertPartlyCovered(line, 2, 7); - } - } - - public void assertSwitchNotCovered(final Line line) { - if (isJDKCompiler) { - assertNotCovered(line, 4, 0); - } else { - // Filtering for ECJ not yet implemented: - assertNotCovered(line, 9, 0); - } - } - - public void assertLookupswitch(final Line line) { - if (isJDKCompiler) { - assertNotCovered(line, 3, 0); - } else { - // Filtering for ECJ not yet implemented: - assertNotCovered(line, 7, 0); - } - } - } diff --git a/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/targets/StringSwitchTarget.java b/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/targets/StringSwitchTarget.java index 7b1bc291..db933f78 100644 --- a/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/targets/StringSwitchTarget.java +++ b/org.jacoco.core.test.validation.java7/src/org/jacoco/core/test/validation/java7/targets/StringSwitchTarget.java @@ -19,7 +19,7 @@ import static org.jacoco.core.test.validation.targets.Stubs.nop; public class StringSwitchTarget { private static void covered(Object s) { - switch (String.valueOf(s)) { // assertSwitchCovered() + switch (String.valueOf(s)) { // assertFullyCovered(0, 4) case "a": nop("case a"); // assertFullyCovered() break; @@ -36,7 +36,7 @@ public class StringSwitchTarget { } private static void notCovered(Object s) { - switch (String.valueOf(s)) { // assertSwitchNotCovered() + switch (String.valueOf(s)) { // assertNotCovered(4, 0) case "a": nop("case a"); break; @@ -88,7 +88,7 @@ public class StringSwitchTarget { * In this case javac generates LOOKUPSWITCH for second switch. */ private static void lookupswitch(Object s) { - switch (String.valueOf(s)) { // assertLookupswitch() + switch (String.valueOf(s)) { // assertNotCovered(3, 0) case "a": nop("case a"); break; diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/MethodAnalyzerTest.java b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/MethodAnalyzerTest.java index 114ebb59..de3ec4d9 100644 --- a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/MethodAnalyzerTest.java +++ b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/MethodAnalyzerTest.java @@ -14,6 +14,9 @@ package org.jacoco.core.internal.analysis; import static org.junit.Assert.assertEquals; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; import org.jacoco.core.analysis.ILine; import org.jacoco.core.analysis.IMethodCoverage; @@ -449,7 +452,7 @@ public class MethodAnalyzerTest implements IProbeIdGenerator { assertLine(1002, 0, 1, 0, 0); } - // === Scenario: table switch === + // === Scenario: table switch with and without replace filtering === private void createTableSwitch() { final Label l0 = new Label(); @@ -494,6 +497,41 @@ public class MethodAnalyzerTest implements IProbeIdGenerator { assertEquals(4, nextProbeId); } + private static final IFilter SWITCH_FILTER = new IFilter() { + public void filter(final MethodNode methodNode, + final IFilterContext context, final IFilterOutput output) { + final AbstractInsnNode i = methodNode.instructions.get(3); + assertEquals(Opcodes.TABLESWITCH, i.getOpcode()); + final AbstractInsnNode t1 = methodNode.instructions.get(6); + assertEquals(Opcodes.BIPUSH, t1.getOpcode()); + final AbstractInsnNode t2 = methodNode.instructions.get(13); + assertEquals(Opcodes.BIPUSH, t2.getOpcode()); + + final Set newTargets = new HashSet(); + newTargets.add(t1); + newTargets.add(t2); + output.replaceBranches(i, newTargets); + } + }; + + @Test + public void table_switch_with_filter_should_show_2_branches_when_original_replaced() { + createTableSwitch(); + runMethodAnalzer(SWITCH_FILTER); + + assertLine(1001, 2, 0, 2, 0); + } + + @Test + public void table_switch_with_filter_should_show_full_branch_coverage_when_new_targets_covered() { + createTableSwitch(); + probes[0] = true; + probes[1] = true; + runMethodAnalzer(SWITCH_FILTER); + + assertLine(1001, 0, 2, 0, 2); + } + @Test public void table_switch_should_show_missed_when_no_probes_are_executed() { createTableSwitch(); diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java index 50bb59f3..12064123 100644 --- a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java +++ b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FilterTestBase.java @@ -16,6 +16,7 @@ import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.List; +import java.util.Set; import org.objectweb.asm.tree.AbstractInsnNode; import org.objectweb.asm.tree.MethodNode; @@ -42,6 +43,11 @@ public abstract class FilterTestBase { final AbstractInsnNode i2) { fail(); } + + public void replaceBranches(final AbstractInsnNode source, + final Set newTargets) { + fail(); + } }; final void assertIgnored(Range... ranges) { diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FinallyFilterTest.java b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FinallyFilterTest.java index 987f3398..843790ec 100644 --- a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FinallyFilterTest.java +++ b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/FinallyFilterTest.java @@ -421,4 +421,9 @@ public class FinallyFilterTest implements IFilterOutput { } } + public void replaceBranches(final AbstractInsnNode source, + final Set newTargets) { + fail(); + } + } diff --git a/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilterTest.java b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilterTest.java new file mode 100644 index 00000000..e1bfcb82 --- /dev/null +++ b/org.jacoco.core.test/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilterTest.java @@ -0,0 +1,141 @@ +/******************************************************************************* + * Copyright (c) 2009, 2018 Mountainminds GmbH & Co. KG and Contributors + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Evgeny Mandrikov - initial API and implementation + * + *******************************************************************************/ +package org.jacoco.core.internal.analysis.filter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.fail; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import org.jacoco.core.internal.instr.InstrSupport; +import org.junit.Test; +import org.objectweb.asm.Label; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.MethodNode; + +/** + * Unit tests for {@link StringSwitchEcjFilter}. + */ +public class StringSwitchEcjFilterTest { + + private final IFilter filter = new StringSwitchEcjFilter(); + + private final FilterContextMock context = new FilterContextMock(); + + private AbstractInsnNode fromInclusive; + private AbstractInsnNode toInclusive; + + private AbstractInsnNode source; + private Set newTargets; + + private final IFilterOutput output = new IFilterOutput() { + public void ignore(final AbstractInsnNode fromInclusive, + final AbstractInsnNode toInclusive) { + assertNull(StringSwitchEcjFilterTest.this.fromInclusive); + StringSwitchEcjFilterTest.this.fromInclusive = fromInclusive; + StringSwitchEcjFilterTest.this.toInclusive = toInclusive; + } + + public void merge(final AbstractInsnNode i1, + final AbstractInsnNode i2) { + fail(); + } + + public void replaceBranches(final AbstractInsnNode source, + final Set newTargets) { + assertNull(StringSwitchEcjFilterTest.this.source); + StringSwitchEcjFilterTest.this.source = source; + StringSwitchEcjFilterTest.this.newTargets = newTargets; + } + }; + + @Test + public void should_filter() { + final Set expectedNewTargets = new HashSet(); + + final MethodNode m = new MethodNode(InstrSupport.ASM_API_VERSION, 0, + "name", "()V", null, null); + + final Label case1 = new Label(); + final Label case2 = new Label(); + final Label case3 = new Label(); + final Label caseDefault = new Label(); + final Label h1 = new Label(); + final Label h2 = new Label(); + + m.visitVarInsn(Opcodes.ALOAD, 1); + + m.visitVarInsn(Opcodes.ASTORE, 2); + m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "hashCode", + "()I", false); + m.visitTableSwitchInsn(97, 98, caseDefault, h1, h2); + + m.visitLabel(h1); + final AbstractInsnNode expectedFromInclusive = m.instructions.getLast(); + + m.visitVarInsn(Opcodes.ALOAD, 2); + m.visitLdcInsn("a"); + m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "equals", + "(Ljava/lang/Object;)Z", false); + // if equal "a", then goto its case + m.visitJumpInsn(Opcodes.IFNE, case1); + + m.visitVarInsn(Opcodes.ALOAD, 2); + m.visitLdcInsn("\0a"); + m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "equals", + "(Ljava/lang/Object;)Z", false); + // if equal "\0a", then goto its case + m.visitJumpInsn(Opcodes.IFNE, case2); + + // goto default case + m.visitJumpInsn(Opcodes.GOTO, caseDefault); + + m.visitLabel(h2); + + m.visitVarInsn(Opcodes.ALOAD, 2); + m.visitLdcInsn("b"); + m.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/String", "equals", + "(Ljava/lang/Object;)Z", false); + // if equal "b", then goto its case + m.visitJumpInsn(Opcodes.IFNE, case3); + + // goto default case + m.visitJumpInsn(Opcodes.GOTO, caseDefault); + final AbstractInsnNode expectedToInclusive = m.instructions.getLast(); + + m.visitLabel(caseDefault); + m.visitInsn(Opcodes.RETURN); + expectedNewTargets.add(m.instructions.getLast()); + m.visitLabel(case1); + m.visitInsn(Opcodes.RETURN); + expectedNewTargets.add(m.instructions.getLast()); + m.visitLabel(case2); + m.visitInsn(Opcodes.RETURN); + expectedNewTargets.add(m.instructions.getLast()); + m.visitLabel(case3); + m.visitInsn(Opcodes.RETURN); + expectedNewTargets.add(m.instructions.getLast()); + + filter.filter(m, context, output); + + assertEquals(expectedFromInclusive.getPrevious(), source); + assertEquals(expectedNewTargets, newTargets); + assertEquals(expectedFromInclusive, fromInclusive); + assertEquals(expectedToInclusive, toInclusive); + } + +} diff --git a/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java b/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java index ba862adc..82b97466 100644 --- a/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java +++ b/org.jacoco.core/src/org/jacoco/core/internal/analysis/MethodAnalyzer.java @@ -173,6 +173,13 @@ public class MethodAnalyzer extends MethodProbesVisitor } } + private final Map> replacements = new HashMap>(); + + public void replaceBranches(final AbstractInsnNode source, + final Set newTargets) { + replacements.put(source, newTargets); + } + @Override public void visitLabel(final Label label) { currentLabel.add(label); @@ -362,6 +369,7 @@ public class MethodAnalyzer extends MethodProbesVisitor for (final CoveredProbe p : coveredProbes) { p.instruction.setCovered(p.branch); } + // Merge: for (final Instruction i : instructions) { final AbstractInsnNode m = i.getNode(); @@ -371,6 +379,7 @@ public class MethodAnalyzer extends MethodProbesVisitor nodeToInstruction.get(r).merge(i); } } + // Report result: coverage.ensureCapacity(firstLine, lastLine); for (final Instruction i : instructions) { @@ -378,8 +387,23 @@ public class MethodAnalyzer extends MethodProbesVisitor continue; } - final int total = i.getBranches(); - final int covered = i.getCoveredBranches(); + final int total; + final int covered; + final Set r = replacements.get(i.getNode()); + if (r != null) { + int cb = 0; + for (AbstractInsnNode b : r) { + if (nodeToInstruction.get(b).getCoveredBranches() > 0) { + cb++; + } + } + total = r.size(); + covered = cb; + } else { + total = i.getBranches(); + covered = i.getCoveredBranches(); + } + final ICounter instrCounter = covered == 0 ? CounterImpl.COUNTER_1_0 : CounterImpl.COUNTER_0_1; final ICounter branchCounter = total > 1 diff --git a/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Filters.java b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Filters.java index b235cfb8..bdb0854b 100644 --- a/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Filters.java +++ b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/Filters.java @@ -31,9 +31,10 @@ public final class Filters implements IFilter { new TryWithResourcesJavac11Filter(), new TryWithResourcesJavacFilter(), new TryWithResourcesEcjFilter(), new FinallyFilter(), new PrivateEmptyNoArgConstructorFilter(), - new StringSwitchJavacFilter(), new EnumEmptyConstructorFilter(), - new AnnotationGeneratedFilter(), new KotlinGeneratedFilter(), - new KotlinLateinitFilter(), new KotlinWhenSealedFilter()); + new StringSwitchJavacFilter(), new StringSwitchEcjFilter(), + new EnumEmptyConstructorFilter(), new AnnotationGeneratedFilter(), + new KotlinGeneratedFilter(), new KotlinLateinitFilter(), + new KotlinWhenSealedFilter()); private final IFilter[] filters; diff --git a/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java index 4ca9b814..bbcbf00e 100644 --- a/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java +++ b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/IFilterOutput.java @@ -11,6 +11,8 @@ *******************************************************************************/ package org.jacoco.core.internal.analysis.filter; +import java.util.Set; + import org.objectweb.asm.tree.AbstractInsnNode; /** @@ -41,4 +43,16 @@ public interface IFilterOutput { */ void merge(AbstractInsnNode i1, AbstractInsnNode i2); + /** + * Marks instruction whose outgoing branches should be replaced during + * computation of coverage. + * + * @param source + * instruction which branches should be replaced + * @param newTargets + * new targets of branches + */ + void replaceBranches(AbstractInsnNode source, + Set newTargets); + } diff --git a/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilter.java b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilter.java new file mode 100644 index 00000000..cbd2a216 --- /dev/null +++ b/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/StringSwitchEcjFilter.java @@ -0,0 +1,108 @@ +/******************************************************************************* + * Copyright (c) 2009, 2018 Mountainminds GmbH & Co. KG and Contributors + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Evgeny Mandrikov - initial API and implementation + * + *******************************************************************************/ +package org.jacoco.core.internal.analysis.filter; + +import java.util.HashSet; +import java.util.Set; + +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.JumpInsnNode; +import org.objectweb.asm.tree.LabelNode; +import org.objectweb.asm.tree.LookupSwitchInsnNode; +import org.objectweb.asm.tree.MethodNode; +import org.objectweb.asm.tree.TableSwitchInsnNode; + +/** + * Filters code that is generated by ECJ for a switch statement + * with a String. + */ +public final class StringSwitchEcjFilter implements IFilter { + + public void filter(final MethodNode methodNode, + final IFilterContext context, final IFilterOutput output) { + final Matcher matcher = new Matcher(); + for (AbstractInsnNode i = methodNode.instructions + .getFirst(); i != null; i = i.getNext()) { + matcher.match(i, output); + } + } + + private static class Matcher extends AbstractMatcher { + public void match(final AbstractInsnNode start, + final IFilterOutput output) { + + cursor = start; + + nextIsVar(Opcodes.ASTORE, "s"); + nextIsInvokeVirtual("java/lang/String", "hashCode"); + nextIsSwitch(); + if (cursor == null) { + return; + } + + final AbstractInsnNode s = cursor; + final int hashCodes; + final LabelNode defaultLabel; + if (s.getOpcode() == Opcodes.LOOKUPSWITCH) { + final LookupSwitchInsnNode lookupSwitch = (LookupSwitchInsnNode) cursor; + defaultLabel = lookupSwitch.dflt; + hashCodes = lookupSwitch.labels.size(); + } else { + final TableSwitchInsnNode tableSwitch = (TableSwitchInsnNode) cursor; + defaultLabel = tableSwitch.dflt; + hashCodes = tableSwitch.labels.size(); + } + + final Set replacements = new HashSet(); + replacements.add(instructionAfterLabel(defaultLabel)); + + for (int i = 0; i < hashCodes; i++) { + while (true) { + nextIsVar(Opcodes.ALOAD, "s"); + nextIs(Opcodes.LDC); + nextIsInvokeVirtual("java/lang/String", "equals"); + // jump to case + nextIs(Opcodes.IFNE); + if (cursor == null) { + return; + } + + replacements.add(instructionAfterLabel( + ((JumpInsnNode) cursor).label)); + + if (cursor.getNext().getOpcode() == Opcodes.GOTO) { + // end of comparisons for same hashCode + // jump to default + nextIs(Opcodes.GOTO); + break; + } + } + } + + output.ignore(s.getNext(), cursor); + output.replaceBranches(s, replacements); + } + } + + private static AbstractInsnNode instructionAfterLabel( + final LabelNode label) { + AbstractInsnNode i = label.getNext(); + while (i.getType() == AbstractInsnNode.FRAME + || i.getType() == AbstractInsnNode.LABEL + || i.getType() == AbstractInsnNode.LINE) { + i = i.getNext(); + } + return i; + } + +} diff --git a/org.jacoco.doc/docroot/doc/changes.html b/org.jacoco.doc/docroot/doc/changes.html index 35c61284..46208992 100644 --- a/org.jacoco.doc/docroot/doc/changes.html +++ b/org.jacoco.doc/docroot/doc/changes.html @@ -31,6 +31,10 @@ (GitHub #669).
  • Synthetic classes are filtered out during generation of report (GitHub #668).
    • +
  • Part of bytecode generated by ECJ for switch statements on + java.lang.String values is filtered out during generation of + report + (GitHub #735).
  • Methods added by the Kotlin compiler are filtered out during generation of report. Idea and implementation by Nikolay Krasko (GitHub #689).
    • -- cgit v1.2.3