Add z/OS support with clang compiler#5423
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial z/OS (s390x) platform support to Botan, primarily targeting the z/OS clang toolchain, by introducing a new OS configuration and z/OS-specific build/compiler adjustments.
Changes:
- Added a new z/OS OS configuration (
src/build-data/os/zos.txt) with target feature declarations and aliases. - Extended clang/gcc build-data to recognize
zosfor shared library linking and (clang) z/OS-specific ABI/compiler flags. - Updated low-level platform code paths for z/OS (S390X cycle counter) and adjusted FFI behavior for z/OS builds.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils/os_utils/os_utils.cpp | Adds a z/OS (__MVS__) S390X cycle counter implementation using __stckf. |
| src/lib/ffi/ffi.cpp | Adds a z/OS workaround related to thread_local usage in the FFI exception message storage. |
| src/build-data/os/zos.txt | Introduces a new z/OS OS definition with target features and aliases. |
| src/build-data/cc/gcc.txt | Adds zos shared library link command and adjusts pthread flag application for zos. |
| src/build-data/cc/clang.txt | Adds zos shared library link command and introduces z/OS-specific ABI/compiler flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| #if defined(__MVS__) | ||
| // z/OS has a bug with thread_local | ||
| #define thread_local | ||
| #endif |
There was a problem hiding this comment.
On z/OS this replaces the C++ thread_local keyword with an empty macro, turning g_last_exception_what into a process-global variable. That makes the FFI error message storage non-thread-safe (data races and cross-thread contamination) and also relies on redefining a language keyword.
Instead of redefining thread_local, please keep thread_local for platforms that support it and provide a z/OS-specific fallback that is still thread-safe (e.g., a pthread_key_t/TLS-based per-thread string, or another per-thread storage mechanism).
There was a problem hiding this comment.
Instead of branching off for the z/OS compiler specifically, you could instead remove the thread_local feature flag in zos.txt as suggested elsewhere and use #if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL) here.
Like so:
#if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL)
// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
thread_local std::string g_last_exception_what;
#else
// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
std::string g_last_exception_what;
#endif... I guess we should introduce a mutex instead, to at least guard multithreaded access to this global, but that is out-of-scope for this change and we can do that independently.
| unsigned long long value; | ||
| __stckf(&value); | ||
| return value; |
There was a problem hiding this comment.
The z/OS-specific branch returns early, which makes the trailing return rtc; unreachable for this compilation path and can also trigger an -Wunused-variable warning for rtc (since it's never used when __MVS__ is set). To keep warning-free builds, assign the __stckf result into rtc and fall through to the common return rtc; at the end of the function.
| unsigned long long value; | |
| __stckf(&value); | |
| return value; | |
| __stckf(&rtc); |
| #ifdef __MVS__ | ||
| unsigned long long value; | ||
| __stckf(&value); | ||
| return value; | ||
| #else | ||
| asm volatile("stck 0(%0)" : : "a"(&rtc) : "memory", "cc"); | ||
| #endif |
There was a problem hiding this comment.
Preprocessor directives inside this function are typically indented to match the surrounding scope (e.g. #if ...). The #ifdef __MVS__ / #else / #endif are currently unindented, which is inconsistent with the existing style in this file and makes the nested conditional harder to read. Please indent these directives consistently with the surrounding #elif blocks.
| #ifdef __MVS__ | |
| unsigned long long value; | |
| __stckf(&value); | |
| return value; | |
| #else | |
| asm volatile("stck 0(%0)" : : "a"(&rtc) : "memory", "cc"); | |
| #endif | |
| #ifdef __MVS__ | |
| unsigned long long value; | |
| __stckf(&value); | |
| return value; | |
| #else | |
| asm volatile("stck 0(%0)" : : "a"(&rtc) : "memory", "cc"); | |
| #endif |
There was a problem hiding this comment.
Within the source itself the general usage is to take environment cues from macros set by us, eg here probably __MVS__ should be BOTAN_TARGET_OS_IS_ZOS
| sockets | ||
| system_clock | ||
| threads | ||
| thread_local |
There was a problem hiding this comment.
This OS config advertises the thread_local target feature, but the FFI layer currently works around z/OS by effectively disabling thread_local (see src/lib/ffi/ffi.cpp). This should be made consistent: either remove thread_local from the z/OS feature list until the toolchain reliably supports it, or implement a proper thread-local fallback so thread_local can remain enabled.
| thread_local |
There was a problem hiding this comment.
Interestingly, the thread_local feature flag that the robot refers to doesn't currently reflect in any compile-time changes of our code-base. Nevertheless, the robot is right, we should disable the feature flag here and use this feature-flag in ffi.cpp. I'll add a comment there as well.
You can access it tough, via #if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL).
| x86_32 -> "-m32" | ||
| x86_64 -> "-m64" | ||
| ppc64 -> "-m64" | ||
| zos -> "-fzos-le-char-mode=ascii -mnocsect -fno-short-enums -m64 -mzos-target=zosv3r1 -DNSIG=42 -D_XOPEN_SOURCE=600 -D_ALL_SOURCE -D_OPEN_SYS_FILE_EXT=1 -D_AE_BIMODAL=1 -D_ENHANCED_ASCII_EXT=0xFFFFFFFF" |
There was a problem hiding this comment.
This zos entry mixes clang codegen/ABI flags with a large set of z/OS feature macros (-D_XOPEN_SOURCE=..., _ALL_SOURCE, etc.) and also forces NSIG via -DNSIG=42. This makes the z/OS OS configuration clang-specific and can cause surprising header/API changes from defining common libc macros (and NSIG isn’t referenced anywhere in Botan).
Please move the z/OS feature macros into src/build-data/os/zos.txt’s <feature_macros> block, keep only the clang-specific -f.../-m... flags here, and avoid defining NSIG globally unless Botan code actually depends on it (in which case prefer a guarded fix at the use site).
| zos -> "-fzos-le-char-mode=ascii -mnocsect -fno-short-enums -m64 -mzos-target=zosv3r1 -DNSIG=42 -D_XOPEN_SOURCE=600 -D_ALL_SOURCE -D_OPEN_SYS_FILE_EXT=1 -D_AE_BIMODAL=1 -D_ENHANCED_ASCII_EXT=0xFFFFFFFF" | |
| zos -> "-fzos-le-char-mode=ascii -mnocsect -fno-short-enums -m64 -mzos-target=zosv3r1" |
reneme
left a comment
There was a problem hiding this comment.
Thanks for opening a PR for this. I left a few suggestions and also the comments made by Copilot seem relevant or reasonable to consider at least. The most relevant issue is centered around thread_local, where I would suggest to actually make use of Botan's OS-feature-flags as outlined in the comments.
I could work on a patch to use a mutex that guards the global error string, as I would extract this aspect from this PR.
| x86_32 -> "-m32" | ||
| x86_64 -> "-m64" | ||
| ppc64 -> "-m64" | ||
| zos -> "-fzos-le-char-mode=ascii -mnocsect -fno-short-enums -m64 -mzos-target=zosv3r1 -DNSIG=42 -D_XOPEN_SOURCE=600 -D_ALL_SOURCE -D_OPEN_SYS_FILE_EXT=1 -D_AE_BIMODAL=1 -D_ENHANCED_ASCII_EXT=0xFFFFFFFF" |
There was a problem hiding this comment.
Question: Isn't -mzos-target=zosv3r1 overly specific? Or does this just set some baseline for the supported OS version?
| # AIX and OpenBSD don't use sonames at all | ||
| aix -> "{cxx} -shared -fPIC" | ||
| zos -> "{cxx} -shared -fPIC" | ||
| openbsd -> "{cxx} -shared -fPIC" |
There was a problem hiding this comment.
| # AIX and OpenBSD don't use sonames at all | |
| aix -> "{cxx} -shared -fPIC" | |
| zos -> "{cxx} -shared -fPIC" | |
| openbsd -> "{cxx} -shared -fPIC" | |
| # These platforms don't use sonames at all | |
| aix -> "{cxx} -shared -fPIC" | |
| zos -> "{cxx} -shared -fPIC" | |
| openbsd -> "{cxx} -shared -fPIC" |
| sockets | ||
| system_clock | ||
| threads | ||
| thread_local |
There was a problem hiding this comment.
Interestingly, the thread_local feature flag that the robot refers to doesn't currently reflect in any compile-time changes of our code-base. Nevertheless, the robot is right, we should disable the feature flag here and use this feature-flag in ffi.cpp. I'll add a comment there as well.
You can access it tough, via #if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL).
| #if defined(__MVS__) | ||
| // z/OS has a bug with thread_local | ||
| #define thread_local | ||
| #endif |
There was a problem hiding this comment.
Instead of branching off for the z/OS compiler specifically, you could instead remove the thread_local feature flag in zos.txt as suggested elsewhere and use #if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL) here.
Like so:
#if defined(BOTAN_TARGET_OS_HAS_THREAD_LOCAL)
// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
thread_local std::string g_last_exception_what;
#else
// NOLINTNEXTLINE(*-avoid-non-const-global-variables)
std::string g_last_exception_what;
#endif... I guess we should introduce a mutex instead, to at least guard multithreaded access to this global, but that is out-of-scope for this change and we can do that independently.
8f65abe to
b538ff6
Compare
Add the z/OS build-data entry, clang ABI flags, shared library linker handling, and s390x cycle counter support. Move z/OS feature macros into the OS configuration and leave clang.txt with clang-specific flags only. Disable the thread_local OS feature for z/OS and gate FFI exception storage on BOTAN_TARGET_OS_HAS_THREAD_LOCAL. Tested with diff checks, z/OS configure generation, native configure, and native compilation of ffi.o and utils_os_utils.o.
b538ff6 to
a9182d0
Compare
|
Looking much better. CI failure is relevant though |
Summary
This PR adds comprehensive z/OS support to Botan, enabling the library to build and run successfully on IBM z/OS systems using the clang compiler.
Changes Made
Compiler Configuration (clang.txt)
-fzos-le-char-mode=ascii: Enable ASCII/UTF-8 character mode-mnocsect: Disable CSECT generation-fno-short-enums: Use standard enum sizes-m64 -mzos-target=zosv3r1: Target 64-bit z/OS v3.1Build Configuration (zos.txt)
_XOPEN_SOURCE=600,_ALL_SOURCE_OPEN_SYS_FILE_EXT=1,_AE_BIMODAL=1_ENHANCED_ASCII_EXT=0xFFFFFFFFNSIG=42Source Code Updates
Test Results
All (I think) 2797 tests passed successfully on z/OS
Ran with:
Platform