Fix a big endian bug on the 32k and larger specializations of chorba#1891
Merged
Dead2 merged 1 commit intozlib-ng:developfrom Mar 28, 2025
Merged
Conversation
Contributor
WalkthroughThis pull request modifies CRC32 functions in Changes
Sequence Diagram(s)sequenceDiagram
participant CRC_Function as crc32_chorba Function
participant System as CPU/Environment
CRC_Function->>System: Check BYTE_ORDER
alt BYTE_ORDER == LITTLE_ENDIAN
System-->>CRC_Function: Return crc directly
else
System-->>CRC_Function: Return ZSWAP64(crc)
end
sequenceDiagram
participant TestRunner as Test Framework
participant StaticInit as Static Initializer
participant SetupBuffer as setup_buf Function
TestRunner->>StaticInit: Initialize static variable 'setup'
StaticInit->>SetupBuffer: Call setup_buf() to initialize fullwin_buf
SetupBuffer-->>StaticInit: Return initialization success
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
be11e0e to
d0fda47
Compare
Contributor
|
definitely sounds like crc table at the end, the large version is copying
whole qwords at a time which should be endian independent
…On Tue, Mar 25, 2025, 23:01 Adam Stylinski ***@***.***> wrote:
It turns out there's something in there preventing them from being endian
independent. It's likely something to do with how the crc table is used at
the end but until now, we'll disable.
I've also added a test case which enabled me to spot this issue while
testing another SIMD acceleration of chorba.
@samrussell <https://github.com/samrussell> might be able to fix this but
I figured I'd commit things back to a working state, first.
------------------------------
You can view, comment on, or merge this pull request online at:
#1891
Commit Summary
- be11e0e
<be11e0e>
Disable the 32k and larger specializations of chorba on big endian
File Changes
(3 files <https://github.com/zlib-ng/zlib-ng/pull/1891/files>)
- *M* arch/generic/crc32_c.c
<https://github.com/zlib-ng/zlib-ng/pull/1891/files#diff-03af5f1d110144636117965edd072c35b842a7fad06af14e1ee15415216a1aa4>
(8)
- *M* arch/generic/crc32_chorba_c.c
<https://github.com/zlib-ng/zlib-ng/pull/1891/files#diff-dda9b309e4e59ad092fc8f5e9c5c2305f5c34705c6e1dc47398e7bf884378ffc>
(2)
- *M* test/test_crc32.cc
<https://github.com/zlib-ng/zlib-ng/pull/1891/files#diff-1f45a9de31adec84d54ad1399f4e25d91c01839348706f0d316188c2c9ae8cda>
(18)
Patch Links:
- https://github.com/zlib-ng/zlib-ng/pull/1891.patch
- https://github.com/zlib-ng/zlib-ng/pull/1891.diff
—
Reply to this email directly, view it on GitHub
<#1891>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALJUP7ZMAJC5ABKMMBQ2W32WHG5JAVCNFSM6AAAAABZYYNCCCVHI2DSMVQWIX3LMV43ASLTON2WKOZSHE2DOOBXGM3DSNQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1891 +/- ##
===========================================
+ Coverage 82.00% 82.99% +0.99%
===========================================
Files 141 138 -3
Lines 12666 12528 -138
Branches 2906 2812 -94
===========================================
+ Hits 10387 10398 +11
+ Misses 1290 1199 -91
+ Partials 989 931 -58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Collaborator
Author
|
Hah, yeah that was it, at least for the 32k variant. |
d0fda47 to
594a9a2
Compare
594a9a2 to
e0549fd
Compare
nmoinvaz
reviewed
Mar 25, 2025
nmoinvaz
reviewed
Mar 25, 2025
e0549fd to
712e56d
Compare
In testing a SIMD vectorization for this, I wrote a gtest which stumbled onto the fact that this had a bug on big endian. Before the initial CRC had been mixed in it needed to be byte swapped.
712e56d to
32812e6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
arch/generic/crc32_chorba_c.c(3 hunks)test/test_crc32.cc(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
arch/generic/crc32_chorba_c.c (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-03-26T15:10:53.588Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
🪛 Cppcheck (2.10-2)
test/test_crc32.cc
[error] 29-29: There is an unknown macro here somewhere. Configuration is required. If ALIGNED_ is a macro then please configure it.
(unknownMacro)
arch/generic/crc32_chorba_c.c
[error] 648-648: Uninitialized variable
(uninitvar)
⏰ Context from checks skipped due to timeout of 90000ms (61)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: Windows MSVC 2019 v141 Win64
- GitHub Check: Ubuntu GCC Native Instructions (AVX)
- GitHub Check: Ubuntu 20.04 GCC
- GitHub Check: Ubuntu GCC Symbol Prefix
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
🔇 Additional comments (5)
test/test_crc32.cc (3)
29-38: Good addition of the test buffer for the 32K specialization.The implementation of a properly aligned 32768-byte buffer with a helper function to initialize it systematically is a good approach for testing the CRC32 algorithm with large inputs.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 29-29: There is an unknown macro here somewhere. Configuration is required. If ALIGNED_ is a macro then please configure it.
(unknownMacro)
195-196: Good test case addition for 32K buffer size.This test case specifically targets the 32K specialization of the chorba algorithm that was having the endian issue. The expected CRC value 0x217726B2 should verify correctness on both little and big endian systems.
40-40:Details
✅ Verification successful
Static initialization looks good.
Using the
setup_buf()function to initialize the static buffer ensures the test data is properly prepared before any tests run.
🏁 Script executed:
#!/bin/bash # Verify that the static buffer initialization approach is effective by checking if any test functions # directly reference the static variable before it's initialized # Search for any usage of buf32k in test functions rg -A 3 -B 3 "buf32k" test/test_crc32.ccLength of output: 631
Static Buffer Initialization Confirmed
The static variable
buf32kis correctly initialized via thesetup_buf()function before being referenced in thetestsarray, and no test functions use it prior to initialization. The output from the verification script confirms that the buffer is set up in the proper order.arch/generic/crc32_chorba_c.c (2)
647-647: Minor style fix with added space after 'for'.Adding a space after the
forkeyword improves code consistency with the rest of the codebase.
647-649: Potential uninitialized variable issue in the loop.According to static analysis, there might be an uninitialized variable in this loop. It's worth reviewing this loop to ensure all variables are properly initialized.
While the static analyzer flags a potential issue, nothing obvious stands out. The loop variables appear to be properly initialized. Could be a false positive, but worth double-checking the code path that leads to this loop to ensure all variables (
crc,final_bytes, andbitbufferbytes) are properly initialized before use.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 648-648: Uninitialized variable
(uninitvar)
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out there's something in there preventing them from being endian independent. @samrussell found the issue in this PR, it's simply the fact that we weren't byteswapping the crc before folding it into the multiplications. While the value was 0xFFFFFFFF - it still gets widened to a 64 bit variable, which means the 4 bytes ended up falling on the wrong half of the word.
I've also added a test case which enabled me to spot this issue while testing another SIMD acceleration of chorba.
Summary by CodeRabbit
Refactor
Tests
0x217726B2.