10000 Support Java 9 bytecode by adding "-target:jvm-1.9" by mkurz · Pull Request #6146 · scala/scala · GitHub
[go: up one dir, main page]

Skip to content

Support Java 9 bytecode by adding "-target:jvm-1.9" #6146

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

Closed
wants to merge 1 commit into from

Conversation

mkurz
Copy link
Contributor
@mkurz mkurz commented Oct 25, 2017

Thanks to upgrading scala-asm to 5.2-scala-2 in #6116 we now have support for Java 9 bytecode because asm now provides Opcodes.V1_9.

Note: asm 6.0 renames the constant to Opcodes.V9, see scala/scala-asm@f7f44a8

This should also be cherry-picked to the 2.13.x branch I assume.

@scala-jenkins scala-jenkins added this to the 2.12.5 milestone Oct 25, 2017
@mkurz
Copy link
Contributor Author
mkurz commented Oct 25, 2017

Just signed the CLA right now.

@mkurz
Copy link
Contributor Author
mkurz commented Oct 25, 2017

Maybe we should also use jvm-9 instead of jvm-1.9?

@adriaanm
Copy link
Contributor

Let's move taking advantage of Java 9 to 2.13.x, where new feature development is now focussed. The scope of 2.12 is limited to making sure the compiler and other tools run properly on Java 9. To do this, please use the edit button to select 2.13.x as your PR's base branch.

@SethTisue
Copy link
Member
SethTisue commented Oct 25, 2017

a possible counterargument is that emitting the higher bytecode version, without actually otherwise altering the bytecode, is a quick way for a library that requires Java 9 (e.g. because it uses Java 9 only APIs) to fail fast if inadvertently used on a lower version.

@mkurz we appreciate you looking at this and submitting the patch. are there any other arguments in favor of this (on 2.12.x) you want to make? adding this isn't a no-brainer, I'm afraid. adding it might raise expectations that we aren't prepared to meet in the 2.12.x series. such a flag wouldn't be backed by our normal testing regimen (we wouldn't be validating PRs with the new bytecode version, or running the community build with it), and it's entirely possible that the new bytecode version might be problematic in ways that we don't even know about at the moment.

on the balance I think I agree with Adriaan, but having given it some thought, I thought I'd write those thoughts down.

@adriaanm
Copy link
Contributor

Good point Seth — I should have phrased my comment more as a question. I’m open to discussing, but am indeed concerned about setting to high expectations for 2.12, which has to yield to 2.13 development.

@SethTisue SethTisue added the welcome hello new contributor! label Oct 25, 2017
@mkurz mkurz changed the base branch from 2.12.x to 2.13.x October 25, 2017 21:57
@mkurz mkurz changed the base branch from 2.13.x to 2.12.x October 25, 2017 21:58
@mkurz mkurz changed the base branch from 2.12.x to 2.13.x October 25, 2017 22:00
@mkurz
Copy link
Contributor Author
mkurz commented Oct 25, 2017

are there any other arguments in favor of this (on 2.12.x) you want to make?

No. I also can only think of the fail fast argument you made. I don't know if that is worth the trouble that may arise.

So I changed the base branch to 2.13.x (had to rebase as well). If you change your mind we can still backport later to the 2.12.x branch.

However do we want to keep jvm-1.9 or rename it to jvm-9?

@SethTisue
Copy link
Member
SethTisue 10000 commented Oct 25, 2017

However do we want to keep jvm-1.9 or rename it to jvm-9

I like jvm-9. Oracle switched, that's our chance to switch too. especially with Java 18 coming... 1.18 would be ridiculous :-)

@mkurz
Copy link
Contributor Author
mkurz commented Oct 25, 2017

Alright, updated!

@retronym
Copy link
Member

We need to tweak the encoding of final vals in traits before we allow the new classfile format. In short, the backing field in subclassed should no longer have the final modifier in bytecode, that constructor of the subclass should conclude with a call the VarHandle.fence to explicitly request the same memory barrier that the VM would have inserted if the field were JVM final.

We also need to add JDK 9 to our CI matrix, and work through test cases that fail. That doesn't strictly need to happen before this change, but we must realise that we're flying blind.

@retronym
Copy link
Member

Also, we should probably get rid of the jvm- prefix on the -target arguments, that's a hangover from the days of supporting .NET and now is an unnecessary deviation from the well known command line of javac. We could make this change in a way to still support the jvm- versions for backwards compatibility.

@adriaanm adriaanm modified the milestones: 2.12.5, 2.13.0-M4 Oct 26, 2017
@retronym retronym added the WIP label Nov 28, 2017
@mkurz
Copy link
Contributor Author
mkurz commented Dec 14, 2017

There is a new a new (scala-)asm version available - v6.0.0-scala-1: https://github.com/scala/scala-asm/releases

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
@mkurz
Copy link
Contributor Author
mkurz commented May 30, 2018

FYI: ASM 6.2 released with better JDK 10 support and even with JDK 11 support already!
http://asm.ow2.io/versions.html
https://gitlab.ow2.org/asm/asm/tags
https://gitlab.ow2.org/asm/asm/issues/317830

@retronym
Copy link
Member

Thanks for the heads up, @mkurz. I've rebased our ASM fork atop 6.2. Our test suite seems happy with the result. We'll need to do a bit of due diligence on the 6.0..6.2 diffs in ASM before we incorporate it. We'll definitely include this in Scala 2.13. I'd also like to include it in the next maintenance release of Scala 2.12.

@mkurz
Copy link
Contributor Author
mkurz commented May 30, 2018

@retronym Thanks! Great to hear that you will also backport it to the 2.12 branch.

BTW: I don't think I will have time to work on this open pull request here anymore, so everyone is welcome to take over. Thanks!

@retronym
Copy link
Member
retronym commented Jun 7, 2018

Here's the upgrade to ASM 6.2: #6733. We still can't offer -target 9 until #6425 is agreed upon and merged as we know our trait encoding does not comply with the verifier under that classfile version. Once those tasks are done, I'll submit your changes as a new PR.

@retronym retronym closed this Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
welcome hello new contributor!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0