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

Java 15 Use Unsafe.defineAnonyousClass() to define Hidden classes. #9485

Merged
merged 5 commits into from
May 21, 2020

Conversation

hangshao0
Copy link
Contributor

Add implementation of new Hidden class APIs to unblock the JDK15 builds.

issue #9328

Signed-off-by: Hang Shao [email protected]

pshipton added 3 commits May 6, 2020 14:26
Related to 8242452: During module definition, move conversion of
packages from native to VM

The signature of JVM_DefineModule has been modified from `..., const
char* const* packages, jsize numPackages` to `..., jobjectArray
packages`.

Also the package parameter of following has been modified from char* to
jstring.
JVM_AddModuleExports
JVM_AddModuleExportsToAllUnnamed
JVM_AddModuleExportsToAll

Fixes eclipse-openj9#9293

Signed-off-by: Peter Shipton <[email protected]>
[ci skip]

Signed-off-by: Peter Shipton <[email protected]>
@hangshao0
Copy link
Contributor Author

This PR is based on #9292. The last commit is my change. Other changes have already been review under #9292.

@hangshao0
Copy link
Contributor Author

Reviewer: @pshipton

@hangshao0
Copy link
Contributor Author

More change is required to resolve #9328. The change here is to unblock building of the latest jdk15.

@pshipton pshipton requested a review from DanHeidinga May 7, 2020 18:26
@pshipton pshipton changed the title Use Unsafe.defineAnonyousClass() to define Hidden classes. Java 15 Use Unsafe.defineAnonyousClass() to define Hidden classes. May 7, 2020
@pshipton
Copy link
Member

pshipton commented May 7, 2020

jenkins test sanity zlinux jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

@pshipton pshipton changed the title Java 15 Use Unsafe.defineAnonyousClass() to define Hidden classes. WIP Java 15 Use Unsafe.defineAnonyousClass() to define Hidden classes. May 7, 2020
@pshipton
Copy link
Member

pshipton commented May 7, 2020

Set as WIP since when this is merged, the jdk openj9-staging branch also needs to be promoted. We'll need to run a full acceptance build first.

I should also mention that openj9-staging is typically updated every night, so this is a moving target. Although we can promote the SHA that was validated.

@keithc-ca
Copy link
Contributor

keithc-ca commented May 7, 2020

jenkins test sanity zlinux jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

That didn't work: it built openj9 instead of openj9-staging. Oops, looking at the wrong part of the log.

@AdamBrousseau
Copy link
Contributor

The trigger is open enough that any type of text/code/quoting jenkins followed by compile or test will trigger a build.

@keithc-ca
Copy link
Contributor

This error is unexpected:

[2020-05-07T18:42:21.333Z] /home/jenkins/workspace/Build_JDKnext_s390x_linux_Personal/openj9/debugtools/DDR_VM/src/com/ibm/j9ddr/view/dtfj/image/J9DDRImage.java:336: error: method does not override or implement a method from a supertype
[2020-05-07T18:42:21.333Z] 	@Override
[2020-05-07T18:42:21.333Z] 	^

I'll investigate.

@pshipton
Copy link
Member

pshipton commented May 7, 2020

The build error is the following which I think we've seen before. @keithc-ca are you going to fix this one?

/home/jenkins/workspace/Build_JDKnext_s390x_linux_Personal/openj9/debugtools/DDR_VM/src/com/ibm/j9ddr/view/dtfj/image/J9DDRImage.java:336: error: method does not override or implement a method from a supertype
 	@Override

@keithc-ca
Copy link
Contributor

keithc-ca commented May 7, 2020

It's unclear how that could happen. The DDR.gmk makefile is being executed for the openj9.dtfj-ddr-jar target which depends upon openj9.dtfj-ddr-gen which in turn depends on openj9.dtfj-java: when openj9.dtfj-java has completed we should expect the class files for that module to be available.

The previous problem was due to improper javac options, but that has been fixed: see ibmruntimes/openj9-openjdk-jdk#199.

@keithc-ca
Copy link
Contributor

Jenkins test sanity zlinux jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

@keithc-ca
Copy link
Contributor

Jenkins test sanity zlinux jdknext depends keithc-ca/openj9-openjdk#makefiles

@pshipton
Copy link
Member

Jenkins test sanity zlinux jdknext depends keithc-ca/openj9-openjdk-jdk#makefiles

@keithc-ca
Copy link
Contributor

That incantation doesn't appear to work: I think it needs to reference a pull request (instead of a branch in some random repository).

@pshipton
Copy link
Member

That incantation doesn't appear to work: I think it needs to reference a pull request (instead of a branch in some random repository).

The one you started had the wrong repo name, openj9-openjdk instead of openj9-openjdk-jdk

@pshipton
Copy link
Member

The machines are offline for a short while for sanitizing, the job is waiting on a machine. We'll see if it works in a bit.

@keithc-ca
Copy link
Contributor

The one you started had the wrong repo name, openj9-openjdk instead of openj9-openjdk-jdk

No, that is the name I use for that repo (I omit the -jdk segment).

@pshipton
Copy link
Member

Jenkins test sanity zlinux jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
runtime/j9vm/java11vmi.c Show resolved Hide resolved
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity aix,zlinux,win jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

@DanHeidinga
Copy link
Member

Running testing on a limited subset of platforms to catch any compiler errors given previous test runs were largely clean

@pshipton
Copy link
Member

@DanHeidinga note that ibmruntimes/openj9-openjdk-jdk#openj9-staging is a moving target, it's typically updated every night. We should do a full run of the platforms*, and then merge this and promote openj9-staging before it gets updated in the nightly merge from OpenJDK. Otherwise, the tested openj9-staging sha can be promoted manually, which hopefully is just a push if no additional jdknext PRs get merged in the meantime.

  • the subset of platforms tested by the acceptance build are:
    ppc64_aix,x86-64_linux,x86-64_linux_xl,x86-64_linux_cm,ppc64le_linux,s390x_linux,x86-64_windows,x86-64_mac

@pshipton
Copy link
Member

The AIX failure is

14:23:59  /home/jenkins/workspace/Build_JDKnext_ppc64_aix_Personal/build/aix-ppc64-server-release/support/j9jcl_sources/java.base/share/classes/java/lang/Access.java:61: error: Access is not abstract and does not override abstract method stringConcatInitialCoder() in JavaLangAccess
14:23:59  final class Access implements JavaLangAccess {
14:23:59        ^

@pshipton
Copy link
Member

@hangshao0 can you please stub the new method so we can re-test.

@hangshao0
Copy link
Contributor Author

can you please stub the new method so we can re-test.

Done.

runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved
@keithc-ca
Copy link
Contributor

Jenkins test sanity aix,osx,plinux,win,xlinux,xlinuxcm,xlinuxxl,zlinux jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

@pshipton
Copy link
Member

Jenkins test sanity aix,zlinux,win,osx jdknext depends ibmruntimes/openj9-openjdk-jdk#openj9-staging

@pshipton
Copy link
Member

pshipton commented May 20, 2020

sorry @keithc-ca, for some reason I didn't see your PR build before I started mine. I stopped mine, yours is still running https://ci.eclipse.org/openj9/job/PullRequest-OpenJ9/3621/

@pshipton
Copy link
Member

@hangshao0 pls fix the commit title for "Add Javadoc to ClassOption and defineHiddenClass()". This title is misleading since the commit doesn't only add javadoc changes.

@hangshao0
Copy link
Contributor Author

hangshao0 commented May 21, 2020

Added more info into the title.

@keithc-ca
Copy link
Contributor

@hangshao0 Can you please update the commit comment again?
The first line should be no more than 70 characters and the second line should be blank:
see https://github.com/eclipse/openj9/blob/master/CONTRIBUTING.md.

1. Use copyStringToUTF8Helper() to copy and covert packageName
2. Add Javadoc to ClassOption and defineHiddenClass().
3. Add implementation of stringConcatInitialCoder() in Access.java

issue eclipse-openj9#9328

Signed-off-by: Hang Shao <[email protected]>
@hangshao0
Copy link
Contributor Author

Can you please update the commit comment again?

Done.

@keithc-ca
Copy link
Contributor

Jenkins test sanity aix,osx,win,win32,zlinux jdk8

@keithc-ca
Copy link
Contributor

Jenkins test sanity aix,osx,plinux,win jdk11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants