Support composing 32 bit builds for arm #88

JamesKingdon · 2017-11-22T11:26:49Z

With all the review changes the previous PR became obsolete. This PR now covers all the changes required (in this project) for a working 32 bit cross compile hosted on x86 and targeting armhf.

jdekonin · 2017-11-22T13:28:29Z

closed/OpenJ9.gmk

@@ -28,6 +28,14 @@ ifeq (,$(BUILD_ID))
  BUILD_ID := 000000
 endif

+# 64 bit compressedrefs builds put some of the libraries in lib/compressedrefs.
+# What's the appropriate directory for a 32 bit build? I've opted for 'default'.
+ifeq (1,$(OPENJDK_DOCKER_CC_ARM))  # condition should be more specific to 32bit


This limits to arm only, could this not be set at the time platform is determined within the custom-hook-m4?

Hi Joe, my objective with this set of changes is to take the shortest path that isn't totally objectionable to getting a working arm build. For sure there will be better solutions to most of the problems, and where that's obvious I'm adding comments to the effect. I'd like to get these changes in so that we can have the platform building and testing (and therefore preventing ever more bit-rot in the arm code base). I would suggest we open issues for future improvements and then people that actually understand how the build system works can implement gold standard solutions.

jdekonin · 2017-11-22T13:36:38Z

closed/OpenJ9.gmk

 $(foreach file,$(OPENJ9_MISC_FILES) $(OPENJ9_PROPERTY_FILES),$(eval $(call openj9_copy_file,$2/lib/$(file),$(OUTPUT_ROOT)/vm/$(file))))
 $(foreach file,$(OPENJ9_NOTICE_FILES),$(eval $(call openj9_copy_file,$2/$(file),$(SRC_ROOT)/$(file))))
 $(foreach file,$(OPENJ9_REDIRECTOR),$(eval $(call openj9_copy_file,$2/$(OPENJ9_LIBS_OUTPUT_DIR)/j9vm/$(LIBRARY_PREFIX)jvm$(SHARED_LIBRARY_SUFFIX),$(OUTPUT_ROOT)/vm/$(file))))
-stage_openj9_shared_libraries_$1 := $(addprefix $2/$(OPENJ9_LIBS_OUTPUT_DIR)/compressedrefs/,$(OPENJ9_SHARED_LIBRARIES))
+ifeq (1,$(OPENJDK_DOCKER_CC_ARM))


We could determine if its a cross compile if we compared OPENJDK_TARGET_* and OPENJDK_BUILD_*. Might be better to determine this at configure time. This would keep it more generic.

jdekonin · 2017-11-22T13:37:58Z

closed/OpenJ9.gmk

@@ -210,7 +233,12 @@ define openj9_copy_tree_impl
  @$(TOUCH) $1/$(OPENJ9_MARKER_FILE)
 endef

-$(eval $(call generated_target_rules_build,build_jdk,$(BUILD_JDK)))
+# Temporary fix until we work out how to handle the build_jdk for cross compiles


Does generating the rule cause problems if its not being run via the change to closed/make/Main.gmk?

It certainly did at one point in time. I don't know if any of my more recent changes have made any difference to that. The generated_target_rules* behaviour is remarkably opaque, even after several hours of make --debug hacking.

jdekonin · 2017-11-22T13:47:46Z

closed/make/Main.gmk

@@ -30,7 +30,11 @@ j9vm-build :
 	+($(CD) $(SRC_ROOT)/closed && $(MAKE) -f OpenJ9.gmk SPEC=$(SPEC) VERSION_MAJOR=$(VERSION_MAJOR) build-j9)

 j9vm-compose-buildjvm : j9vm-build


Not sure this is the correct spot for this, I'm just not sure where is atm. Openjdk should already have a case to handle cross compilation and not be generating a buildjdk in that case. More than likely we have added the 'j9vm-compose-buildjvm' dependency to the wrong target.

make/Main.gmk...
java.base-jmod: j9vm-compose-buildjvm
...should likely be about 10-15 lines down in the CREATE_BUILDJDK ifeq.

Yes, Openjdk works entirely as expected (I started with an Openjdk/hotspot build before trying to build OpenJ9). It's certainly something we broke. Although to be more specific, it handles the cross compile and builds the appropriate build jdk for the host platform.

JamesKingdon · 2017-11-23T01:58:58Z

First version was naive in my use of ifeq inside the define clause and didn't work for x86. I've updated and tested on both platforms.

jdekonin

After thinking on it, I don't believe this is the right approach using OPENJDK_DOCKER_CC_ARM set in the host system to indicated its a cross-compile. All variables in use are determined based on configure and stored in either the spec.gmk or custom-spec.gmk.

The variable, OPENJDK_DOCKER_CC_ARM, is very specific for cross compilation and shouldn't be introduced specific to arm or docker. There should be an addition made in the custom-hook.m4 for a 'CROSS_COMPILE' variable (assuming one doesn't already exist, I didn't look extensively) as well as setting LIB_SUBDIR (better OPENJ9_LIB_SUBDIR to be consistent with variable naming) on openj9 platform selection/designation.

JamesKingdon · 2017-11-23T15:51:48Z

Ok, I can circle around and see what I can do. I may need further guidance though, as build scripts are not my thing.

JamesKingdon · 2017-12-01T01:47:40Z

@jdekonin I reworked the changes to get rid of the OPENJ9_DOCKER_CC_ARM flag.

keithc-ca · 2017-12-04T20:41:14Z

closed/OpenJ9.gmk

+	$$(stage_openj9_redirector_$1) \
+	$$(stage_openj9_property_files_$1)
+endef
+else	# Not an ARM cross compile


The above looks very similar to what follows. I'd much prefer to avoid this repetition if possible.

My first attempt tried to put the conditional just around the extra libraries, but it didn't work. This version is clearly not very elegant, but it's simple to understand and it works. I'd be happy to change it, but my makefile skills are not sufficient.

@keithc-ca How strongly do you feel about this? I don't know how to fix it, but I'd be happy to make changes if you show me how.

I feel strongly enough to request you give it another try.

keithc-ca · 2017-12-04T20:42:58Z

closed/autoconf/custom-hook.m4

@@ -152,15 +152,22 @@ AC_DEFUN([OPENJ9_PLATFORM_EXTRACT_VARS_FROM_CPU],
    powerpc64)
      OPENJ9_CPU=ppc-64
      ;;
+    arm)
+      OPENJ9_CPU=arm
+      ;;


Changes to autoconf files are expected to be accompanied by regenerated configure scripts.

OK, I wasn't sure if I was allowed to check in the generated scripts. I can add those.

Scripts regenerated and added to the PR

jdekonin · 2017-12-04T18:50:51Z

common/conf/jib-profiles.js

@@ -928,6 +928,9 @@ var getJibProfilesDependencies = function (input, common) {
        linux_arm: (input.profile != null && input.profile.indexOf("hflt") >= 0
                    ? "gcc-linaro-arm-linux-gnueabihf-raspbian-2012.09-20120921_linux+1.0"
                    : "arm-linaro-4.7+1.0")
+        linux_arm_docker: (input.profile != null && input.profile.indexOf("hflt") >= 0


I don't see where this would be a required to openjdk. The uses of linux_arm_docker above are specific to openj9.

Excellent. I wasn't sure what that was for and duplicated the block as part of a "find all the uses of linux_arm" exercise. I've confirmed that the JVM can be configured and built without that change, so I'll take it back out.

Changes removed.

jdekonin · 2017-12-04T18:56:12Z

closed/OpenJ9.gmk

@@ -28,6 +28,14 @@ ifeq (,$(BUILD_ID))
  BUILD_ID := 000000
 endif

+# 64 bit compressedrefs builds put some of the libraries in lib/compressedrefs


This entire block should be done once at configure time.

@jdekonin , I'm happy to make that change. Where/how should it be done?

I don't see why this couldn't be set at the same time OPENJ9_PLATFORM_CODE or OPENJ9_BUILDSPEC are defined.

Got it. I added a new variable to custom_hook.m4 in the same place as OPENJ9_PLATFORM_CODE is set. It took me a couple of hours to debug why the value wasn't visible from OpenJ9.gmk, but it works now.

jdekonin · 2017-12-04T21:39:05Z

closed/OpenJ9.gmk

@@ -164,13 +172,43 @@ endef
 # ----------------------
 # param 1 = The jdk/jre directory name
 # param 2 = The jdk/jre directory to add openj9 content
+ifeq (xr32,$(OPENJ9_PLATFORM_CODE))


What are the files that are missed if the 'buildjvm' target(s) aren't called?

Possibly not a complete list, but these are the libraries that for a while I was manually copying in order to compose the build:

libomrsig.so libj9vm29.so libj9thr29.so libj9hookable29.so libj9prt29.so \ libffi29.so libhyprtshim29.so libhythr.so libj9dmp29.so libj9trc29.so \ libj9zlib29.so libj9jit29.so libj9gc29.so libj9vrb29.so libj9jvmti29.so \ libjclse9_29.so libj9shr29.so libjvm.so ../j9vm/libjvm.so

There seemed to be good overlap between this set and the files defined for OPENJ9_JAVA_BASE_LIBRARIES and OPENJ9_SHARED_CLASSES_LIBRARIES.

JamesKingdon · 2017-12-12T02:25:42Z

@jdekonin @keithc-ca I've squashed the commits, any more changes required?

JamesKingdon · 2017-12-18T13:08:31Z

@jdekonin Any more feedback on this PR?

jdekonin · 2017-12-18T16:08:44Z

The only part I question now is surrounding the openj9.gmk:generated_target_rules macro. I don't see why the targets need be removed if they are not being called. The extra targets specific to cross compile I can understand, but the extra duplicating isn't very 'clean'.

I attempted to build last week, but there appeared to be some omr/j9 native compilation issues. I seem to have gotten past that today, but its failing to compose.

Cross compilation detected, skipping generated_target_rules_build
adding extra libs for arm cross compile
adding extra libs for arm cross compile
make[4]: *** No rule to make target 'stage_openj9_build_jdk'.  Stop.
/root/openj9-openjdk-jdk9/closed/make/Main.gmk:34: recipe for target 'j9vm-compose-buildjvm' failed
make[3]: *** [j9vm-compose-buildjvm] Error 1

All repos current to days content.
openj9-ojdk9 - 17056e9
openj9 - 8a758d70
omr - 8d9dc32a

JamesKingdon · 2017-12-19T02:35:30Z

For sure it would be nice to clean the generated rules stuff up. But I sank hours into trying to understand what it was doing and eventually got frustrated with it.

Not sure why it didn't build for you, it was working yesterday: https://travis-ci.org/JamesKingdon/openj9-openjdk-jdk9/builds/317768853

Edit2: But of course my build is off a different branch to the one this PR is for, and the branches have diverged over the last few weeks, so it doesn't tell us much any more.

Edit3 (&4): Ran another local build with the compose_arm branch and confirmed it's still working. Steps used were:

BUILD_JDK=/root/buildjdk
TOOLCHAIN=/root/gcc-linaro-4.9.4-2017.01-x86_64_arm-linux-gnueabihf
bash ./configure --openjdk-target=arm-linux-gnueabihf --with-abi-profile=armv6-vfp-hflt  --with-x=${TOOLCHAIN}/arm-linux-gnueabihf/ --with-freetype=${TOOLCHAIN}/arm-linux-gnueabihf/libc/usr/ --with-freemarker-jar=/root/freemarker.jar --with-build-jdk=$BUILD_JDK --disable-warnings-as-errors

make CONF=linux-arm-normal-server-release clean

make CONF=linux-arm-normal-server-release all

Edit: the compose buildjvm step is supposed to be skipped, which is based on

ifeq ($(OPENJDK_BUILD_CPU),$(OPENJDK_TARGET_CPU))
	+($(CD) $(SRC_ROOT)/closed && $(MAKE) -f OpenJ9.gmk SPEC=$(SPEC) stage_openj9_build_jdk)
else
	@$(ECHO) Cross compile detected, skipping stage_openj9_build_jdk
endif

JamesKingdon · 2018-01-27T21:15:12Z

@jdekonin @keithc-ca I've removed the duplicate copy of generated_target_rules by introducing two new make variables that are conditionally initialized with the libraries needed when cross compiling for arm. This seems like a neater solution.

~~The Copyright check seems to be failing on a whole load of files - do I need to find them all and update the dates in this PR or is there going to be a stand-alone PR for that?~~ (Just the generated files failing copyright now, see last comment below)

~~Oh, and what do I need to do about the generated-configure.sh conflicts? I tried rebasing and git just tells me that nothing has changed.~~

pshipton · 2018-01-27T21:39:49Z

Some of those files have correct copyrights, not sure why they are showing up in the report. Do you need to rebase? Only custom-hook.m4 needs a fix, which I can do.

pshipton · 2018-01-27T21:41:10Z

I notice there are conflicts that need to be resolved before merging, so a rebase does seem to be required.

JamesKingdon · 2018-01-27T23:34:43Z

Maybe I have my upstream misconfigured - I'll check and try another rebase.

Multiple changes were required to enable cross-compilation and composition of the 32bit build into the 'default' lib subdirectory. This subdirectory name is now parameterised throughout the composition steps and can be set from custom-hook.m4 using OPENJ9_LIBS_SUBDIR. The build jdk is not currently supported (if attempted it builds for arm and then fails to run when used), so --with-build-jdk is used to specify a pre-existing build jdk. Because the build jdk isn't being built it was also necessary to prevent it being staged, otherwise the pre-existing build jdk gets overwritten with arm binaries. A new buildspec was added; linux_arm_linaro. Signed-off-by: James Kingdon <jkingdon@ca.ibm.com>

JamesKingdon · 2018-01-27T23:44:39Z

There's definitely something odd going on with my upstream config, but I think I managed to get a successful rebase this time.

JamesKingdon · 2018-01-27T23:55:24Z

Looks like I still need to update copyrights though. I guess because the changes are assumed to be made at the time the PR is eventually merged, rather than when I edited the code.

Instead of duplicating the entire generated_target_rules section, define two new variables that contain the extra libraries needed when cross-compiling. These variables can be used within the generated_target_rules to make sure the jvm/jre is correctly composed. Signed-off-by: James Kingdon <jkingdon@ca.ibm.com>

JamesKingdon · 2018-01-28T00:03:29Z

The copyright check is complaining about the generated-configure.sh files, but they don't look like something I should be editing our copyright into. Is this a bug in the copyright checker?

pshipton · 2018-01-28T16:02:32Z

@AdamBrousseau ^^^

AdamBrousseau · 2018-01-29T16:03:02Z

Opened eclipse-openj9/openj9#1043 to address copyright failures

JamesKingdon · 2018-01-31T18:29:05Z

@jdekonin @keithc-ca Given the copyright failure is a tooling issue, do the changes to OpenJ9.gmk meet your requests?

jdekonin · 2018-02-01T16:25:05Z

The copyright failure is a tooling issues, agreed. I want to discuss further with @keithc-ca before approving/merging.

Out of curiosity, why the addition to openj9.gmk for just xr, would these same changes not also apply to other platforms.

JamesKingdon · 2018-02-03T15:47:47Z

The extra libraries addition is to work around the fact that the cross compile doesn't create a build JVM. Since arm is the only platform currently being cross compiled the extra libraries are only needed for xr. The rest of the changes that allow for parameterizing the target directory name are more generally applicable, and I have another change set that uses that to build x86 non-compressed refs. It should also be useful if we want to build 32 bit versions of the other platforms.

jdekonin

There are a number of open issues that closely touch OpenJ9.gmk and how images are created.
#34, #67, #81, #82, #83 and #100.

The method currently being used (prior to these changes) while functional for a basic build, is not the correct way of hooking into openjdk as you found out when adding a cross compile platform.

I believe most of these changes will become irrelevant once the openj9 native are generated into the correct location(s); into support/module_libs// ??? But that should be performed under a different pull request.

jdekonin · 2018-02-07T15:54:34Z

closed/OpenJ9.gmk

+# These are libraries that would normally be copied as part of the build jdk rules, but that section is currently
+# not implemented for arm. Once we have a working build jdk as part of the build it will probably be possible
+# to remove these declarations.
+ifeq (xr32,$(OPENJ9_PLATFORM_CODE))


Please make this more generic for cross compilation, much like the change in Main.gmk 'ifeq ($(OPENJDK_BUILD_CPU),$(OPENJDK_TARGET_CPU))' and update the comment to not reference arm specifically.

jdekonin · 2018-02-07T15:55:03Z

closed/OpenJ9.gmk

+# to remove these declarations.
+ifeq (xr32,$(OPENJ9_PLATFORM_CODE))
+OPENJ9_JAVA_BASE_LIBRARIES_ARM := $(OPENJ9_JAVA_BASE_LIBRARIES)
+OPENJ9_SHARED_CLASSES_LIBRARIES_ARM := $(OPENJ9_SHARED_CLASSES_LIBRARIES)


Please modify naming to not be ARM specific.

jdekonin · 2018-02-07T15:56:26Z

closed/OpenJ9.gmk

-$(eval $(call generated_target_rules_build,build_jdk,$(BUILD_JDK)))
+# Temporary fix until we work out how to handle the build_jdk for cross compiles
+ifeq (xr32,$(OPENJ9_PLATFORM_CODE))
+  $(info Cross compilation detected, skipping generated_target_rules_build)


Same comment as the if statement above, please make it about cross compilation and not an ARM specific activity.

JamesKingdon · 2018-02-10T14:23:41Z

Hi Joe, thanks for the comments. Agreed, I think #67 is really the crux of the matter, once we get that worked out we should be able to clean up these changes substantially.

I'll take a crack at your revisions.

Removed the references to ARM as requested in the review. Signed-off-by: James Kingdon <jkingdon@ca.ibm.com>

JamesKingdon · 2018-02-10T15:31:26Z

@jdekonin Changes made

jdekonin

I'm good with this now. @keithc-ca ?

keithc-ca · 2018-02-12T17:37:52Z

@jdekonin I'm fine with the content, but I'd like to see the changes squashed to a single commit, which can be done in the merge action.

JamesKingdon mentioned this pull request Nov 22, 2017

Main.gmk attempts to compose the buildjvm when it shouldn't #83

Closed

jdekonin reviewed Nov 22, 2017

View reviewed changes

JamesKingdon force-pushed the compose_arm branch from 5b99b16 to 6fefe60 Compare November 23, 2017 01:57

JamesKingdon changed the title ~~WIP: Support composing 32 bit builds for arm~~ Support composing 32 bit builds for arm Nov 23, 2017

jdekonin suggested changes Nov 23, 2017

View reviewed changes

dsouzai mentioned this pull request Nov 24, 2017

Add Dockerfile for ARM cross compilation eclipse-openj9/openj9#622

Closed

JamesKingdon mentioned this pull request Nov 29, 2017

ARM build? eclipse-openj9/openj9#53

Open

JamesKingdon force-pushed the compose_arm branch from a51ddb6 to 3dffa4c Compare December 1, 2017 01:45

JamesKingdon mentioned this pull request Dec 1, 2017

ARM builds for OpenJDK eclipse-openj9/openj9#686

Merged

keithc-ca reviewed Dec 4, 2017

View reviewed changes

jdekonin suggested changes Dec 4, 2017

View reviewed changes

JamesKingdon force-pushed the compose_arm branch 2 times, most recently from 7bf4e34 to 1a4e450 Compare December 12, 2017 02:23

JamesKingdon force-pushed the compose_arm branch from 1a4e450 to 17056e9 Compare December 17, 2017 02:08

JamesKingdon force-pushed the compose_arm branch 2 times, most recently from f40d740 to 6c01b8b Compare January 27, 2018 21:09

JamesKingdon force-pushed the compose_arm branch from 6c01b8b to bb8893b Compare January 27, 2018 23:41

JamesKingdon force-pushed the compose_arm branch from bb8893b to fda2f51 Compare January 28, 2018 00:00

jdekonin suggested changes Feb 7, 2018

View reviewed changes

Cleanup cross compilation changes in OpenJ9.gmk

af6e257

Removed the references to ARM as requested in the review. Signed-off-by: James Kingdon <jkingdon@ca.ibm.com>

jdekonin approved these changes Feb 12, 2018

View reviewed changes

jdekonin merged commit 32ec58c into ibmruntimes:openj9 Feb 12, 2018

JamesKingdon mentioned this pull request Feb 12, 2018

Where/how should we build for ARM? eclipse-openj9/openj9#1159

Closed

		@@ -30,7 +30,11 @@ j9vm-build :
		+($(CD) $(SRC_ROOT)/closed && $(MAKE) -f OpenJ9.gmk SPEC=$(SPEC) VERSION_MAJOR=$(VERSION_MAJOR) build-j9)

		j9vm-compose-buildjvm : j9vm-build

Support composing 32 bit builds for arm #88

Support composing 32 bit builds for arm #88

Conversation

JamesKingdon commented Nov 22, 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

JamesKingdon Nov 22, 2017 • edited

Choose a reason for hiding this comment

JamesKingdon commented Nov 23, 2017

jdekonin left a comment

Choose a reason for hiding this comment

JamesKingdon commented Nov 23, 2017

JamesKingdon commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKingdon Dec 6, 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

JamesKingdon Dec 5, 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

JamesKingdon commented Dec 12, 2017 • edited

JamesKingdon commented Dec 18, 2017

jdekonin commented Dec 18, 2017

JamesKingdon commented Dec 19, 2017 • edited

JamesKingdon commented Jan 27, 2018 • edited

pshipton commented Jan 27, 2018

pshipton commented Jan 27, 2018

JamesKingdon commented Jan 27, 2018

JamesKingdon commented Jan 27, 2018

JamesKingdon commented Jan 27, 2018

JamesKingdon commented Jan 28, 2018

pshipton commented Jan 28, 2018

AdamBrousseau commented Jan 29, 2018

JamesKingdon commented Jan 31, 2018

jdekonin commented Feb 1, 2018

JamesKingdon commented Feb 3, 2018

jdekonin left a 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

JamesKingdon commented Feb 10, 2018

JamesKingdon commented Feb 10, 2018

jdekonin left a comment

Choose a reason for hiding this comment

keithc-ca commented Feb 12, 2018

JamesKingdon commented Nov 22, 2017 •

edited

JamesKingdon Nov 22, 2017 •

edited

JamesKingdon Dec 6, 2017 •

edited

JamesKingdon Dec 5, 2017 •

edited

JamesKingdon commented Dec 12, 2017 •

edited

JamesKingdon commented Dec 19, 2017 •

edited

JamesKingdon commented Jan 27, 2018 •

edited