During creation of report merge duplicate finally blocks that compiler generates #521

Godin · 2017-04-22T21:41:11Z

https://github.com/jacoco/jacoco/wiki/filtering-JAVAC.FINALLY

Godin · 2017-04-23T19:27:54Z

@marchof To merge conditional jumps where different branches are covered in different instructions we need to determine which branch covered - this is what first change does. Second introduces actual merging. While there is some room for improvements, changes are pretty straightforward, and so main care should be paid for third change that introduces filter for finally blocks. Thus wondering what you think about it?

marchof · 2017-04-24T07:32:13Z

org.jacoco.core/src/org/jacoco/core/internal/flow/Instruction.java

 	 */
-	public int getCoveredBranches() {
+	public BitSet getCoveredBranches() {


@Godin I would prefer to keep the BitSet internally. So

Keep int getCoveredBranches()

Add void merge(Instruction)

This encapsulation looks cleaner to me allows internal optimizations later.

allows internal optimizations later

I suppose internal in respect to a class and not project since class is already in internal package 😉

Jokes aside - while indeed such encapsulation looks cleaner, this will complicate a test which currently able to directly assert on a content of this BitSet, but ok can do this.

Of course internal to the internal class 😉 . For whitebox testing of Instruction we can still expose the BitSet but for interaction with other classes I would prefer a more semantic interface (like merge()).

For whitebox testing of Instruction we can still expose the BitSet

I think that so far we don't have fields/methods that are "only for testing".

I think the only ones are in LabelFlowAnalyzer.

@marchof encapsulated, added toString instead of package-local field to avoid its accidental usage in other classes
✅

@Godin Thanks, LGTM now! 👍

marchof · 2017-04-24T07:34:22Z

@Godin I definitely like the straightforward approach! I will go though the implementation and add comments if I have specific questions or concerns.

marchof · 2017-04-24T07:47:52Z

org.jacoco.core/src/org/jacoco/core/internal/flow/Instruction.java

 	 */
-	public void setPredecessor(final Instruction predecessor) {
+	public void setPredecessor(final Instruction predecessor,


Unrelated to this change but while trying to understand my own code after some time I wondered, why a instruction can't have multiple predecessors: While this is of course possible in byte code JaCoCo adds probes in a way that each instruction has at most one predecessor. Otherwise probes are inserted. Maybe we can improve documentation here:

Sets the given instruction as a direct predecessor to this instruction. Every instruction can have at most one direct predecessor otherwise probes are inserted in-between. This will add an branch to the predecessor.

What about adding a assertion to enforce this invariant?

if (this.predecessor != null) { throw new AssertionError(); }

@marchof I also was puzzled by this multiple times.

Maybe terms "direct predecessor" and "in-between" are a bit vague: in case of

L0: IFEQ L2 L1: NOP L2: NOP

L1 will not be marked as predecessor of L2 and there is probe "in-between" them
L2 will be marked as predecessor of L0, however not so "direct" (i.e. not a previous instruction) - via jump and there is an "in-between" probe that is executed on this jump from L0 to L2 😉

But your proposal anyway better than nothing.

I updated javadoc and added check of invariant using a slightly modified version of your proposal.

✅

@Godin Thanks, hopefully helps us and others with this tricky piece of code in future.

marchof · 2017-04-24T08:00:52Z

org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/FinallyFilter.java

+	private static boolean isSame(final int size, AbstractInsnNode e,
+			AbstractInsnNode n) {
+		for (int i = 0; i < size; i++) {
+			if (e.getOpcode() != n.getOpcode()) {


What about also checking operands here? Should be the same except for labels.

@marchof turns out that not only for labels: for example for

class Fun { static void nop(String s) { } void fun() { try { nop("1"); } finally { try { nop("2"); } catch (Exception e) { nop("catch " + e); } finally { nop("finally"); } } } }

javac will generate code that uses astore 1 and aload 1 on non exceptional path, while astore 4 and aload 4 on exceptional path. This can be easily seen in LocalVariableTable, which we can't use during comparison:

LocalVariableTable: Start Length Slot Name Signature 19 22 1 e Ljava/lang/Exception; 76 23 4 e Ljava/lang/Exception;

and now I recall that I already saw this during implementation of filter for try-with-resources. So let's have a talk about this tomorrow at GenevaJUG.

marchof · 2017-04-24T08:02:57Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Finally.java

+ */
+public class Finally {
+
+	private static void ex(boolean t) {


Move this to org.jacoco.core.test.validation.targets.Stubs?

@marchof done

marchof · 2017-04-24T08:04:10Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Finally.java

+/**
+ * This test target is a finally block.
+ */
+public class Finally {


I wonder whether filter works for nested finally blocks too. These create really ugly flows.

@marchof Do you think about some particular example of an "ugly flow"? or just nested? in any case we can talk/test tomorrow at GenevaJUG 😉

Just nested: A try/finally block within a finally block. This is probably too much 😉 .

Also a test for finally blocks with some control structures (e.g. if) would be a nice test for the isSame() method.

In fact we already have finally of try-with-resources nested into finally of another try-with-resources and this new filter changed assertion there 😉 but indeed - can add explicit one here.

@marchof About control structures such as if: there is already test of conditional jump that is generated for nop(i == 0), so I'm hesitating with addition of test for if statement - in this case why not add tests for other statements? and what about their relative order and so on? i.e. how to not go mad trying to generate all possible variants that are infinite in general.

Nested finally in his turn shows difference of slots for different duplicates of same finally block, which is not shown in existing tests here, so added it.

@Godin Ok, this is enough of control structures for now.

marchof · 2017-04-27T16:39:16Z

org.jacoco.core.test/src/org/jacoco/core/internal/flow/InstructionTest.java

+		try {
+			instruction.setPredecessor(predecessor, 0);
+			fail("exception expected");
+		} catch (IllegalStateException e) {


@Godin Why not @Test(expected=IllegalStateException.class)?

@marchof to isolate operation that throws since there was already code in this test

@Godin Ah I see. But in this case I would prefer to have a separate test case specifically for this scenario.

@marchof done
✅

@Godin 👍 Thanks!

marchof · 2017-05-06T10:24:44Z

@Godin I wouldn't consider or test functional correctness for compiler versions < 1.5. But our code should not throw exceptions on any valid class file supplied.

As a side note I think all matching should happen with well-unit-tested matching primitives (like provided by AbstractMatcher) which by their API design cannot throw NPEs in the specific filter implementations.

marchof · 2017-05-11T12:18:15Z

@Godin Another finding on the "big repo" by @p45q:

java.lang.IllegalStateException
at org.jacoco.core.internal.flow.Instruction.setPredecessor(Instruction.java:81)
at org.jacoco.core.internal.analysis.MethodAnalyzer.visitEnd(MethodAnalyzer.java:371)
at org.jacoco.core.internal.analysis.ClassAnalyzer$1.visitEnd(ClassAnalyzer.java:71)
at org.objectweb.asm.MethodVisitor.visitEnd(MethodVisitor.java:878)
at org.jacoco.core.internal.analysis.MethodAnalyzer.accept(MethodAnalyzer.java:142)
at org.jacoco.core.internal.flow.ClassProbesAdapter$2.visitEnd(ClassProbesAdapter.java:89)
at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1036)
at org.objectweb.asm.ClassReader.accept(ClassReader.java:708)
at org.objectweb.asm.ClassReader.accept(ClassReader.java:521)
at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:111)
at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:127)
... 5 more

Happens on a Eclipse EMF class of unknown version (packaged in some IBM product): ResourceImpl.zip

Godin · 2017-05-11T12:44:29Z

@marchof wow, thanks! However this one violates contract that was already here for method Instruction.setPredecessor, in this PR we just added check of this contract. Anyway I'll have a look.

marchof · 2017-05-11T13:09:09Z

@Godin That's why I was also considering opening a separate issue. If this is a bigger think we should move the assertion and fix into a separate PR.

Godin · 2017-05-11T15:43:20Z

@marchof here is what I've discovered for now:
This class also contains JSR and RET instructions, so that JSRInlinerAdapter kicks in before analysis. It adds labels, so that same instruction has multiple labels - comment about that was added in commit bead2e4 to MethodAnalyzer:

Due to ASM issue #315745 there can be more than one label per instruction

As a very simplified example of code after JSRInlinerAdapter for this class, which triggers IllegalStateException:

  ALOAD 1
  IFNE L1
  GOTO L2
 L1
 L2
  RETURN

And according to MethodAnalyzerTest in such case nextProbeId is 1, while expected 3 as in case with single label. So I have feeling that problem comes from LabelFlowAnalyzer that wasn't updated in commit bead2e4.

Godin · 2017-05-13T00:13:36Z

@marchof as was decided by phone: removed verification of contract of Instruction.setPredecessor and created separate ticket - #533

Godin · 2018-08-16T22:27:05Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Finally.java

+	 * Note that difference of this case from {@link #breakStatement()} is that
+	 * there is nothing between <code>break</code> statement and end of
+	 * <code>try</code> block. And current implementation of filter does not
+	 * handle such case.


Seems that filter that was implemented in #604 handles this case 😋

Godin · 2018-08-16T22:33:42Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Finally.java

+	/**
+	 * Note that javac generates <code>goto</code> instruction at the end of
+	 * <code>while</code> statement that refers to a line of previous
+	 * instruction. And so causes partial coverage of last line of finally


Seems that we forgot to preserve this case in #604.

Godin · 2018-08-16T22:46:34Z

org.jacoco.core.test/src/org/jacoco/core/test/filter/targets/Finally.java

+
+	/**
+	 * Note that in this case javac generates bytecode that uses different slots
+	 * in different duplicates of same finally block for variable that stores


Seems that we forgot to preserve this or at least similar comment in #604.

Godin added component: core.filters type: enhancement labels Apr 22, 2017

Godin added this to the 0.7.10 milestone Apr 22, 2017

Godin self-assigned this Apr 22, 2017

Godin added this to IN PROGRESS in Current work items Apr 22, 2017

Godin added this to TODO in Filtering Apr 22, 2017

Godin force-pushed the issue-521 branch from 291f4f5 to 8744042 Compare April 22, 2017 22:25

Godin changed the title ~~Merge duplicate finally blocks in a report~~ During creation of report merge duplicate finally blocks that compiler generates Apr 23, 2017

Godin force-pushed the issue-521 branch 2 times, most recently from 7f2f506 to 5d9e92f Compare April 23, 2017 14:16

Godin added 2 commits April 23, 2017 16:16

(WIP) yak shaving: for each instruction determine covered branches

77f6888

(WIP) yak shaving: merging of coverage of instructions

5f1dc98

Godin force-pushed the issue-521 branch from 5d9e92f to 81f23ed Compare April 23, 2017 19:16

(WIP) add filter for finally blocks

81f23ed

Godin requested a review from marchof April 23, 2017 19:28

Godin moved this from TODO to IN PROGRESS in Filtering Apr 23, 2017

marchof reviewed Apr 24, 2017

View reviewed changes

Godin and others added 4 commits April 24, 2017 23:47

Merge branch 'master' into issue-521

2f62465

(WIP) improve filter for ECJ

28b6158

(WIP) encapsulate data of Instruction

7b0651b

(WIP) clarify contract of method Instruction.setPredecessor

b198857

Godin force-pushed the issue-521 branch from d5393f8 to b198857 Compare April 27, 2017 16:15

marchof reviewed Apr 27, 2017

View reviewed changes

(WIP) add test with a finally block nested into another finally block

fdd6b67

(WIP) fix NPE

4e0c5d4

Godin force-pushed the issue-521 branch 3 times, most recently from 70fdc19 to 19eb9cb Compare May 6, 2017 23:03

Godin added 3 commits May 7, 2017 01:05

(WIP) add more tests

19eb9cb

(WIP) add more tests

d7b413e

(wip) add example of uncovered case

c7fa359

Godin mentioned this pull request May 12, 2017

Fix incorrect placement of probes in case of JSR and RET instructions #533

Open

Godin force-pushed the issue-521 branch from 443fb45 to 875ef11 Compare May 13, 2017 00:15

(WIP) remove verification of a contract in Instruction.setPredecessor

71f910e

Godin force-pushed the issue-521 branch from 875ef11 to 71f910e Compare May 13, 2017 00:21

Godin added 3 commits May 13, 2017 02:22

Merge branch 'master' into issue-521

baba750

(WIP) add more tests

29dedb3

(WIP) add link to JDK bug

2f48c35

Godin force-pushed the issue-521 branch 2 times, most recently from 191fdf9 to 102707e Compare May 13, 2017 13:33

(WIP) add example of uncovered case

102707e

Godin mentioned this pull request Jun 24, 2017

Recognize and ignore Kotlin-generated code #552

Closed

Merge branch 'master' into issue-521

607da44

Godin commented Aug 16, 2018

View reviewed changes

Godin moved this from Implementation to Review in Current work items Aug 16, 2018

Godin added the language: Java label Oct 12, 2023

During creation of report merge duplicate finally blocks that compiler generates #521

Are you sure you want to change the base?

During creation of report merge duplicate finally blocks that compiler generates #521

Conversation

Godin commented Apr 22, 2017 • edited Loading

Godin commented Apr 23, 2017

Choose a reason for hiding this comment

Godin Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

marchof Apr 24, 2017 • edited by Godin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchof commented Apr 24, 2017

Choose a reason for hiding this comment

Godin Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchof commented May 6, 2017

marchof commented May 11, 2017

Godin commented May 11, 2017

marchof commented May 11, 2017

Godin commented May 11, 2017 • edited Loading

Godin commented May 13, 2017

Godin Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Godin Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Godin commented Apr 22, 2017 •

edited

Loading

Godin Apr 24, 2017 •

edited

Loading

marchof Apr 24, 2017 •

edited by Godin

Loading

Godin Apr 26, 2017 •

edited

Loading

Godin Apr 27, 2017 •

edited

Loading

Godin Apr 28, 2017 •

edited

Loading

Godin commented May 11, 2017 •

edited

Loading

Godin Aug 16, 2018 •

edited

Loading

Godin Aug 16, 2018 •

edited

Loading