Fix "RLE" compression with big endian architectures#1832
Fix "RLE" compression with big endian architectures#1832Dead2 merged 1 commit intozlib-ng:developfrom
Conversation
This was missed in zlib-ng#1831. The RLE methods compare a string of bytes directly with itself to directly derive a simple run length encoding. They use similar but not identical methods to compare256. This needs a similar endianness check at compile time to know which compare bit count to use (leading or trailing).
WalkthroughThe pull request introduces a new header file Changes
Sequence DiagramsequenceDiagram
participant Caller
participant compare256_rle_func
participant Input Arrays
Caller->>compare256_rle_func: Call with src0, src1
compare256_rle_func->>Input Arrays: Compare bytes
compare256_rle_func-->>Caller: Return matching byte length
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
compare256_rle.h (1)
126-130: Validate the 64-bit endianness handling.
Similar to the 32-bit version, this code properly uses __builtin_ctzll and __builtin_clzll for little- and big-endian systems. As a best practice, verify that the code compiles and runs on both big-endian and little-endian systems, possibly through continuous integration or an emulator-based test.Would you like help setting up an emulator-based or cross-compilation build pipeline to test big-endian architectures automatically?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
compare256_rle.h(3 hunks)
🔇 Additional comments (2)
compare256_rle.h (2)
92-96: Double-check built-in usage for big-endian.
The usage of __builtin_ctz vs __builtin_clz for big-endian is correct and ensures that we detect the first differing byte on both architectures. However, confirm that additional alternative branches (e.g., #elif) are discarded properly for other architectures that might not be strictly little- or big-endian.
8-8: Confirm necessity of 'zendian.h' include.
This header seems essential for detecting and handling byte endianness, but please verify if any part of this file depends on macros or definitions from 'zendian.h'. If it's only needed for the LITTLE_ENDIAN or BIG_ENDIAN macros, confirm that they're defined if the compiler doesn't, or provide a fallback.
Run the following shell script to check references of endian macros across the repository and confirm usage:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information to verify the necessity of the 'zendian.h' include.
'zendian.h' include is necessary - used for endianness-dependent code
The file actively uses endianness macros from zendian.h for conditional compilation. Specifically:
- Uses
BYTE_ORDER == LITTLE_ENDIANchecks for different byte counting implementations - Contains endianness-specific code paths using
__builtin_ctz/__builtin_clzfor both 32-bit and 64-bit operations - The header provides essential endianness detection and fallback definitions across different platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of endian macros across the repository, ensuring 'zendian.h' is required.
# Searching for occurrences of LITTLE_ENDIAN, BIG_ENDIAN, or zendian.h usage.
rg -A 3 -i 'LITTLE_ENDIAN|BIG_ENDIAN|zendian\.h'
Length of output: 11908
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1832 +/- ##
===========================================
+ Coverage 82.10% 82.39% +0.29%
===========================================
Files 136 136
Lines 10969 11020 +51
Branches 2716 2799 +83
===========================================
+ Hits 9006 9080 +74
+ Misses 1266 1241 -25
- Partials 697 699 +2 ☔ View full report in Codecov by Sentry. |
|
CI errors (Ubuntu GCC AARCH64 No NEON UBSAN): |
|
Yes, those are not new, they were introduced in a PR that re-enabled the alignment sanitizer. A subsequent PR is likely to follow to fix that one, but it's unrelated to this. |
| diff = sv ^ mv; | ||
| if (diff) { | ||
| #if BYTE_ORDER == LITTLE_ENDIAN | ||
| uint32_t match_byte = __builtin_ctz(diff) / 8; |
There was a problem hiding this comment.
Would it be worth using macros for this so that the endianness checks (and the builtin availability checks) can be performed in one place?
There was a problem hiding this comment.
As of right now this is only ever used in two files in 2 functions. Maybe it'd be worth a second PR, though.
This was missed in #1831. The RLE methods compare a string of bytes directly with itself to directly derive a simple run length encoding. They use similar but not identical methods to compare256. This needs a similar endianness check at compile time to know which compare bit count to use (leading or trailing).
Summary by CodeRabbit