Upgrade to ASM 6 #600

andrioli · 2017-09-27T15:29:23Z

Upgrade to new ASM release 6.0. This version supports Java 9 class files, so I also removed Java9Support class.

This ASM version comes with a module-info.class file (major 53) but the shade plugin don't like it. So I also updated the maven-shade-plugin.

The shade plugin will output some warnings:

[WARNING] Discovered module-info.class. Shading will break its strong encapsulation.

I tried to apply a filter but the filter have no effect removing the warn message. Looking in the bundled JAR the module-info.class looks to not be included. So I think is ok to ignored the warn.

I built with success in my machine with Maven 3.5.0 and JDK 1.7.0_80, 1.8.0_144 and 9+181. Also tried with Maven 3.2.5 and JDK 1.6.0_65

andrioli · 2017-09-27T16:01:05Z

org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java

-					new ClassReader(Java9Support.downgradeIfRequired(buffer)));
-		} catch (final RuntimeException cause) {
+			analyzeClass(new ClassReader(buffer));
+		} catch (final Exception cause) {


Note that here I changed from RuntimeException to Exception

Wondering why? how this is related to ASM 6? I'm asking because we had some improvements and quite some nasty regressions/missed cases of exception handling exactly here.

Initially, I got some tests with errors (the test was expecting an exception with a actual message, but null was given).

Later I noticed that the method Instrumenter.instrument(InputStream...) need to be wrapped with a try-catch block (noticed by looking the previous state in the git history).

But I didn't know if it was related

@Godin I double checked my changes. Looks that this can be reverted.

andrioli · 2017-09-27T16:01:49Z

org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java

-		} catch (final IOException e) {
-			throw analyzerError(location, e);
+			analyzeClass(new ClassReader(input));
+		} catch (final Exception cause) {


And here from IOException to Exception

Also wondering why? and how this is related to ASM 6?

Same comment here, I got a test with an exception (I think was an ArrayIndexOutOfBoundsException in this case)

But reverting this change I got Failed tests: testAnalyzeClass_BrokenStream(org.jacoco.core.analysis.AnalyzerTest): expected:<Error while analyzing BrokenStream.> but was:<null>

marchof · 2017-09-27T17:06:37Z

@andrioli Thanks for this nice and clean contribution!

@Godin Beside change log entry this PR looks good to me. Local IDE and builds are ok.

marchof · 2017-09-27T17:10:29Z

@andrioli Also nice that you managed to open PR #600 for ASM 6. 😄

Godin · 2017-09-27T17:48:38Z

@andrioli thank you for your contribution 👍 and congratulations with round number 600 😉

Upgrade to new ASM release 6.0.

This is definitely good thing since should bring fixes for bugs reported by us.

@marchof I believe that we would need to add tests for them to prevent regressions since they are not tested in ASM explicitly. Also maybe will be better to do this update after finishing work on "filtering" - to ease its integration into Eclipse EclEmma, recall that it consumes ASM from Eclipse Orbit.

The shade plugin will output some warnings:
[WARNING] Discovered module-info.class. Shading will break its strong encapsulation.
I tried to apply a filter but the filter have no effect removing the warn message. Looking in the bundled JAR the module-info.class looks to not be included. So I think is ok to ignored the warn.

This is unavoidable warning, meaning that initially non-exported packages became visible at least for classes in final JAR - see mention of this in announcement of release of maven-shade-plugin version 3.1.0 (http://markmail.org/message/s6ogzz6h2j72xno6).

Godin · 2017-09-27T18:07:44Z

org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java

-					new ClassReader(Java9Support.downgradeIfRequired(buffer)));
-		} catch (final RuntimeException cause) {
+			analyzeClass(new ClassReader(buffer));
+		} catch (final Exception cause) {


Wondering why? how this is related to ASM 6? I'm asking because we had some improvements and quite some nasty regressions/missed cases of exception handling exactly here.

Godin · 2017-09-27T18:09:46Z

org.jacoco.core/src/org/jacoco/core/analysis/Analyzer.java

-		} catch (final IOException e) {
-			throw analyzerError(location, e);
+			analyzeClass(new ClassReader(input));
+		} catch (final Exception cause) {


Also wondering why? and how this is related to ASM 6?

Godin · 2017-09-27T18:20:05Z

org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java

 		try {
-			bytes = Java9Support.readFully(input);
+			return instrument(new ClassReader(input));


If instrumentation fails, then exception won't have location information due to this usage of instrument(ClassReader) instead of previously used instrument(byte[], String). Maybe simply preserve readFully?

This reminds me again recurring old wish to deprecate instrument(ClassReader) or at least add a note.

@Godin IMHO we can even just remove it. But it's a separate issue though.

Godin · 2017-09-27T18:22:59Z

org.jacoco.core/src/org/jacoco/core/instr/Instrumenter.java

+		try {
+			output.write(instrument(input, name));
+		} catch (final RuntimeException e) {
+			throw instrumentError(name, e);


IMO addition of this is an indication of regression in instrument(InputStream, String).

And if we don't wrap it with a try-catch block we got Tests in error: testInstrumentAll_BrokenClassFileInZip(org.jacoco.core.instr.InstrumenterTest): 67 that is the ArrayOutOfBoundsException

@andrioli if you fix the regression in instrument(InputStream, String) described in #600 (comment) , then test won't be failing and try-catch won't be needed here.

marchof · 2017-09-27T18:46:29Z

@Godin As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

But this is not something we can request from @andrioli .

Godin · 2017-09-27T19:20:09Z

As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

@marchof Don't know specifically about PDE, but AFAIK for Oxygen.1a, whose focus is Java 9, there were no interest in ASM 6 BETA (see https://dev.eclipse.org/mhonarc/lists/orbit-dev/msg04882.html) and AFAIK final version was released too late for inclusion into Oxygen.1a.

But that's doesn't matter: by saying "ease integration", I meant exactly "easier", otherwise will just need to work on its addition as did for the two previous versions. There are quite some mess in sources of ASM published to Maven Central as usual, but maybe I'll find time to deal with this and raise CQ for addition.

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

😆

But this is not something we can request from @andrioli .

This wasn't a request, just a remark/reminder for us.

andrioli · 2017-09-28T02:22:38Z

Done. @Godin

Now is even easier to understand the changes. Unfortunately Travis is failing. Looks that the JDK download URL is not valid. Maybe you can fix the environment variables and re-run the jobs to keep everything green 😄

Godin · 2017-09-28T19:44:35Z

@andrioli Thanks! We'll merge this later.

@marchof I raised CQ for ASM 6 and it is already approved, so I'll raise PR for addition to Eclipse Orbit soon. However migration from Hudson to Jenkins for Orbit has been requested today, so might be delays in merges of PRs and builds.

Godin · 2017-10-13T11:13:32Z

org.jacoco.core/src/org/jacoco/core/internal/Files.java

+/**
+ * File handling utilities
+ */
+public final class Files {


This is not just about files, because method inside this class works for any InputStream, so better to change name. I'll do this during rebase.

Godin · 2017-10-13T16:36:14Z

org.jacoco.core.test/src/org/jacoco/core/test/validation/ResizeInstructionsTest.java

@@ -59,8 +58,8 @@ public void teardown() {
 	@Test
 	public void should_not_loose_InnerClasses_attribute() throws Exception {


After upgrade this test fails on JDK 5 due to another ASM bug - https://gitlab.ow2.org/asm/asm/issues/317800 😆

However ASM 5.2 already had an issues in a similar scenario - see #487

Given that scenarios are limited to class files without frames and with big methods that require wide jumps, and that there was problem #487 in the exact same scenarios, I do not consider this as regression.

In these scenarios after update of ASM, we'll be producing class files with frames that are not supposed to be present. While this can cause problems of reading of produced classes by the ASM as described in https://gitlab.ow2.org/asm/asm/issues/317800 , seems that JVM can read such classes and I believe that doesn't perform validation of frames, so this should not cause execution problems.

And taking into account that this update will resolve #487, #544, #584 and #606, I propose to update test to make it pass, and merge update into master.

Also seems that we can implement removal of these unwanted frames after instrumentation if there were no frames in original class - see my comment in ASM issue (https://gitlab.ow2.org/asm/asm/issues/317800#note_9609). However I propose to defer this and merge update without this - can implement later and maybe soon there will be ASM 6.0.1 with fix (https://gitlab.ow2.org/asm/asm/merge_requests/46).

@marchof WDYT?

@Godin Let's move forward to 6.0 without the workaround -- a you propose. I assume that 6.0.1 will be there before we get the first bug report for old class files.

andrioli commented Sep 27, 2017

View reviewed changes

Godin self-requested a review September 27, 2017 16:19

Godin self-assigned this Sep 27, 2017

Godin added this to the ASM milestone Sep 27, 2017

Godin added component: core type: enhancement labels Sep 27, 2017

Godin requested changes Sep 27, 2017

View reviewed changes

Godin requested changes Oct 13, 2017

View reviewed changes

Godin force-pushed the upgrade-asm branch from ee44b77 to 26e6909 Compare October 13, 2017 11:56

Godin reviewed Oct 13, 2017

View reviewed changes

Godin force-pushed the upgrade-asm branch 3 times, most recently from fcad229 to c68f464 Compare October 19, 2017 20:43

Godin modified the milestones: ASM, 0.8.0 Oct 19, 2017

Godin force-pushed the upgrade-asm branch from c68f464 to 3f00a3c Compare October 19, 2017 21:37

Godin approved these changes Oct 19, 2017

View reviewed changes

Upgrade ASM to 6.0

3f00a3c

Godin merged commit caa820e into jacoco:master Oct 19, 2017

This was referenced Oct 19, 2017

NullPointerException when instrumenting module-info.class #544

Closed

getEnclosingClass() and getDeclaringClass() return null [reflection] #584

Closed

NegativeArraySizeException #487

Closed

Godin mentioned this pull request Dec 9, 2017

Support JDK 10 #629

Merged

This was referenced Dec 23, 2017

Offline instrumentation should not damage module-info #634

Merged

Restore exec files compatibility regarding Java 9 class files #636

Merged

jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

		@@ -59,8 +58,8 @@ public void teardown() {
		@Test
		public void should_not_loose_InnerClasses_attribute() throws Exception {

Upgrade to ASM 6 #600

Upgrade to ASM 6 #600

Uh oh!

Conversation

andrioli commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrioli Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrioli Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marchof commented Sep 27, 2017

Uh oh!

marchof commented Sep 27, 2017

Uh oh!

Godin commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marchof commented Sep 27, 2017

Uh oh!

Godin commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrioli commented Sep 28, 2017

Uh oh!

Godin commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrioli Sep 27, 2017 •

edited

Loading

andrioli Sep 27, 2017 •

edited

Loading

Godin commented Sep 27, 2017 •

edited

Loading

Godin commented Sep 27, 2017 •

edited

Loading