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.

Sign up for GitHub

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

Jump to bottom

Initial changes for Open XL z/os support #7320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Initial changes for Open XL z/os support #7320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include_core/omrsig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ typedef __sighandler_t sighandler_t;
typedef void (*sighandler_t)(int sig);
#elif defined(J9ZOS390) || defined(AIXPPC)
typedef void (*sighandler_t)(int sig);
#if !defined(__GNUC__)
#define __THROW
#endif
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Deigue Deigue Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Deigue Deigue Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

#elif defined(OMR_OS_WINDOWS)
typedef void (__cdecl *sighandler_t)(int sig);
#define __THROW
Expand Down
2 changes: 2 additions & 0 deletions port/common/omrgetsysname.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

#include "omrcomp.h"

struct OMRPortLibrary;
Copy link
Contributor

Choose a reason for hiding this comment

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

A blank line after this one would be welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


/**
* Get the zOS SYSNAME sysparm.
*
Expand Down
2 changes: 1 addition & 1 deletion port/unix/omrshmem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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) */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) */

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@Deigue Deigue Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Deigue Deigue Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#if defined(OSX)
/*Use ._key for OSX*/
if (buf.shm_perm._key != controlinfo->common.ftok_key)
Expand Down
2 changes: 1 addition & 1 deletion port/unix/omrshsem_deprecated.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment regarding KEY here.

#if defined(OSX)
/*Use _key for OSX*/
if (buf.sem_perm._key != controlinfo->ftok_key)
Expand Down
4 changes: 2 additions & 2 deletions port/zos390/omrgetthent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
#define PGTHA_ACCESS_CURRENT 1
#define PGTHA_FLAG_PROCESS_DATA 0x80

#pragma pack(packed)
#pragma pack(1)

typedef struct pgtha {
/* Start of PGTHACONTINUE group. */
Expand Down Expand Up @@ -177,6 +177,6 @@ typedef struct ProcessData {
char padding[256];
} ProcessData;

#pragma pack(reset)
#pragma pack(pop)

#endif /* !defined(OMRGETTHENT_H_) */
16 changes: 8 additions & 8 deletions port/zos390/omrsimap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
* The RSM Internal Table (RIT) reached from the `ritp' address field in PVT. The
* field of interest here is the total amount of online storage.
*/
typedef _Packed struct J9RIT {
typedef struct __attribute__((packed)) J9RIT {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

uint8_t ritFiller1[296]; /**< 0:296 Fields irrelevant to our current purpose. */
uint64_t rittos; /**< 296:8 The total amount of online storage at IPL */
/**< Ignore rest of the fields in RIT. */
Expand All @@ -58,7 +58,7 @@ typedef _Packed struct J9RIT {
* The Page Vector Table (PVT). Reached from the virtual address found at offset 356 of
* CVT.
*/
typedef _Packed struct J9PVT {
typedef struct __attribute__((packed)) J9PVT {
uint8_t pvtFiller1[4]; /**< 0:4 PVT Control Block Identifier 'PVT' */
J9RIT *__ptr32 pvtritp; /**< 4:4 Address of the start of RSM Internal Table (RIT) */
/**< Ignore rest of the fields in PVT. */
Expand All @@ -73,7 +73,7 @@ typedef _Packed struct J9PVT {
* CCVCPUCT - No of online CPUs
* CCVUTILP - System CPU utilization
*/
typedef _Packed struct J9CCT {
typedef struct __attribute__((packed)) J9CCT {
uint8_t cctFiller1[72]; /**< 0:72 Ignore fields not relevant to current implementation */
uint32_t ccvrbswt; /**< 72:4 Recent base system wait time */
uint8_t cctFiller2[4]; /**< 76:4 Ignore fields not relevant to current implementation */
Expand All @@ -91,7 +91,7 @@ typedef _Packed struct J9CCT {
* Fields of interest:
* RMCTCCT - CPU Management Control Table
*/
typedef _Packed struct J9RMCT {
typedef struct __attribute__((packed)) J9RMCT {
uint8_t rmctname[4]; /**< 0:4 Block Identification */
J9CCT *__ptr32 rmctcct; /**< 4:4 CPU Management Control Table */
/**< Ignore rest of the RMCT */
Expand All @@ -106,7 +106,7 @@ typedef _Packed struct J9RMCT {
* ASMNVSC - Count of non-VIO allocated slots
* ASMERRS - Count of bad slots
*/
typedef _Packed struct J9ASMVT {
typedef struct __attribute__((packed)) J9ASMVT {
uint8_t asmvtFiller1[112]; /**< 0:112 Ignore fields not relevant to current implementation */
uint32_t asmslots; /**< 112:4 Count of total local slots in all open local page data sets */
uint32_t asmvsc; /**< 116:4 Count of total local slots allocated to VIO private area pages */
Expand All @@ -122,7 +122,7 @@ typedef _Packed struct J9ASMVT {
* RCEPOOL - No of frames currently available to system
* RCEAFC - Total no of frames currently on all available frame queues
*/
typedef _Packed struct J9RCE {
typedef struct __attribute__((packed)) J9RCE {
uint8_t rceid[4]; /**< 0:4 RCE control block Id */
int32_t rcepool; /**< 4:4 No of frames currently available to system */
uint8_t rceFiller1[128]; /**< 8:128 Ignore fields not relevant to current implementation */
Expand All @@ -140,7 +140,7 @@ typedef _Packed struct J9RCE {
* CVTASMVT - Pointer to auxiliary storage management vector table (ASMVT)
* CVTRCEP - Address of the RSM Control & Enumeration Area
*/
typedef _Packed struct J9CVT {
typedef struct __attribute__((packed)) J9CVT {
uint8_t cvtFiller1[356]; /**< 0:356 Ignore fields not relevant to current implementation */
J9PVT *__ptr32 cvtpvtp; /**< 356:4 Address of Page Vector Table (PVT). */
uint8_t cvtFiller2[244]; /**< 360:244 Ignore fields not relevant to current implementation */
Expand All @@ -160,7 +160,7 @@ typedef _Packed struct J9CVT {
* Fields of interest:
* FLCCVT - Address of CVT after IPL
*/
typedef _Packed struct J9PSA {
typedef struct __attribute__((packed)) J9PSA {
uint8_t psaFiller1[16]; /**< 0:16 Ignore 16 bytes before CVT pointer */
J9CVT *__ptr32 flccvt; /**< 16:4 Address of CVT after IPL */
/**< Ignore rest of the PSA */
Expand Down
10 changes: 2 additions & 8 deletions thread/common/thrprof.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,14 +464,8 @@ uintptr_t
omrthread_get_handle(omrthread_t thread)
{
#if defined(J9ZOS390)
/* Hack!! - If we do the simple cast (in the #else case) we get the following
compiler error in z/OS:
"ERROR CBC3117 ./thrprof.c:79 Operand must be a scalar type."
In order to work around the compiler error, we have to reach inside
the structure do the dirty work. The handle may not even be correct! */
uintptr_t *tempHandle;
tempHandle = (uintptr_t *)&(thread->handle.__[0]);
return *tempHandle;
OSTHREAD tempHandle = thread->handle;
return (uintptr_t)*(unsigned long long *)&tempHandle;
#else
return (uintptr_t)thread->handle;
#endif
Expand Down
4 changes: 2 additions & 2 deletions thread/zos390/omrgetthent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
#pragma linkage(BPX1GTH,OS)
#endif /* _LP64 */

#pragma pack(packed)
#pragma pack(1)

/*
PGTHA DSECT , I N P U T - - - - - - - - - - -
Expand Down Expand Up @@ -553,6 +553,6 @@ struct j9pg_thread_data {
char padding[256];
};

#pragma pack(reset)
#pragma pack(pop)

#endif /* OMRGETTHENT_H */
4 changes: 2 additions & 2 deletions util/omrutil/gettimebase.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ getTimebase(void)
#endif /* OMR_ENV_DATA64 */
#endif /* GCC 4.8 */

#elif defined(LINUX) && defined(S390)
asm("stck %0" : "=m" (tsc));
#elif (defined(LINUX) && defined(S390)) || (defined(__open_xl__) && defined(J9ZOS390))
asm(" stck %0" : "=m" (tsc));
#elif defined(J9ZOS390)
__stck((unsigned long long*)&tsc);
#elif defined(LINUX) && (defined(OMR_ARCH_ARM) || defined(OMR_ARCH_RISCV) || defined(RISCV64))
Expand Down