Create an OpenJ9 JCL preprocessor config for Java 8 #72

pshipton · 2017-09-19T15:59:03Z

Create a new preprocessor configuration SIDECAR18-SE-OPENJ9 based on the
existing SIDECAR18-SE configuration which is used internally by IBM to
create Java 8 builds. Add the additional flag Sidecar18-SE-OpenJ9 and
use this flag to include additional stub classes and code that are
required to compile the OpenJDK JCL code when the OpenJ9 JCL is
overlayed.

Also enable the CUDA4J flag to include the com.ibm.cuda classes.

@keithc-ca @DanHeidinga @JasonFengJ9 @andrew-m-leonard

Signed-off-by: Peter Shipton Peter_Shipton@ca.ibm.com

DanHeidinga

Overall, this looks pretty reasonable.

DanHeidinga · 2017-09-19T17:00:04Z

jcl/jpp_configuration.xml

+		  flags="Sidecar18-SE-OpenJ9,CUDA4J"
+		  dependencies="SIDECAR18-SE"
+		  jdkcompliance="1.8">
+		<classpathentry kind="lib" path="/binaries/vm/third/rt-compressed.sun180.jar"/>


This relies on a binary file - a stripped down version of rt.jar - which isn't available here. Can it be based on a unmodified rt.jar? Or can we produce a script that knows how to strip rt.jar?

It can (now) be based on an unmodified rt.jar from Java 8, plus the tools.jar. There is no need to use a stripped down rt.jar. The modified version was created only to save repo & disk space, and build time. This part of the preprocessor configuration is only used by the Eclipse Java Preprocessor plugin, which at this time isn't contributed to open source. I don't see any reason to change from the existing classpathentry, or have an idea of what I would change it to.

DanHeidinga · 2017-09-19T17:02:05Z

jcl/src/java.base/share/classes/java/lang/ClassLoader.java

+static ClassLoader getClassLoader(Class<?> clz) {
+	if (null != clz) {
+		return clz.getClassLoader0();
+	} else {


< grumbles about else blocks after return unconditional returns >

(I don't expect this to be changed here as it was pre-existing code)

DanHeidinga · 2017-09-19T17:04:08Z

jcl/src/java.base/share/classes/java/lang/System.java

@@ -172,15 +172,19 @@ static void afterClinitInitialization() {
 		/*[PR JAZZ 58297] - continue with the rules defined by JAZZ 57070 - Build a Java 8 J9 JCL using the SIDECAR18 preprocessor configuration */
 		// Check the default encoding
 		/*[Bug 102075] J2SE Setting -Dfile.encoding=junk fails to run*/
-		/*[IF Sidecar19-SE]*/
+		/*[IF !Sidecar19-SE]*/
+		StringCoding.encode(new char[1], 0, 1);


Does moving this effect initialization order? We've had issues with initializing string encoding-related functions at the wrong time....

The resulting post processed code is the same in all cases.

From code history, StringCoding.encode was invoked before setErr & setOut for both Java 8 & 9 around five weeks ago. StringCoding.encode was moved after setErr & setOut unintentionally in a changeset for this code area. I suggest to put StringCoding.encode for Java 9 back to initial location in same order as Java 8 as following code snippet:
/*[IF Sidecar19-SE]*/
StringCoding.encode(String.LATIN1, new byte[1]);
/*[ELSE]*/
StringCoding.encode(new char[1], 0, 1);
/*[ENDIF]*/
/*[IF Sidecar19-SE|Sidecar18-SE-OpenJ9]*/
setErr(new PrintStream(new BufferedOutputStream(new FileOutputStream(FileDescriptor.err)), true));
setOut(new PrintStream(new BufferedOutputStream(new FileOutputStream(FileDescriptor.out)), true));
/*[IF Sidecar19-SE_RAWPLUSJ9]*/
setIn(new BufferedInputStream(new FileInputStream(FileDescriptor.in)));
/*[ENDIF]*/
/*[ELSE]*/
/*[PR s66168] - ConsoleInputStream initialization may write to System.err */
/*[PR s73550, s74314] ConsolePrintStream incorrectly initialized */
setErr(com.ibm.jvm.io.ConsolePrintStream.localize(new BufferedOutputStream(new FileOutputStream(FileDescriptor.err)), true));
setOut(com.ibm.jvm.io.ConsolePrintStream.localize(new BufferedOutputStream(new FileOutputStream(FileDescriptor.out)), true));
/*[ENDIF]*/

DanHeidinga · 2017-09-19T17:05:46Z

jcl/src/java.base/share/classes/java/lang/invoke/MethodHandle.java

@@ -1236,9 +1236,11 @@ BoundMethodHandle rebind() {
 		throw OpenJDKCompileStub.OpenJDKCompileStubThrowError();
 	}

+/*[IF Sidecar19-SE-OpenJ9]*/


Does this have to be Java 9 only? Given it's a compile stub, I'd be tempted to leave it in for Java 8 as well

Its not used in Java 8 atm, but it doesn't need to be Java 9 only. Including it makes Java 8 code slightly bigger for no reason, but I can include it if you like.

I'm fine with leaving it. Its written now and is unlikely to need to be modified being compile stubs.

keithc-ca · 2017-09-19T17:09:11Z

jcl/src/java.base/share/classes/java/lang/invoke/MemberName.java

@@ -29,6 +29,12 @@
 */

 final class MemberName {
+	/*[IF Sidecar18-SE-OpenJ9]*/
+	static final class Factory {


This wasn't previously needed: perhaps the condition ought to be ! Sidecar19-SE-OpenJ9?

A positive condition is better than a negative one, as the negative condition could end up including the code in unexpected places.

keithc-ca · 2017-09-19T17:09:35Z

jcl/src/java.base/share/classes/java/lang/invoke/LambdaForm.java

@@ -59,6 +59,12 @@
 		MethodHandle resolvedHandle() {
 			throw OpenJDKCompileStub.OpenJDKCompileStubThrowError();
 		}
+
+		/*[IF Sidecar18-SE-OpenJ9]*/
+		MethodHandle resolve() {


This wasn't previously needed: perhaps the condition ought to be ! Sidecar19-SE-OpenJ9?

A positive condition is better than a negative one, as the negative condition could end up including the code in unexpected places.

keithc-ca · 2017-09-19T17:16:38Z

Sidecar19-SE implies Sidecar18-SE: I think the use of flags in conditionals should assume that Sidecar19-SE-OpenJ9 implies Sidecar18-SE-OpenJ9.

pshipton · 2017-09-19T17:40:30Z

Makes sense, I can make Sidecar19-SE-OpenJ9 imply Sidecar18-SE-OpenJ9. I'll have to change the places which are Java 8 only to Sidecar18-SE-OpenJ9&!Sidecar19-SE-OpenJ9, which is only 3 places.

pshipton · 2017-09-19T21:53:13Z

I've left the separate commits for easier further review. I plan to squash them.

JasonFengJ9 · 2017-09-19T22:07:27Z

>> Sidecar19-SE implies Sidecar18-SE: I think the use of flags in conditionals should assume that Sidecar19-SE-OpenJ9 implies Sidecar18-SE-OpenJ9.
Just like to indicate that there will be two different Java 8 pConfigs, i.e., Sidecar18-SE for IBM Java 8 and Sidecar18-SE-OpenJ9 for OpenJ9 Java 8; however there will be only one Java 9 pConfig which is for OpenJ9 Java 9, Sidecar19-SE-OpenJ9 is going to become (or be replaced by) Sidecar19-SE after b148 Java 9 builds are removed.

Since there is no really b174 builds, those /*[INCLUDE-IF Sidecar18-SE-OpenJ9|Sidecar19-SE-B174]*/ can be replaced by /*[INCLUDE-IF Sidecar18-SE-OpenJ9]*/ instead.

Later when b148 Java 9 is removed, /*[IF Sidecar18-SE-OpenJ9|Sidecar19-SE]*/ can be replaced with /*[IF Sidecar18-SE-OpenJ9|Sidecar19-SE]*/ assuming Sidecar19-SE implies Sidecar18-SE-OpenJ9

pshipton · 2017-09-19T22:16:11Z

Agree with Jason's statements about preprocessor flag cleanup, but I think that is better left for a separate commit. Even though Sidecar19-SE-B174 is obsolete, I though it was better to leave it as-is than do a partial cleanup.

JasonFengJ9

lgtm

genie-openj9 · 2017-09-20T18:42:15Z

Can one of the admins verify this patch?

genie-openj9 · 2017-09-21T14:17:03Z

Can one of the admins verify this patch?

pshipton · 2017-09-21T14:21:33Z

Jenkins test sanity

pshipton · 2017-09-21T14:31:17Z

As per the merged #58, I've rebased and added the new openj9.jvm module to the source path of the SIDECAR18-SE-OPENJ9 jpp config

Create a new java preprocessor configuration SIDECAR18-SE-OPENJ9 based on the existing SIDECAR18-SE configuration which is used internally by IBM to create Java 8 builds. Add the additional flag Sidecar18-SE-OpenJ9 and use this flag to include additional stub classes and code that are required to compile the OpenJDK JCL code when the OpenJ9 JCL is overlayed. Sidecar19-SE-OpenJ9 also includes Sidecar18-SE-OpenJ9. Enable the CUDA4J flag to include the com.ibm.cuda classes. Enable the DAA flag to include the com.ibm.dataaccess classes. Move Java 9 initialization call to StringCoding.encode() earlier. This move matches how it was originally, until the call was inadvertently moved by an earlier change. Signed-off-by: Peter Shipton <Peter_Shipton@ca.ibm.com>

pshipton · 2017-09-21T20:06:24Z

I've squashed all the commits, this is ready to be merged.

DanHeidinga · 2017-09-21T20:15:35Z

I will merge this either later tonight or tomorrow depending on when the OMR-related build issue is merged across - see eclipse/omr#1710

edit: change merged - see eclipse-openj9/openj9-omr#5 for status of merges to openj9-omr/openj9

DanHeidinga · 2017-09-22T01:27:10Z

Jenkins test sanity

…n_fix Set default variables from extensions

DanHeidinga reviewed Sep 19, 2017

View reviewed changes

keithc-ca reviewed Sep 19, 2017

View reviewed changes

pshipton changed the title ~~Create an OpenJ9 JCL preprocessor configuration for Java 8~~ WIP Create an OpenJ9 JCL preprocessor configuration for Java 8 Sep 19, 2017

pshipton changed the title ~~WIP Create an OpenJ9 JCL preprocessor configuration for Java 8~~ Create an OpenJ9 JCL preprocessor configuration for Java 8 Sep 19, 2017

pshipton changed the title ~~Create an OpenJ9 JCL preprocessor configuration for Java 8~~ Create an OpenJ9 JCL preprocessor config for Java 8 Sep 19, 2017

JasonFengJ9 approved these changes Sep 20, 2017

View reviewed changes

pshipton added comp:build comp:vm jdk8 and removed comp:build labels Sep 20, 2017

pshipton force-pushed the java8 branch from 2130b07 to fa6d8f6 Compare September 21, 2017 14:17

pshipton force-pushed the java8 branch from fa6d8f6 to 1d21e4f Compare September 21, 2017 20:05

DanHeidinga approved these changes Sep 21, 2017

View reviewed changes

DanHeidinga self-assigned this Sep 21, 2017

DanHeidinga merged commit 957c253 into eclipse-openj9:master Sep 22, 2017

babsingh pushed a commit to babsingh/openj9 that referenced this pull request Jul 4, 2018

Merge pull request eclipse-openj9#72 from Theresa-T-Mammarella/versio…

feff8ab

…n_fix Set default variables from extensions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an OpenJ9 JCL preprocessor config for Java 8 #72

Create an OpenJ9 JCL preprocessor config for Java 8 #72

pshipton commented Sep 19, 2017

DanHeidinga left a comment

DanHeidinga Sep 19, 2017

pshipton Sep 19, 2017 •

edited

DanHeidinga Sep 19, 2017

DanHeidinga Sep 19, 2017

pshipton Sep 19, 2017

JasonFengJ9 Sep 19, 2017

DanHeidinga Sep 19, 2017

pshipton Sep 19, 2017

DanHeidinga Sep 19, 2017

keithc-ca Sep 19, 2017

pshipton Sep 19, 2017

keithc-ca Sep 19, 2017

pshipton Sep 19, 2017

keithc-ca commented Sep 19, 2017

pshipton commented Sep 19, 2017 •

edited

pshipton commented Sep 19, 2017

JasonFengJ9 commented Sep 19, 2017

pshipton commented Sep 19, 2017

JasonFengJ9 left a comment

genie-openj9 commented Sep 20, 2017

genie-openj9 commented Sep 21, 2017

pshipton commented Sep 21, 2017

pshipton commented Sep 21, 2017

pshipton commented Sep 21, 2017

DanHeidinga commented Sep 21, 2017 •

edited

DanHeidinga commented Sep 22, 2017

Create an OpenJ9 JCL preprocessor config for Java 8 #72

Create an OpenJ9 JCL preprocessor config for Java 8 #72

Conversation

pshipton commented Sep 19, 2017

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton Sep 19, 2017 • edited

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

keithc-ca commented Sep 19, 2017

pshipton commented Sep 19, 2017 • edited

pshipton commented Sep 19, 2017

JasonFengJ9 commented Sep 19, 2017

pshipton commented Sep 19, 2017

JasonFengJ9 left a comment

Choose a reason for hiding this comment

genie-openj9 commented Sep 20, 2017

genie-openj9 commented Sep 21, 2017

pshipton commented Sep 21, 2017

pshipton commented Sep 21, 2017

pshipton commented Sep 21, 2017

DanHeidinga commented Sep 21, 2017 • edited

DanHeidinga commented Sep 22, 2017

pshipton Sep 19, 2017 •

edited

pshipton commented Sep 19, 2017 •

edited

DanHeidinga commented Sep 21, 2017 •

edited