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.

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

Upgrade ASM to 6.2 #706

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Upgrade ASM to 6.2 #706

merged 2 commits into from
Jul 3, 2018

Conversation

Godin
Copy link
Member

@Godin Godin commented Jul 2, 2018

No description provided.

@Godin Godin self-assigned this Jul 2, 2018
@Godin Godin changed the title Update ASM to 6.2 Upgrade ASM to 6.2 Jul 3, 2018
@Godin Godin force-pushed the asm-6.2 branch 2 times, most recently from 9304fab to e3486e8 Compare July 3, 2018 01:24
@Godin
Copy link
Member Author

Godin commented Jul 3, 2018

@marchof could you please have a look? I didn't update changelog to first discuss below points.


All changes in our tests, except one about ClassWriter.COMPUTE_FRAMES, are because starting from version 6.1 ASM is more strict about allowed arguments.


Should be noted that some regressions were already found in ASM 6.2, however to me seems that JaCoCo can't be affected by them, but might be worth double-checking - AFAIK as of 2 July 2018 from the list of issues (https://gitlab.ow2.org/asm/asm/issues) and changes after ASM 6.2 (https://gitlab.ow2.org/asm/asm/compare/ASM_6_2...master):

And seems that while https://gitlab.ow2.org/asm/asm/issues/317833 was fixed after 6.2, problem was already there with ASM 6.0, also we don't use ClassWriter.COMPUTE_MAXS and don't change number of arguments of methods.


Also should be noted that ASM 6.2 contains experimental support for the upcoming JDK 11 features (nest mates JEP-181 and ConstantDynamic JEP-309) - see https://mail.ow2.org/wws/arc/asm/2018-05/msg00004.html Furthermore ClassReader doesn't reject JDK 11 classes even if flag Opcodes.ASM7_EXPERIMENTAL is not used - see https://mail.ow2.org/wws/arc/asm/2018-05/msg00000.html

While seems that this allows to use our ModifiedSystemClassRuntime under JDK 11 and perform on-the-fly instrumentation of classes compiled into bytecode version 55 without other changes (ClassReader reads new ConstantDynamic constant pool entries and new nest mates attributes, ClassWriter copies them), just an update of ASM alone is clearly not enough for proper full support of JDK 11:

Also even if JDK 11 is now in Rampdown Phase One, AFAIK there is no strong guarantees that bytecode won't change, and this is also why support for JDK 11 features is experimental in ASM 6.2. IMO the best what we can do - is also "experimental support", i.e. without any guarantees. WDYT? In any case, I would prefer to separate ASM update from JDK 11 support.

@marchof
Copy link
Member

marchof commented Jul 3, 2018

@Godin +1 for adding experimental JDK 11 support in a separate PR

Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Godin
Copy link
Member Author

Godin commented Jul 3, 2018

@marchof and just to be sure - what about known regressions in ASM 6.2? do you agree with my analysis that they don't affect us? any concerns about possible others, given that 6.1 was a massive refactoring?

@marchof
Copy link
Member

marchof commented Jul 3, 2018

@Godin I agree with you that the regressions do not affect usage in JaCoCo. And hey, our tests are green ;)

@Godin
Copy link
Member Author

Godin commented Jul 3, 2018

@marchof well, as history shows - our tests are not totally ehaustive and bulletproof 😉

I updated changelog and will do the merge. Thank you for review!

@Godin Godin added this to the 0.8.2 milestone Jul 3, 2018
@Godin Godin merged commit 6cd17a8 into master Jul 3, 2018
@Godin Godin deleted the asm-6.2 branch July 3, 2018 13:41
@Godin
Copy link
Member Author

Godin commented Jul 4, 2018

@marchof in fact while

ClassReader reads new ConstantDynamic constant pool entries and new nest mates attributes, ClassWriter copies them

is correct, ClassVisitor without Opcodes.ASM7_EXPERIMENTAL throws UnsupportedOperationException when ClassReader encounters/visits nest mates 😞

@ijuma ijuma mentioned this pull request Jul 5, 2018
@Godin
Copy link
Member Author

Godin commented Jul 23, 2018

For the record: ticket to upgrade ASM in Eclipse Orbit - https://bugs.eclipse.org/bugs/show_bug.cgi?id=536922

@jacoco jacoco locked as resolved and limited conversation to collaborators Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants