Initial changes for Open XL z/os support #7320

Deigue · 2024-04-25T15:33:26Z

Includes all the initial work done a while ago made in order to support compiling with Open XL in context of z/OS.

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

keithc-ca · 2024-04-25T16:02:15Z

thread/common/thrprof.c

 	uintptr_t *tempHandle;
-	tempHandle = (uintptr_t *)&(thread->handle.__[0]);
+	#if defined(__GNUC__)
+		tempHandle = (uintptr_t *)&(thread->handle.__);
+	#else
+		tempHandle = (uintptr_t *)&(thread->handle.__[0]);
+	#endif


This should use the same pattern as in #7323.

So I am seeing https://github.com/Deigue/omr/blob/4401fae20ac9db1255fafb68333e322297d43620/include_core/thrtypes.h#L57 , handle is of type OSTHREAD

OSTHREAD tempHandle = thread->handle; return (uint_ptr)* <cast> tempHandle

But what would the cast look like? Where can I find OSTHREAD to see what exactly __ is pulling out and how to cast it properly? "handle" also seemed like too broad a search/grep for /usr/include

Something like this is what I meant:

OSTHREAD tempHandle = thread->handle; return (uintptr_t)*(unsigned long long *)&tempHandle;

For z/OS, OSTHREAD is defined in include_core/unix/thrdsup.h:

typedef pthread_t OSTHREAD;

Thanks, confirmed it works and everything compiles, and changed accordingly.

babsingh · 2024-06-10T20:16:47Z

jenkins build all

Deigue · 2024-06-11T15:56:57Z

java 21 jenkins passed
However, will be making more additional changes, so will need reverify here after have a more final draft.

Removed unnecessary changes and confirmed compiling with Open XL upto latest wip error.
Cleanup of pragma packed changes. pragma packed and pragma reset is unsupported by Open XL, and pragma pack(push,1) and pragma pop(pop) is functionally identical and works with both XLC and Open XL on z/OS.
Pending to RE-verify Jenkins XLC build.

Deigue · 2024-06-19T15:06:37Z

j21 jenkins verify Verified new changes compiling with XLC as well, after checking locally working with Open XL. Removing draft and changing to ready for review ...

keithc-ca · 2024-06-19T18:00:54Z

include_core/omrsig.h

+#if !defined(__GNUC__)
 #define __THROW
+#endif


This is within an extern "C" block, so none of the uses of __THROW below seem appropriate.
The macro definition, and its uses here and in omrsig.cpp should just be removed.

It is extern "C" only if __cplusplus is defined right?

And if that is the case, the it is safe to remove the define instances below entirely?
L35: #define __THROW
Snippet L35:37

#if defined(OSX) #define __THROW #endif /* defined(OSX) */

And what does that mean for these below where __THROW is being used, does the "__THROW" part need to be removed specifically from every line? https://github.com/eclipse/omr/blob/c256823419b4579cf64f94072337fa108f1761f9/include_core/omrsig.h#L104-L115

The definitions of __THROW should be removed as well as all uses.

thanks Ill remove these, and from omrsig.cpp as well, will need somebody to help me run/issue a jenkins build for macos/windows as well, to verify that it doesnt impact and is all good.

I get these errors when removing the specifications:

/omr/omrsigcompat/omrsig.cpp:241:1: error: 'signal' is missing exception specification 'throw()' 241 | signal(int signum, sighandler_t handler) | ^ | throw()

This being shown for all 5 functions defined in omrsig.cpp with __THROW (probably more errors if not just zos, as some might be not defined on zos)

These are declared within an extern "C" { } block; C functions don't (can't) have throw() specifications.
If you preprocess omrsig.cpp, do you see an ealier declaration of these functions?

The definition of the signal handler in omrsig.cpp should be qualified with extern "C". One might expect that it would be sufficient that the declaration appears within the extern "C" block beginning on line 45, but apparently not: That is something we should bring to the attention of the compiler team.

The suppression of warnings on Windows suggest to me that the declaration of signal is wrong there as well.

keithc-ca · 2024-06-19T18:01:30Z

port/common/omrgetsysname.h

@@ -25,6 +25,7 @@

 #include "omrcomp.h"

+struct OMRPortLibrary;


A blank line after this one would be welcome.

keithc-ca · 2024-06-19T18:09:22Z

omrsigcompat/makefile

@@ -24,7 +24,7 @@ top_srcdir := ..
 include $(top_srcdir)/omrmakefiles/configure.mk

 MODULE_NAME := omrsig
-ARTIFACT_TYPE := c_shared
+ARTIFACT_TYPE := cxx_shared


This has nothing to do with OpenXL; it's just an error that could be corrected separately.

removed from this PR.

keithc-ca · 2024-06-19T18:14:26Z

port/unix/omrshmem.c

@@ -1663,7 +1663,7 @@ omrshmem_openSharedMemory (OMRPortLibrary *portLibrary, intptr_t fd, const char
 				goto failDontUnlink;
 			}
 		} else {
-#if defined(__GNUC__) || defined(AIXPPC)
+#if (defined(__GNUC__) || defined(AIXPPC)) && !defined(J9ZOS390)


Suggest reordering the #if branches so it's clear which one is intended for z/OS, or #define KEY appropriately for each platform/compiler and then use KEY instead of key, _key or __key.

can you detail that a bit, looking at the logic again, since I have excluded via !defined(J9ZOS390) , it should mean that z/OS does not use any of the child if branches underlying within the parent if clause right?

I see now that none of the branches would apply for z/OS: is that the right behavior?
If so, I suggest we reorder the code to put the z/OS path first, and this in the #else block:

#if defined(OMRZOS390) // existing lines 1686-1709 #elif defined(__GNUC__) || defined(AIXPPC) // existing lines 1667-1683 #endif /* defined(OMRZOS390) */

yup, and that does look cleaner.
But what is the difference (if any) between OMRZOS390 and J9ZOS390 ... because if it seems there is diff, then I will still need it as follows:

#if defined(OMRZOS390) // existing lines 1686-1709 #elif (defined(__GNUC__) || defined(AIXPPC)) && !defined(J9ZOS390) // existing lines 1667-1683 #endif /* defined(OMRZOS390) */

I think OMRZOS390 is the preferred flag to test in new code, J9ZOS390 is a name that originated in OpenJ9. They're both still used and I think they're equivalent, but OMR is supposed to be a base for OpenJ9, not dependent on anything from OpenJ9 (including naming conventions).

Seems the build is failing after reorganizing this and omrshsem_deprecated.c

cd .../omr/build/port && /xlc210/usr/lpp/IBM/cnw/v2r1/openxl/bin/ibm-clang64 -D__STDC_LIMIT_MACROS -D_ALL_SOURCE -D_OPEN_THREADS=3 -D_XOPEN_SOURCE=600 -DJ9ZOS390 -DJ9ZOS39064 -DLONGLONG -DOMR_EBCDIC -DOMRPORT_LIBRARY_DEFINE -DSUPPORTS_THREAD_LOCAL -DZOS -I...(all includes here) -fstrict-aliasing -mzos-target=ZOSV2R4 -m64 -march=arch10 -fvisibility=default -o CMakeFiles/omrport_obj.dir/unix/omrshmem.c.o -c /.../omr/port/unix/omrshmem.c

The full command follows the above format, and J9Z0S390 is defined, but looks like OMRZOS390 is not ... so I might need to switch it to J9ZOS390. Doing this seems to highlight a bigger issue, that OMRZOS390 clauses aren't being used for the file, so all the conditionals that are expected to be entered arent, and thus causes compilation problems.

Looks like from what I can see it was not set for xlc all along either, unless Im missing somewhere in code where its defined.

I don't see OMRZOS390 defined anywhere (despite tests in this file and in omrsharedhelper.h).
Unless someone can point to where OMRZOS390 is defined, this should use J9ZOS390.

Agreed, thats what I was mentioning in my prior comment. As such, in the current state of the PR I am still utilizing J9ZOS390 in this file.
Unless you are saying replace all the mentions in this file, that I would have to change and I suspect I will be running into other problems.

keithc-ca · 2024-06-19T18:24:40Z

port/unix/omrshsem_deprecated.c

@@ -1060,7 +1060,7 @@ omrshsem_openSemaphore(struct OMRPortLibrary *portLibrary, intptr_t fd, char *ba
 				goto failDontUnlink;
 			}
 		} else {
-#if defined(__GNUC__) || defined(AIXPPC) || defined(OMRZTPF)
+#if (defined(__GNUC__) || defined(AIXPPC) || defined(J9ZTPF)) && !defined(J9ZOS390)


Same comment regarding KEY here.

keithc-ca · 2024-06-19T18:25:12Z

port/zos390/omrgetthent.h

@@ -28,7 +28,7 @@
 #define PGTHA_ACCESS_CURRENT 1
 #define PGTHA_FLAG_PROCESS_DATA 0x80

-#pragma pack(packed)
+#pragma pack(push, 1)


Does this mean the same for both xlc and OpenXL?

yes they should be functionally the same.
https://www.ibm.com/docs/en/zos/2.4.0?topic=descriptions-pragma-pack
also have confirmed internally with Sean from the open-xl/cpp team

Have changed this to #pragma pack(1) after having confirmed the packed struct sizes between XLC and Open XL and confirming the functionality is identical with Sean. He suggested to use this notation instead.
Also verified both XLC and Open XL compilers all omr tests are passing with this.

keithc-ca · 2024-06-19T18:26:09Z

port/zos390/omrintrospect.h

+#if defined(OMR_ENV_DATA64)
+#else


Just say #if !defined(OMR_ENV_DATA64) and add the missing #endif comment.

looks like I have your feedback in the other PR: #7321
Might just remove this file from this PR, as that one seems to handle more updated/latest code for omrintrospect.h

keithc-ca · 2024-06-19T18:26:58Z

port/zos390/omrsimap.h

-typedef _Packed struct J9RIT {
+typedef struct __attribute__((packed)) J9RIT {


Again, do xlc and OpenXL attach the same meaning to this?

They do have the same meaning as per discussing internally with Sean from the cpp team. I did inquire if we have any published/public documentation for this, but I dont think they have a dedicated page regarding this particular suggestion/tweak.
However digging a bit and searching for it, there is a small mention of it on this page: https://www.ibm.com/docs/en/open-xl-c-cpp-zos/2.1?topic=guide-binary-compatibility

To quote part of our discussions:

The problem with _Packed is that it is C only and the usual change to attribute((packed)) for C++ doesn't have the same semantics.

typedef struct S _Packed { char c; int i; } S; S s1; struct S s2;

In this example s1 and s2 have different sizes and types. The _Packed keyword applies to the declarator (eg. typedef name S) on the struct definition. The _Packed keyword creates a hidden duplicate version of the struct.
Avoid the keyword even in XL.

Includes all the initial work done a while ago made in order to support compiling with Open XL in context of z/OS. Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue · 2024-09-18T18:13:24Z

@keithc-ca This one is also ready. I have added comments for prior questions above. Wanted to make sure this is approved from your perspective before Babneet can kick off the jenkins builds and start the process for merging the changes.

keithc-ca · 2024-09-18T18:53:58Z

include_core/omrsig.h

+#if !defined(__GNUC__)
 #define __THROW
+#endif


The definition of the signal handler in omrsig.cpp should be qualified with extern "C". One might expect that it would be sufficient that the declaration appears within the extern "C" block beginning on line 45, but apparently not: That is something we should bring to the attention of the compiler team.

The suppression of warnings on Windows suggest to me that the declaration of signal is wrong there as well.

keithc-ca · 2024-09-18T19:19:31Z

port/unix/omrshmem.c

@@ -1663,7 +1663,7 @@ omrshmem_openSharedMemory (OMRPortLibrary *portLibrary, intptr_t fd, const char
 				goto failDontUnlink;
 			}
 		} else {
-#if defined(__GNUC__) || defined(AIXPPC)
+#if (defined(__GNUC__) || defined(AIXPPC)) && !defined(J9ZOS390)


I don't see OMRZOS390 defined anywhere (despite tests in this file and in omrsharedhelper.h).
Unless someone can point to where OMRZOS390 is defined, this should use J9ZOS390.

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

github-actions bot added arch:z comp:core comp:port comp:thread comp:utilities labels Apr 25, 2024

keithc-ca reviewed Apr 25, 2024

View reviewed changes

Deigue force-pushed the openxl-initial branch from f778bc3 to dee7317 Compare May 13, 2024 16:16

Deigue force-pushed the openxl-initial branch from dee7317 to f2d58f0 Compare May 21, 2024 20:09

Deigue marked this pull request as draft June 10, 2024 19:29

Deigue force-pushed the openxl-initial branch from f2d58f0 to 059476d Compare June 10, 2024 19:32

Deigue force-pushed the openxl-initial branch 2 times, most recently from 566b966 to f06239c Compare June 17, 2024 20:56

Deigue marked this pull request as ready for review June 19, 2024 15:06

keithc-ca suggested changes Jun 19, 2024

View reviewed changes

Deigue force-pushed the openxl-initial branch from f06239c to c256823 Compare July 10, 2024 20:20

Deigue force-pushed the openxl-initial branch 2 times, most recently from 50f0e24 to d8bb359 Compare August 7, 2024 15:15

Deigue force-pushed the openxl-initial branch from d8bb359 to 5136e6b Compare September 16, 2024 16:52

Initial changes for Open XL z/os support

c0d5ea3

Includes all the initial work done a while ago made in order to support compiling with Open XL in context of z/OS. Signed-off-by: Gaurav Chaudhari <[email protected]>

Deigue force-pushed the openxl-initial branch from 5136e6b to c0d5ea3 Compare September 16, 2024 16:54

keithc-ca suggested changes Sep 18, 2024

View reviewed changes

		typedef _Packed struct J9RIT {
		typedef struct __attribute__((packed)) J9RIT {

Initial changes for Open XL z/os support #7320

Are you sure you want to change the base?

Initial changes for Open XL z/os support #7320

Conversation

Deigue commented Apr 25, 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

babsingh commented Jun 10, 2024

Deigue commented Jun 11, 2024 • edited Loading

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

Deigue Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Deigue Jul 29, 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

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 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deigue Sep 23, 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

Deigue Jul 10, 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

Deigue commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deigue commented Jun 11, 2024 •

edited

Loading

Deigue Jul 29, 2024 •

edited

Loading

Deigue Jul 29, 2024 •

edited

Loading

Deigue Jul 29, 2024 •

edited

Loading

Deigue Sep 23, 2024 •

edited

Loading

Deigue Jul 10, 2024 •

edited

Loading