Fix CRC endianness detection and add s390x regression#179
Fix CRC endianness detection and add s390x regression#179PaulTaykalo merged 9 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes CRC fast-path endianness detection by switching to the established Crypto/brg_endian.h byte-order logic, and adds deterministic regression coverage (including an emulated s390x GitHub Actions run) to prevent big-endian regressions.
Changes:
- Replace CRC endianness detection with
PLATFORM_BYTE_ORDER == IS_BIG_ENDIANfromCrypto/brg_endian.h. - Add deterministic fast-vs-slow CRC regression checks (XCTest + standalone regression binary).
- Introduce a GitHub Actions workflow that builds/runs the regression under emulated
linux/s390x.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
CRC.m |
Switches the big-endian conditional compilation to brg_endian.h’s PLATFORM_BYTE_ORDER detection. |
XADMasterTests/CRCCalculationTests.m |
Adds deterministic buffers and assertions to ensure fast CRC matches slow CRC for specific sizes/tails. |
Tests/CRCFastRegression.m |
Adds a standalone deterministic regression executable to compare slow vs fast CRC results. |
.github/workflows/crc-big-endian.yml |
Adds an emulated s390x CI workflow to compile and run the regression binary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| push: | ||
| branches: [ master ] | ||
| # Keep this emulated s390x job scoped to the CRC/endian code it protects. |
There was a problem hiding this comment.
The workflow is restricted to the master branch for both push and pull_request, but the repository/PR diff base appears to be main (e.g., diff headers show index main..branch). If the default branch is main, this workflow won’t run for the PRs it’s meant to protect. Consider switching the branch filter to main, including both main and master, or omitting the branches filter and relying on the paths filter alone.
| if (slow != fast) { | ||
| fprintf(stderr, "%s mismatch: slow=0x%08x fast=0x%08x length=%d\n", label, slow, fast, length); |
There was a problem hiding this comment.
fprintf uses %08x to print uint32_t values (slow/fast). uint32_t is not guaranteed to be an unsigned int on all platforms, so this can trigger warnings or incorrect output on some architectures/toolchains. Consider including <inttypes.h> and using PRIx32 (or casting to an explicitly matching type) for portable formatting.
Summary
Crypto/brg_endian.hlogicXAD_BYTE_ORDER_BIG_ENDIANmacrolinux/s390xTesting
CRC Big EndianGitHub Actions workflow fors390x