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.

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

Add support for s390x architecture. #1431

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Add support for s390x architecture. #1431

merged 3 commits into from
Mar 4, 2024

Conversation

PiotrSikora
Copy link
Contributor

Description of changes:

Add support for s390x architecture.

This is pretty trivial change (a few defines), since the big-endian support was previously added for PowerPC architectures.

Call-outs:

There is a false-positive error when compiling using s390x-linux-gnu-gcc-11 with -O3 (i.e. Release profile):

crypto/pem/pem_lib.c:705:11: error: ‘strncmp’ of strings of length 1 and 9 and bound of 9 evaluates to nonzero [-Werror=string-compare]
 705 |       if (strncmp(buf, "-----END ", 9) == 0) {
     |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

but there are no issues when using -O2 or when building with s390x-linux-gnu-gcc-12.

Testing:

All tests from run_cross_tests.sh, i.e.

crypto/crypto_test --gtest_also_run_disabled_tests
crypto/urandom_test
crypto/mem_test
crypto/mem_set_test
crypto/dynamic_loading_test

ssl/ssl_test
ssl/integration_test

passed, with the exception of BIOTest.InvokeConnectCallback, which fails for me on all platforms.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

cc @andrewhop @justsmth

@PiotrSikora PiotrSikora requested a review from a team as a code owner February 5, 2024 07:25
@justsmth
Copy link
Contributor

justsmth commented Feb 5, 2024

Could this be added to our cross-tests? (After fixing BIOTest.InvokeConnectCallback?)

@justsmth justsmth self-requested a review February 5, 2024 13:36
@PiotrSikora
Copy link
Contributor Author

Could this be added to our cross-tests?

It can be added, but I believe that someone from your team has to upload the cross-compiling toolchain for s390x, which is downloaded as part of the CI (see: tests/ci/run_cross_tests.sh#L34).

(After fixing BIOTest.InvokeConnectCallback?)

As mentioned in the original message, this test is always failing for me without any code changes in all the configurations I've tested (i.e. when running unmodified main branch on native x86-64 system, and also when cross-compiling to different architectures), and I don't believe there is anything s390x-specific about this failure.

Anyway, it looks that the test was broken: #1433.

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.94%. Comparing base (67cf4cc) to head (853b015).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1431      +/-   ##
==========================================
- Coverage   76.95%   76.94%   -0.02%     
==========================================
  Files         425      425              
  Lines       71587    71587              
==========================================
- Hits        55093    55080      -13     
- Misses      16494    16507      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justsmth
Copy link
Contributor

Sorry for the delay in reviewing this. I plan to build a toolchain to perform testing for this platform. I'll hopefully have an update on it soon.

Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
@justsmth
Copy link
Contributor

justsmth commented Mar 4, 2024

@justsmth justsmth merged commit e16ea3d into aws:main Mar 4, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants