Add Open XL toolchain and config changes for z/OS #7319

Deigue · 2024-04-25T15:31:17Z

Adds the initial set of changes to support compilation with Open XL on z/OS. This will define the alternative flags and configuration that needs to be used by the wyvern compiler while running on z/OS.

(This is one part of the multiple changes added for supporting Open XL compilation on OMR)

github-actions

Thank you for supporting the project, and congratulations on your first contribution! A project committer will shortly review your contribution. In the mean time, if you haven't had a chance please skim over the contribution guidelines which all pull requests must adhere to. If the ECA pull request check fails, have a look at the instructions for signing the ECA in the legal considerations section.

If you run into any problems our community will be happy to assist you in any way we can. There are a number of recommended ways to interact with the community. We encourage you to ask questions, or drop by to say hello.

babsingh · 2024-06-03T17:58:15Z

jenkins build all

Deigue · 2024-06-10T18:40:08Z

Changes here confirmed all passing j11 / j17 / j21 builds.

babsingh · 2024-06-10T20:17:49Z

jenkins build all

babsingh

High level feedback:

Squash the two commits into a single commit.
Revise and improve comments.
Remove dead code.

babsingh · 2024-06-13T18:23:29Z

cmake/modules/platform/toolcfg/openxl.cmake

+			-m64
+		)
+	else()
+		# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for the assembler and it has trouble with some assembly files if it is specified


Spread comments longer than 100 chars across multiple lines.

End comments with a full stop / period.

Suggested change

# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for the assembler and it has trouble with some assembly files if it is specified

# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for

# the assembler and it has trouble with some assembly files if it is specified.

babsingh · 2024-06-13T18:25:09Z

cmake/modules/platform/toolcfg/openxl.cmake

+		#"\"-Wc,xplink\""               # link with xplink calling convention
+		#"\"-Wc,rostring\""             # place string literals in read only storage
+		#"\"-Wc,FLOAT(IEEE,FOLD,AFP)\"" # Use IEEE (instead of IBM Hex Format) style floats
+		#"\"-Wc,enum(4)\""              # Specifies how many bytes of storage enums occupy
+		#"\"-Wa,goff\""                 # Assemble into GOFF object files
+		#"\"-Wc,NOANSIALIAS\""          # Do not generate ALIAS binder control statements


Remove dead code instead of commenting it.

babsingh · 2024-06-13T18:26:49Z

cmake/modules/platform/toolcfg/openxl.cmake

+	string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mgoff\"")
+	string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mSYSPARM(BIT64)\"")
+
+    # commenting out options progressively as they are not needed with Open XL, can remove in cleanup later.


Indentation is off, but this comment should go away since dead code will be removed.

babsingh · 2024-06-13T18:31:19Z

cmake/modules/platform/toolcfg/openxl.cmake

+	endfunction()
+else()
+	function(_omr_toolchain_process_exports TARGET_NAME)
+		# we only need to do something if we are dealing with a shared library


First person should be avoided since it is informal and takes attention away from the key point.

babsingh · 2024-06-13T21:32:47Z

@keithc-ca Can you please review this PR?

keithc-ca · 2024-06-17T13:45:44Z

cmake/modules/OmrDetectSystemInformation.cmake

+				set(CMAKE_C_COMPILER_IS_OPENXL ON CACHE BOOL "ibm-clang is the C compiler")
+				set(_OMR_TOOLCONFIG "openxl")
+			else()
+				message(STATUS "NO MATCH")


Perhaps this should be fatal (like line 219)?
Actually, I think line 219 should use CMAKE_C_COMPILER_ID instead of CMAKE_CXX_COMPILER_ID.

Hi Keith, I was verifying all my cleanup changes with different builds. After making the above-mentioned changes, while Open XL passes with no problems, XLC build gives a fatal error, because I guess it is entering this z/OS section, but not matching with the other xlclang compilers and in place of the "NO MATCH" its just throwing the error.

Perhaps it makes more sense to update this to
message(STATUS "NO XLClang match. Using xlc toolchain.")

with the above, both XLC / OpenXL are now compiling successfully.

Does this sound good?

keithc-ca · 2024-06-17T13:47:51Z

cmake/modules/platform/toolcfg/openxl.cmake

+
+# Copyright (c) 2017, 2024 IBM Corp. and others
+#
+# This program and the accompanying materials are made available under
+# the terms of the Eclipse Public License 2.0 which accompanies this
+# distribution and is available at http://eclipse.org/legal/epl-2.0
+# or the Apache License, Version 2.0 which accompanies this distribution
+# and is available at https://www.apache.org/licenses/LICENSE-2.0.
+#
+# This Source Code may also be made available under the following Secondary
+# Licenses when the conditions for such availability set forth in the
+# Eclipse Public License, v. 2.0 are satisfied: GNU General Public License,
+# version 2 with the GNU Classpath Exception [1] and GNU General Public
+# License, version 2 with the OpenJDK Assembly Exception [2].
+#
+# [1] https://www.gnu.org/software/classpath/license.html
+# [2] http://openjdk.java.net/legal/assembly-exception.html
+#
+# SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
+###############################################################################


This is not the right copyright notice. The year should be 2024 (since this file is new); we don't need blank line 2; and the URLs and SPDX are wrong. This should be copied from an existing file.

keithc-ca · 2024-06-17T13:49:11Z

cmake/modules/platform/toolcfg/openxl.cmake

+	# Testarossa build variables. Longer term the distinction between TR and the rest
+	# of the OMR code should be heavily reduced. In the mean time, we keep
+	# the distinction


Punctuation.

keithc-ca · 2024-06-17T13:53:09Z

cmake/modules/platform/toolcfg/openxl.cmake

+
+

Just one blank line, please.

keithc-ca · 2024-06-17T14:07:33Z

cmake/modules/platform/toolcfg/openxl.cmake

+	set(OMR_C_WARNINGS_AS_ERROR_FLAG -qhalt=w)
+	set(OMR_CXX_WARNINGS_AS_ERROR_FLAG -qhalt=w)
+
+	# There is no enhanced warning for XLC right now


This file should not be used for xlc: why is that compiler even mentioned here?

keithc-ca · 2024-06-17T14:09:59Z

ddr/Makefile

@@ -26,7 +26,7 @@ include $(top_srcdir)/omrmakefiles/configure.mk

 OMR_TOOLCHAIN ?= gcc

-ifneq (,$(filter gcc xlc,$(OMR_TOOLCHAIN)))
+ifneq (,$(filter gcc xlc openxl,$(OMR_TOOLCHAIN)))


Do we care about non-cmake builds with OpenXL? If not, the remaining changes should be removed.

keithc-ca · 2024-06-18T19:09:38Z

The commit message and the discussion here should refer to "z/OS", not "zos".

Deigue · 2024-06-19T17:50:13Z

Thanks for the feedback/cleanup suggestions. I've addressed the above comments and verified builds are passing.

keithc-ca · 2024-06-19T19:53:47Z

cmake/modules/platform/toolcfg/openxl.cmake

+		omr_remove_flags(CMAKE_ASM_FLAGS -qhalt=e)
+		omr_remove_flags(CMAKE_CXX_FLAGS -qhalt=s)
+		omr_remove_flags(CMAKE_C_FLAGS   -qhalt=e)


Suggest languages should be addressed in a consistent order; see lines 34-35 and elsewhere, where C is handled before C++.

reordered to put C_FLAGS first as seen in other places.

keithc-ca · 2024-06-19T19:54:56Z

cmake/modules/platform/toolcfg/openxl.cmake

+		# -march should be there for 32 and 64 C/CXX flags but the C compiler is used for
+		# the assembler and it has trouble with some assembly files if it is specified.


Why does this comment reference 64-bit compiles here in the 32-bit branch?

removed along with the rest of "ppc" section, as POWER support will be contributed by another developer as mentioned below.

keithc-ca · 2024-06-19T19:56:07Z

cmake/modules/platform/toolcfg/openxl.cmake

+	endmacro(omr_toolconfig_global_setup)
+endif()
+
+if(OMR_HOST_ARCH STREQUAL "ppc")


Has any of this been tested on POWER? I see several -q options which appear to be in the xlc style.

Another dev (Jackie) is working on support for POWER, as such I will be removing this untested section from here. Believe she has commits that are adding support separately so this section should be not here.

keithc-ca · 2024-06-19T19:56:26Z

cmake/modules/platform/toolcfg/openxl.cmake

+		set(CMAKE_C_ARCHIVE_CREATE "<CMAKE_AR> -X64 cr <TARGET> <LINK_FLAGS> <OBJECTS>")
+		set(CMAKE_C_ARCHIVE_FINISH "<CMAKE_RANLIB> -X64 <TARGET>")
+	endif()
+


Please omit this blank line.

Removed extra line.

keithc-ca · 2024-06-19T19:57:13Z

cmake/modules/platform/toolcfg/openxl.cmake

+
+	set(CMAKE_ASM_FLAGS "-fno-integrated-as")
+	string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mgoff\"")
+	string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mSYSPARM(BIT64)\"")


Shouldn't BIT64 be conditional on OMR_ENV_DATA64?

Added the missing conditional

keithc-ca · 2024-06-19T19:59:01Z

configure

@@ -1727,7 +1727,8 @@ Some influential environment variables:
              Specifies whether the package will run in 32- or 64-bit mode.
              One of: 31,32,64
  OMR_TOOLCHAIN
-              The toolchain used to build the package. One of: gcc,xlc,msvc
+              The toolchain used to build the package. One of:
+              gcc,xlc,msvc,openxl


This file should not be modified: it's not used in cmake builds.

keithc-ca · 2024-06-19T19:59:07Z

configure.ac

@@ -47,7 +47,7 @@ AC_ARG_VAR([exe_output_dir], [The output directory for executables. (Default: $t
 AC_ARG_VAR([OMR_HOST_OS], [The operating system where the package will run. One of: aix,linux,osx,win,zos])
 AC_ARG_VAR([OMR_HOST_ARCH], [The architecture of the CPU where the package will run. One of: aarch64,arm,ppc,riscv,s390,x86])
 AC_ARG_VAR([OMR_TARGET_DATASIZE], [Specifies whether the package will run in 32- or 64-bit mode. One of: 31,32,64])
-AC_ARG_VAR([OMR_TOOLCHAIN], [The toolchain used to build the package. One of: gcc,xlc,msvc])
+AC_ARG_VAR([OMR_TOOLCHAIN], [The toolchain used to build the package. One of: gcc,xlc,msvc,openxl])


This file should not be modified: it's not used in cmake builds.

This change will require changes from eclipse#7319 Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue · 2024-07-18T18:57:26Z

Should have addressed all the comments above, also changed to use -std=gnu++14 as recommended by compiler team earlier. Added change here as it belongs part of the toolchain files commits.

Adds the initial set of changes to support compilation with Open XL on z/OS. This will define the alternative flags and configuration that needs to be used by the wyvern compiler while running on z/OS. Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue · 2024-07-23T16:31:28Z

Added conditional elseif(CMAKE_C_COMPILER_ID MATCHES "IBMClang$") as on Jenkins builds the compiler ID is detected as IBMClang 2.1.0.0 , but locally it may resolve as zOS

Deigue · 2024-09-18T18:14:33Z

@keithc-ca Addressed above comments and ready for review. Awaiting approval on this from your side, then will reach out to Babneet for reviewing again and merging.

keithc-ca · 2024-09-18T18:42:22Z

cmake/modules/platform/toolcfg/openxl.cmake

+if(CMAKE_C_COMPILER_IS_XLCLANG)
+	macro(omr_toolconfig_global_setup)
+		# For XLClang, remove any usages of -qhalt=e or -qhalt=s provided by default
+		# in the CMAKE CXX/C/ASM FLAGS, since xlclang/xlclang++ are not compatible
+		# with the e or s options.
+		omr_remove_flags(CMAKE_ASM_FLAGS -qhalt=e)
+		omr_remove_flags(CMAKE_C_FLAGS   -qhalt=e)
+		omr_remove_flags(CMAKE_CXX_FLAGS -qhalt=s)
+	endmacro(omr_toolconfig_global_setup)
+endif()


This file should not be used when CMAKE_C_COMPILER_IS_XLCLANG (in which case OMR_TOOLCONFIG is xlc). All the resulting dead code should be removed from this file.

Even after such changes this would still use -q options, which I believe are for xlc, not OpenXL.

keithc-ca · 2024-09-18T18:46:14Z

cmake/modules/OmrDetectSystemInformation.cmake

@@ -198,19 +198,28 @@ macro(omr_detect_system_information)
 				# just use GNU config
 				set(_OMR_TOOLCONFIG "gnu")
 			endif()
+		elseif(CMAKE_C_COMPILER_ID MATCHES "IBMClang$")
+			set(_OMR_TOOLCONFIG "openxl")
+			set(CMAKE_C_COMPILER_IS_OPENXL ON CACHE BOOL "ibm-clang is the C compiler")
 		elseif(CMAKE_C_COMPILER_ID MATCHES "^XL(Clang)?$" OR CMAKE_C_COMPILER_ID STREQUAL "zOS")
 			# In CMake 3.14 and prior, XLClang uses CMAKE_C_COMPILER_ID "XL"
 			# In CMake 3.15 and beyond, XLClang uses CMAKE_C_COMPILER_ID "XLClang"
 			set(_OMR_TOOLCONFIG "xlc")


This belongs within the if block for xlc.

Deigue requested review from mstoodle and 0xdaryl as code owners April 25, 2024 15:31

github-actions bot added the first contribution label Apr 25, 2024

github-actions bot reviewed Apr 25, 2024

View reviewed changes

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from 3f0b73e to c39496d Compare April 26, 2024 15:24

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from 3ccf780 to e34c258 Compare May 6, 2024 19:12

Deigue force-pushed the openxl-toolchain branch from e34c258 to e63f484 Compare May 21, 2024 20:11

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from b9fb8d3 to f7c2ff6 Compare June 3, 2024 18:45

babsingh mentioned this pull request Jun 7, 2024

Explicit header definitions for Open XL warnings #7321

Open

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from ff73ec3 to c1479dd Compare June 10, 2024 16:59

babsingh reviewed Jun 13, 2024

View reviewed changes

keithc-ca suggested changes Jun 17, 2024

View reviewed changes

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from 5ce9dec to a47dfb7 Compare June 18, 2024 14:59

Deigue changed the title ~~Add Open XL toolchain and config changes for zos~~ Add Open XL toolchain and config changes for z/OS Jun 19, 2024

Deigue force-pushed the openxl-toolchain branch 2 times, most recently from 57cc58e to 69a94df Compare June 19, 2024 17:48

Deigue force-pushed the openxl-toolchain branch from 69a94df to 979c63b Compare June 19, 2024 17:51

keithc-ca suggested changes Jun 19, 2024

View reviewed changes

Deigue force-pushed the openxl-toolchain branch from 979c63b to c4851ce Compare June 27, 2024 15:28

Deigue force-pushed the openxl-toolchain branch from c4851ce to fc824e2 Compare June 27, 2024 15:33

Deigue added a commit to Deigue/omr that referenced this pull request Jul 18, 2024

Remove unnecessary DDR flags for Open XL

0c06b71

This change will require changes from eclipse#7319 Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue mentioned this pull request Jul 18, 2024

Remove unnecessary DDR flags for Open XL #7415

Open

Deigue force-pushed the openxl-toolchain branch 3 times, most recently from 8e4853d to bcdb1e7 Compare July 18, 2024 18:53

Deigue force-pushed the openxl-toolchain branch from bcdb1e7 to 581768a Compare July 23, 2024 16:29

keithc-ca suggested changes Sep 18, 2024

View reviewed changes

	# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for the assembler and it has trouble with some assembly files if it is specified
	# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for
	# the assembler and it has trouble with some assembly files if it is specified.

		# -march should be there for 32 and 64 C/CXX flags but the C compiler is used for
		# the assembler and it has trouble with some assembly files if it is specified.

Add Open XL toolchain and config changes for z/OS #7319

Are you sure you want to change the base?

Add Open XL toolchain and config changes for z/OS #7319

Conversation

Deigue commented Apr 25, 2024 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

babsingh commented Jun 3, 2024

Deigue commented Jun 10, 2024 • edited Loading

babsingh commented Jun 10, 2024

babsingh 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

Choose a reason for hiding this comment

babsingh commented Jun 13, 2024

Choose a reason for hiding this comment

Deigue Jun 19, 2024 • edited Loading

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 Jun 18, 2024

Deigue commented Jun 19, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deigue commented Jul 18, 2024

Deigue commented Jul 23, 2024 • edited Loading

Deigue commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deigue commented Apr 25, 2024 •

edited

Loading

Deigue commented Jun 10, 2024 •

edited

Loading

Deigue Jun 19, 2024 •

edited

Loading

Deigue commented Jul 23, 2024 •

edited

Loading