Disable CRC32-VX Extention for some Clang versions#1852
Disable CRC32-VX Extention for some Clang versions#1852Dead2 merged 1 commit intozlib-ng:developfrom
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (130)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1852 +/- ##
===========================================
+ Coverage 74.84% 80.35% +5.50%
===========================================
Files 127 138 +11
Lines 8011 11147 +3136
Branches 2141 2860 +719
===========================================
+ Hits 5996 8957 +2961
- Misses 993 1223 +230
+ Partials 1022 967 -55 ☔ View full report in Codecov by Sentry. |
|
@fneddy Codefactor had a couple suggestions to improve portability and performance of the changes to Configure, would be good if you considered these changes, both suggestions might potentially otherwise become parts of a future fix/cleanup PR if someone decides to do some janitorial work on Configure. Other than that, this looks pretty good to me. Have not tested though. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
configure (1)
1468-1471: Add input validation to version_number function.The function assumes well-formed version strings but could fail or produce incorrect results with:
- Version strings with more than 3 components (e.g., "18.0.0.1")
- Version strings with non-numeric components (e.g., "18.0.0-rc1")
- Version numbers > 255 in any component
Consider adding input validation and error handling:
version_number() { # convert a version from string into a number, assuming 8 bits per nibble - echo "$1" | awk -F '.' '{printf "%d", lshift($1,16) + lshift($2,8) + $3}' + echo "$1" | awk -F '.' ' + # Validate format + NF > 3 { exit 1 } + $1 !~ /^[0-9]+$/ || $2 !~ /^[0-9]+$/ || $3 !~ /^[0-9]+$/ { exit 1 } + # Validate ranges + $1 > 255 || $2 > 255 || $3 > 255 { exit 1 } + # Convert to number + { printf "%d", lshift($1,16) + lshift($2,8) + $3 } + ' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)configure(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms (150)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No NEON UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC ARM HF ASAN
- GitHub Check: Ubuntu GCC 32-bit
|
@fneddy Regarding warning the user about the problem with the clang version, I do prefer to not hide it. Personally I would actually prefer it be a visible warning or even an error to enabling VX on known broken Clang versions. Configure and CMake should have the same behavior for this. Suggestion for discussion: If error is the way to go, then this check could perhaps instead be done in |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
arch/s390/s390_functions.h (1)
Line range hint
25-28: Fix syntax error in native_crc32 macro definition.The macro definition contains an incorrect assignment operator that would cause a compilation error.
#ifdef DISABLE_RUNTIME_CPU_DETECTION # if defined(S390_CRC32_VX) && defined(__zarch__) && __ARCH__ >= 11 && defined(__VX__) # undef native_crc32 -# define native_crc32 = crc32_s390_vx +# define native_crc32 crc32_s390_vx # endif #endif
🧹 Nitpick comments (1)
arch/s390/s390_functions.h (1)
14-20: Consider improving the version check readability and maintainability.While the version check logic is correct, consider these improvements:
- Split the version check into separate macros for better maintainability
- Break the long message string into multiple lines for better readability
+#define CLANG_VERSION(major, minor, patch) \ + (__clang_major__ == major && __clang_minor__ == minor && __clang_patchlevel__ == patch) +#define CLANG_VERSION_GE(major, minor, patch) \ + (__clang_major__ > major || \ + (__clang_major__ == major && \ + (__clang_minor__ > minor || \ + (__clang_minor__ == minor && __clang_patchlevel__ >= patch)))) +#define CLANG_VERSION_LT(major, minor, patch) \ + (!CLANG_VERSION_GE(major, minor, patch)) + #ifdef __clang__ -# if ( (__clang_major__ >=18) && (__clang_major__ <= 19 && __clang_minor__ <= 1 && __clang_patchlevel__ < 2) ) +# if (CLANG_VERSION_GE(18, 0, 0) && CLANG_VERSION_LT(19, 1, 2)) # pragma message "CRC32-VX extension is broken on current Clang version (" \ XSTR(__clang_major__) "." XSTR(__clang_minor__) "." XSTR(__clang_patchlevel__) "). \n" \ "Affected Clang versions are: 18.0.0 <= clang_version < 19.1.2. \n" \ "Either disable CRC32-VX extension or switch to another compiler." # error "Incompatible compiler, feature flag combination." # endif #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arch/s390/s390_functions.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (164)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN
- GitHub Check: macOS Clang ASAN (ARM64)
- GitHub Check: macOS Clang ASAN
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v140 Win32
- GitHub Check: Ubuntu Emscripten WASM32
- GitHub Check: Ubuntu Clang RISC-V
- GitHub Check: Ubuntu Clang MSAN
- GitHub Check: Ubuntu 20.04 Clang
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: Ubuntu GCC PPC64LE
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: Ubuntu GCC 32-bit
🔇 Additional comments (3)
arch/s390/s390_functions.h (3)
8-9: LGTM! Well-implemented stringification macros.The double-level macro expansion is correctly implemented to ensure proper stringification of version numbers.
12-12: LGTM! Clear function declaration.The function declaration is properly guarded by the S390_CRC32_VX feature flag.
14-20: Implementation aligns well with PR objectives.The code successfully:
- Identifies problematic Clang versions
- Makes the issue visible through warning messages
- Prevents compilation with incompatible versions
- Implements the check in s390_functions.h as suggested in PR comments
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
arch/s390/s390_functions.h (1)
Line range hint
21-23: Fix the macro definition syntax error.The macro definition contains an incorrect assignment operator that would cause compilation errors.
#ifdef DISABLE_RUNTIME_CPU_DETECTION # if defined(S390_CRC32_VX) && defined(__zarch__) && __ARCH__ >= 11 && defined(__VX__) # undef native_crc32 -# define native_crc32 = crc32_s390_vx +# define native_crc32 crc32_s390_vx # endif #endif
🧹 Nitpick comments (1)
arch/s390/s390_functions.h (1)
11-16: Consider improving version check maintainability.While the version check correctly addresses the Clang issue mentioned in #1845, consider these improvements:
- Define version numbers as macros for better maintainability
- Split the error message for better readability
+/* Clang versions with broken CRC32-VX */ +#define CLANG_MIN_BROKEN_VERSION 180000 /* 18.0.0 */ +#define CLANG_MAX_BROKEN_VERSION 190102 /* 19.1.2 */ + #ifdef __clang__ -# if ( (__clang_major__ >=18) && (__clang_major__ <= 19 && __clang_minor__ <= 1 && __clang_patchlevel__ < 2) ) +# if (__clang_major__ * 10000 + __clang_minor__ * 100 + __clang_patchlevel__ >= CLANG_MIN_BROKEN_VERSION && \ + __clang_major__ * 10000 + __clang_minor__ * 100 + __clang_patchlevel__ < CLANG_MAX_BROKEN_VERSION) # error "CRC32-VX extension is broken on current Clang version. " \ - "Affected Clang versions are: 18.0.0 <= clang_version < 19.1.2. " \ - "Either disable CRC32-VX extension or switch to another compiler." + "Affected Clang versions are: 18.0.0 <= version < 19.1.2.\n" \ + "To fix: Either disable CRC32-VX extension or switch to another compiler." # endif #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arch/s390/s390_functions.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (100)
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Ubuntu GCC ARM HF No NEON
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Ubuntu GCC ARM HF No NEON
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Ubuntu GCC ARM HF No NEON
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Ubuntu GCC ARM HF No NEON
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Ubuntu GCC AARCH64
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix
- GitHub Check: Windows GCC Compat No Opt
- GitHub Check: Windows ClangCl Win64 Native Instructions (AVX)
- GitHub Check: Windows ClangCl Win64
- GitHub Check: Windows ClangCl Win32
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Ubuntu MinGW i686
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix
🔇 Additional comments (1)
arch/s390/s390_functions.h (1)
10-10: LGTM!The function declaration is properly guarded and maintains the expected signature.
|
@fneddy If you could do a rebase, that'd be great. CI is too broken to see anything useful after github actions image update. |
|
ok, hold on. I fix this. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
arch/s390/self-hosted-builder/actions-runner.service (1)
13-14: Re-evaluate the necessity of increased timeout valuesThe
ExecStopandExecStopPostcommands now have increased timeouts of 30 and 10 seconds, respectively. According to previous learnings, systemd already manages shutdown timeouts and retries for services, which may render these manual adjustments unnecessary.I recalled from prior learnings:
Learning: Systemd already handles shutdown timeout and retry for the actions-runner service, so it is not necessary to manually implement timeouts in actions-runner-rebuild.sh.
Please consider whether these increased timeouts are necessary. Unnecessarily long timeouts could delay the shutdown process without providing significant benefits. It might be preferable to rely on systemd's default behavior unless there's a specific need for these adjustments.
arch/generic/chunkset_c.c (1)
16-17: Unrelated change: Memory operations refactored.The replacement of memcpy with specialized zng_mem* functions appears to be a performance optimization unrelated to the PR's objective. While these changes look beneficial, they should be part of a separate PR focused on memory operation improvements.
Also applies to: 21-21, 25-25, 29-29
arch/x86/chunkset_sse2.c (1)
6-6: Unrelated change: SSE2 memory operations refactored.These changes are part of the broader memory operation optimization effort and are unrelated to the PR's objective of disabling CRC32-VX for problematic Clang versions. Consider moving these changes to a separate PR.
Also applies to: 18-18, 22-22, 26-26
arch/power/chunkset_power8.c (1)
8-8: Unrelated change: POWER8 memory operations refactored.These changes are part of the broader memory operation optimization effort and are unrelated to the PR's objective of disabling CRC32-VX for problematic Clang versions. Consider moving these changes to a separate PR.
Also applies to: 19-19, 23-23, 27-27
test/test_compare256_rle.cc (1)
53-59: Consider adding edge case testsWhile the bit-width specific test cases look good, consider adding tests for:
- Edge cases with different alignment offsets
- Input strings with repeated patterns
- Input strings with no matches
arch/s390/dfltcc_common.c (2)
55-59: Improve the DFLTCC_LEVEL_MASK range check message.The error message could be more specific about which bits are invalid.
- Trace((stderr, "DFLTCC_LEVEL_MASK %lu out of range." - "Allowed range is bit[0-10] (0x0 - 0x3ff). Invalid bits will be ignored.\n", + Trace((stderr, "DFLTCC_LEVEL_MASK 0x%lx contains invalid bits." + " Only bits 0-9 (0x0 - 0x3ff) are valid. Bits 0x%lx will be ignored.\n", env_dfltcc_level_mask)); + env_dfltcc_level_mask & ~0x3FF));
63-67: Simplify the power of 2 check using bitwise operations.The power of 2 check can be simplified using a well-known bitwise operation.
- if (env_dfltcc_block_size & (env_dfltcc_block_size - 1) /* power of 2 */) { + if (env_dfltcc_block_size == 0 || (env_dfltcc_block_size & (env_dfltcc_block_size - 1))) { Trace((stderr,"DFLTCC_BLOCK_SIZE %lu out of range. Value must be power of 2 and greater 0x40000." " Reseting to default(%d)\n", env_dfltcc_block_size, DFLTCC_BLOCK_SIZE)); env_dfltcc_block_size = DFLTCC_BLOCK_SIZE; }arch/s390/dfltcc_common.h (1)
6-87: LGTM! Consider adding validation for environment variables.The runtime tuning parameters are well-documented with clear descriptions and valid ranges. The environment variables provide good flexibility for configuring DFLTCC behavior.
Consider adding validation for the environment variables to ensure they are within their valid ranges, especially for:
DFLTCC_LEVEL_MASK: 0 - [1<<10]DFLTCC_BLOCK_SIZE: > 262144DFLTCC_RIBM: 0-255arch/s390/README.md (1)
234-242: Improve configuration file documentation.Consider adding:
- A template for the configuration file
- Example values with proper formatting
- Warning about securing the PAT token
🧰 Tools
🪛 Markdownlint (0.37.0)
234-234: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
235-235: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
241-241: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
235-235: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
241-241: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
zmemory.h (1)
72-97: Consider adding bounds checking for memory comparison functions.While the implementation is correct, consider adding bounds checking to prevent potential buffer overflows when comparing memory regions.
static inline int32_t zng_memcmp_2(const void *src0, const void *src1) { + /* Ensure both pointers are not NULL */ + if (src0 == NULL || src1 == NULL) + return 0; #if defined(HAVE_MAY_ALIAS) return zng_memread_2(src0) != zng_memread_2(src1); #else return memcmp(src0, src1, 2); #endif }compare256_rle.h (1)
Line range hint
12-45: Consider unrolling the loop for better performance.The 8-bit comparison function could benefit from loop unrolling to reduce branch predictions.
static inline uint32_t compare256_rle_8(const uint8_t *src0, const uint8_t *src1) { uint32_t len = 0; + const uint8_t val = *src0; do { - if (*src0 != *src1) + if (val != src1[0]) return len; - src1 += 1, len += 1; - if (*src0 != *src1) + if (val != src1[1]) + return len + 1; + if (val != src1[2]) + return len + 2; + if (val != src1[3]) + return len + 3; + src1 += 4; + len += 4; - src1 += 1, len += 1; - // ... remaining comparisons } while (len < 256);arch/generic/compare256_p.h (1)
69-123: Consider adding compile-time size checks for optimized comparisons.While the implementation is correct, it would be beneficial to add compile-time assertions to ensure the size assumptions hold.
#ifdef HAVE_BUILTIN_CTZ +/* Ensure proper alignment for 32-bit access */ +static_assert(sizeof(uint32_t) == 4, "uint32_t must be 4 bytes"); /* 32-bit integer comparison */ static inline uint32_t compare256_32(const uint8_t *src0, const uint8_t *src1) {arch/arm/crc32_acle.c (1)
32-35: Verify memory access safety.The direct memory access using type casting (e.g.,
*((uint16_t*)buf)) requires proper alignment. While the code checks for alignment before access, consider using safer memory access functions or documenting the alignment requirements.Consider using memory access functions that handle alignment:
- buf2 = *((uint16_t*)buf); + memcpy(&buf2, buf, sizeof(uint16_t));Or document the alignment requirements in the function documentation.
Also applies to: 39-43, 49-52, 56-59, 63-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
.github/workflows/cmake.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(3 hunks)CMakeLists.txt(12 hunks)Makefile.in(1 hunks)README.md(1 hunks)arch/arm/chunkset_neon.c(2 hunks)arch/arm/compare256_neon.c(1 hunks)arch/arm/crc32_acle.c(2 hunks)arch/generic/Makefile.in(1 hunks)arch/generic/chunkset_c.c(2 hunks)arch/generic/compare256_c.c(1 hunks)arch/generic/compare256_p.h(1 hunks)arch/generic/generic_functions.h(1 hunks)arch/power/chunkset_power8.c(2 hunks)arch/power/compare256_power9.c(1 hunks)arch/riscv/adler32_rvv.c(2 hunks)arch/riscv/compare256_rvv.c(1 hunks)arch/riscv/riscv_features.c(1 hunks)arch/s390/Makefile.in(1 hunks)arch/s390/README.md(1 hunks)arch/s390/dfltcc_common.c(1 hunks)arch/s390/dfltcc_common.h(1 hunks)arch/s390/dfltcc_deflate.c(2 hunks)arch/s390/dfltcc_detail.h(2 hunks)arch/s390/dfltcc_inflate.c(2 hunks)arch/s390/s390_functions.h(1 hunks)arch/s390/self-hosted-builder/actions-runner(1 hunks)arch/s390/self-hosted-builder/actions-runner-rebuild.sh(1 hunks)arch/s390/self-hosted-builder/actions-runner.Dockerfile(2 hunks)arch/s390/self-hosted-builder/actions-runner.service(1 hunks)arch/s390/self-hosted-builder/fs/usr/bin/actions-runner(0 hunks)arch/s390/self-hosted-builder/runner-global.json(0 hunks)arch/s390/self-hosted-builder/runner-s390x.patch(0 hunks)arch/x86/Makefile.in(1 hunks)arch/x86/chunkset_avx2.c(2 hunks)arch/x86/chunkset_avx512.c(4 hunks)arch/x86/chunkset_sse2.c(2 hunks)arch/x86/chunkset_ssse3.c(2 hunks)arch/x86/compare256_avx2.c(1 hunks)arch/x86/compare256_sse2.c(1 hunks)chunkset_tpl.h(1 hunks)cmake/detect-arch.c(1 hunks)cmake/detect-intrinsics.cmake(1 hunks)cmake/detect-sanitizer.cmake(1 hunks)compare256_rle.h(4 hunks)configure(5 hunks)deflate.h(6 hunks)deflate_quick.c(1 hunks)deflate_rle.c(1 hunks)functable.c(2 hunks)inflate_p.h(3 hunks)insert_string_tpl.h(1 hunks)match_tpl.h(5 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/benchmarks/benchmark_compare256.cc(2 hunks)test/benchmarks/benchmark_compare256_rle.cc(1 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(1 hunks)test/test_compare256.cc(2 hunks)test/test_compare256_rle.cc(1 hunks)test/test_crc32.cc(3 hunks)win32/Makefile.a64(2 hunks)win32/Makefile.arm(2 hunks)win32/Makefile.msc(2 hunks)zbuild.h(3 hunks)zlib-ng.h.in(2 hunks)zlib.h.in(2 hunks)zmemory.h(1 hunks)zutil.c(1 hunks)zutil_p.h(0 hunks)
💤 Files with no reviewable changes (4)
- zutil_p.h
- arch/s390/self-hosted-builder/runner-global.json
- arch/s390/self-hosted-builder/fs/usr/bin/actions-runner
- arch/s390/self-hosted-builder/runner-s390x.patch
✅ Files skipped from review due to trivial changes (12)
- test/fuzz/CMakeLists.txt
- arch/power/compare256_power9.c
- arch/x86/compare256_sse2.c
- Makefile.in
- arch/arm/compare256_neon.c
- deflate_quick.c
- test/infcover.c
- arch/x86/compare256_avx2.c
- arch/riscv/riscv_features.c
- cmake/detect-arch.c
- zutil.c
- zlib-ng.h.in
🚧 Files skipped from review as they are similar to previous changes (1)
- arch/s390/s390_functions.h
🧰 Additional context used
📓 Learnings (7)
arch/s390/dfltcc_inflate.c (1)
Learnt from: fneddy
PR: zlib-ng/zlib-ng#1813
File: arch/s390/dfltcc_inflate.c:34-36
Timestamp: 2024-11-27T14:17:29.562Z
Learning: In the zlib-ng codebase, the variable `env_dfltcc_disabled` is initialized once during the library constructor `init_globals()` in `arch/s390/dfltcc_common.c`, and does not change afterward; therefore, accessing `env_dfltcc_disabled` in multiple threads does not introduce concurrency issues.
arch/s390/self-hosted-builder/actions-runner-rebuild.sh (2)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1846
File: arch/s390/self-hosted-builder/actions-runner-rebuild.sh:16-19
Timestamp: 2025-01-05T11:57:38.357Z
Learning: It is acceptable to remove the podman container without checking for its existence in actions-runner-rebuild.sh.
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1846
File: arch/s390/self-hosted-builder/actions-runner-rebuild.sh:22-25
Timestamp: 2025-01-05T12:02:00.645Z
Learning: User prefers overwriting logs in the self-hosted builder script rather than rotating them, due to an automated environment. They also do not want the script to exit on errors because no human actively monitors the container build process.
arch/s390/self-hosted-builder/actions-runner.service (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1846
File: arch/s390/self-hosted-builder/actions-runner-rebuild.sh:16-19
Timestamp: 2025-01-05T11:57:38.357Z
Learning: Systemd already handles shutdown timeout and retry for the actions-runner service, so it is not necessary to manually implement timeouts in actions-runner-rebuild.sh.
arch/s390/dfltcc_common.c (1)
Learnt from: fneddy
PR: zlib-ng/zlib-ng#1813
File: arch/s390/dfltcc_inflate.c:34-36
Timestamp: 2024-11-27T14:17:29.562Z
Learning: In the zlib-ng codebase, the variable `env_dfltcc_disabled` is initialized once during the library constructor `init_globals()` in `arch/s390/dfltcc_common.c`, and does not change afterward; therefore, accessing `env_dfltcc_disabled` in multiple threads does not introduce concurrency issues.
chunkset_tpl.h (1)
Learnt from: KungFuJesus
PR: zlib-ng/zlib-ng#1802
File: chunkset_tpl.h:135-135
Timestamp: 2024-11-12T09:04:30.407Z
Learning: In the `CHUNKMEMSET` function within `chunkset_tpl.h`, extra bounds checks are avoided to maintain performance in critical code sections. Branching is minimized to prevent negative impacts on speculative execution. The variable `len` is enforced with `safelen` early on.
test/test_crc32.cc (1)
Learnt from: KungFuJesus
PR: zlib-ng/zlib-ng#1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
arch/generic/generic_functions.h (1)
Learnt from: Dead2
PR: zlib-ng/zlib-ng#1830
File: arch/generic/generic_functions.h:39-53
Timestamp: 2024-12-20T23:35:59.830Z
Learning: The `longest_match_unaligned_*` functions are templated using the LONGEST_MATCH macro in match_tpl.h, so their implementations are generated rather than explicitly defined.
🪛 GitHub Actions: Static Analysis
arch/s390/dfltcc_inflate.c
[warning] 856-856: Left shift overflows the capacity of 'unsigned int' in NEEDBITS macro
[warning] 866-866: Left shift overflows the capacity of 'unsigned int' in NEEDBITS macro
[warning] 872-872: Left shift overflows the capacity of 'unsigned int' in NEEDBITS macro
[warning] 953-953: Left shift overflows the capacity of 'unsigned int' in PULLBYTE macro
[warning] 1017-1017: Left shift overflows the capacity of 'unsigned int' in PULLBYTE macro
🪛 Markdownlint (0.37.0)
arch/s390/README.md
253-253: null
Dollar signs used before commands without showing output
(MD014, commands-show-output)
229-229: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
234-234: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
245-245: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
251-251: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
232-232: null
Bare URL used
(MD034, no-bare-urls)
235-235: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
241-241: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
246-246: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
252-252: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
235-235: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
241-241: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
246-246: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
252-252: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
257-257: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
263-263: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🪛 cppcheck (2.10-2)
arch/s390/dfltcc_deflate.c
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If PREFIX is a macro then please configure it.
(unknownMacro)
test/benchmarks/benchmark_compare256_rle.cc
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
test/benchmarks/benchmark_compare256.cc
[error] 68-68: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (98)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
- GitHub Check: macOS Clang Native Instructions (ARM64)
- GitHub Check: Windows MSVC 2019 v140 Win64
- GitHub Check: Windows MSVC 2019 v141 Win32
- GitHub Check: Ubuntu GCC AARCH64 Compat No Opt UBSAN
- GitHub Check: Ubuntu GCC AARCH64 No ACLE UBSAN
- GitHub Check: macOS GCC
- GitHub Check: Ubuntu GCC AARCH64 ASAN
- GitHub Check: macOS GCC Symbol Prefix & Compat (ARM64)
- GitHub Check: macOS GCC Symbol Prefix & Compat
- GitHub Check: macOS GCC Symbol Prefix (ARM64)
🔇 Additional comments (80)
cmake/detect-intrinsics.cmake (1)
154-154: LGTM! Consistent BMI2 support detection.The addition of
-mbmi2flag is consistently implemented for both Intel and GNU/Clang compilers, ensuring proper detection of BMI2 support alongside AVX2.Also applies to: 159-159
arch/generic/compare256_c.c (4)
7-8: Include necessary header for comparison functionsIncluding
"compare256_p.h"provides access to the inline comparison functions needed for the optimized implementations. This inclusion is appropriate and ensures the required functions are available.
9-18: Verify macro definitions forCOMPARE256selectionThe conditional macros determine the optimal
COMPARE256function based onOPTIMAL_CMPand the availability of built-in functions like__builtin_ctzand__builtin_ctzll. Please ensure thatOPTIMAL_CMP,HAVE_BUILTIN_CTZ, andHAVE_BUILTIN_CTZLLare correctly defined in the build configuration to select the most efficient comparison function and prevent any unintended fallbacks.
21-21: Utilize theCOMPARE256macro incompare256_cThe
compare256_cfunction now callsCOMPARE256(src0, src1), which invokes the selected comparison function based on the macros defined above. This change enhances flexibility and maintainability by centralizing the comparison logic.
24-28: Generatelongest_matchfunctions using templatesBy defining
LONGEST_MATCHand including"match_tpl.h", thelongest_match_candlongest_match_slow_cfunctions are generated efficiently. This approach promotes code reuse and consistency across different match implementations.arch/generic/generic_functions.h (1)
52-54: Updatenative_*macros to use_cimplementationsThe macros
native_longest_match,native_longest_match_slow, andnative_compare256have been updated to refer to the_cimplementations. This aligns with the removal of unaligned functions and ensures that the generic C versions are used when runtime CPU detection is disabled.arch/generic/chunkset_c.c (1)
6-6: Unrelated change: New dependency added.This change appears unrelated to the PR's objective of disabling CRC32-VX for problematic Clang versions. Consider submitting memory operation optimizations as a separate PR.
arch/s390/Makefile.in (1)
35-40: Unrelated change: New DFLTCC targets added.The addition of dfltcc_common targets appears unrelated to the PR's objective of disabling CRC32-VX for problematic Clang versions. Consider moving these changes to a separate PR focused on DFLTCC support.
arch/power/chunkset_power8.c (1)
Line range hint
1-1: Critical files missing from review.None of the provided files contain changes related to the PR's objective of disabling CRC32-VX for problematic Clang versions. Instead, all changes focus on memory operation optimizations. Please provide the files containing the CRC32-VX disabling logic for review, particularly any modifications to compiler version checks or CRC32-VX implementation.
Additionally, consider moving the memory operation optimization changes to a separate PR to maintain focus and facilitate easier review of both changes.
arch/riscv/compare256_rvv.c (1)
10-10: LGTM: Header change looks goodThe change from
zutil_p.htozmemory.haligns with the codebase's move towards using memory functions fromzmemory.h.test/benchmarks/benchmark_compare256_rle.cc (1)
62-68: LGTM: Benchmark changes align with implementationThe benchmark updates correctly reflect the new bit-width specific functions and properly handle CPU capability checks.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 62-62: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
arch/arm/chunkset_neon.c (2)
35-35: LGTM! Improved memory reading operations.The replacement of memcpy with specialized zng_memread_* functions is a good optimization that could improve performance by reducing function call overhead.
Also applies to: 39-39, 43-43
8-8: Verify thread safety of zmemory.h functions.The new header introduces specialized memory reading functions. Ensure these functions are thread-safe and handle unaligned memory access correctly on ARM architecture.
✅ Verification successful
Memory functions are thread-safe and handle unaligned access correctly ✅
The
zng_memread_*functions are inherently thread-safe as they:
- Perform read-only operations without modifying any shared state
- Are implemented as static inline functions
Unaligned memory access is properly handled through:
__packed__and__may_alias__attributes when supported- Fallback implementation when these attributes aren't available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if zmemory.h functions have proper alignment checks rg -A 5 "zng_memread_[248]" "$(fd -t f zmemory.h)" # Look for any thread safety mechanisms rg -i "thread|atomic|mutex|lock" "$(fd -t f zmemory.h)"Length of output: 1236
arch/generic/Makefile.in (1)
43-46: LGTM! Dependencies updated correctly.The dependencies for compare256_c targets have been properly updated to include the new headers.
deflate_rle.c (2)
Line range hint
1-1: Verify alignment with PR objectives.The changes in these files focus on memory operations and build system updates, but don't appear to address the PR's primary objective of disabling CRC32-VX for specific Clang versions (18.0.0 to 19.1.2). Please clarify if these changes are part of a different PR or if additional files need to be reviewed.
13-20: Verify performance impact of the new default comparison function.The default comparison function has been changed from compare256_rle_c to compare256_rle_16. While this might improve performance on some architectures, we should verify there's no regression on others.
✅ Verification successful
The performance impact of the new default comparison function is properly managed
The change is safe because:
- Runtime CPU detection automatically selects the optimal variant (64, 32, 16, or 8-bit) based on available CPU features
- The 16-bit fallback is a safe default that works across all architectures
- Comprehensive benchmark infrastructure exists to verify performance across different implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for any performance benchmarks or tests rg -l "compare256_rle_(8|16|32|64)" | xargs rg -l "benchmark|test|perf" # Check if there are architecture-specific optimizations fd -t f "compare256.*\.(c|h)" -x rg -l "arch|cpu|processor"Length of output: 275
Script:
#!/bin/bash # Check benchmark implementation echo "=== Benchmark Implementation ===" cat test/benchmarks/benchmark_compare256_rle.cc # Look for performance documentation echo -e "\n=== Performance Documentation ===" fd -e md -e txt | xargs rg -l "performance|benchmark|compare256" # Check architecture notes echo -e "\n=== Architecture Notes ===" rg -i "architecture|processor|cpu" README.md docs/ 2>/dev/null || trueLength of output: 2690
Script:
#!/bin/bash # Check benchmark documentation echo "=== Benchmark README ===" cat test/benchmarks/README.md # Check architecture-specific docs echo -e "\n=== Architecture Docs ===" cat arch/s390/README.md # Check CPU detection configuration echo -e "\n=== CPU Detection Config ===" rg "CPU|HAVE_BUILTIN_CTZ" CMakeLists.txtLength of output: 14711
arch/x86/chunkset_avx2.c (1)
23-23: Improve memory operations using specialized functions.The changes replace
memcpywith specialized memory read functions (zng_memread_*), which can potentially improve performance by using optimized memory operations.Also applies to: 27-27, 31-31
arch/riscv/adler32_rvv.c (1)
103-105: Prevent overflow in Adler-32 checksum calculations.The changes add modulo operations to ensure that
sum2_sum,adler_sum,sum2, andadlerremain within the bounds defined byBASE. This is a critical fix to prevent overflow and maintain the integrity of the checksum calculation.Also applies to: 109-115
cmake/detect-sanitizer.cmake (1)
114-114: Add alignment sanitizer check.The addition of the
alignmentsanitizer check enhances the detection of unaligned memory accesses, which can help identify potential performance and correctness issues.arch/x86/chunkset_avx512.c (3)
37-37: Improve memory operations using specialized functions.The changes replace
memcpywith specialized memory read functions (zng_memread_*), which can potentially improve performance by using optimized memory operations.Also applies to: 41-41, 45-45
64-71: Improve remainder handling in CHUNKCOPY.The changes enhance the handling of remaining bytes when the length is less than the size of a chunk:
- Check if length is less than chunk size and use masked load/store.
- Handle remainder of zero by defaulting to chunk size.
Also applies to: 74-75
149-158: Simplify remainder handling in HALFCHUNKCOPY.The changes streamline the handling of remaining bytes by:
- Defaulting remainder of zero to half chunk size.
- Using masked load/store operations.
inflate_p.h (2)
142-142: LGTM! Memory read operation improved.Replacing
memcpywithzng_memread_8is a good optimization for reading 64-bit values.
178-181: Address the TODO comment about memory alignment.The comment indicates that
memcpycalls aren't inlined on architectures with strict memory alignment. This could impact performance.Let's verify which architectures are affected:
✅ Verification successful
The TODO comment about memory alignment is informative and should be retained.
The comment accurately describes a hardware limitation that affects certain architectures. The codebase already implements appropriate architecture-specific optimizations to handle alignment requirements. Removing this comment would lose valuable information for future maintainers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find architectures with strict memory alignment requirements. # Search for alignment-related code or comments rg -i "strict.*align|align.*strict|memcpy.*align|align.*memcpy" # Search for architecture-specific alignment handling rg -i "align" "arch/"Length of output: 7925
arch/s390/dfltcc_detail.h (2)
2-2: LGTM! Include added for runtime tuning parameters.The include directive provides access to the newly defined environment variables.
206-207: LGTM! Environment variables used for configuration.Good changes:
- Using
env_dfltcc_source_date_epochto control DFLTCC_CMPR for reproducible builds- Using
env_dfltcc_ribminstead of hardcoded valueAlso applies to: 214-214
match_tpl.h (3)
25-27: LGTM! Documentation improved.Good addition of documentation explaining the behavior of the LONGEST_MATCH_SLOW variant.
46-47: LGTM! Memory handling improved.Good changes:
- Using 64-bit variables instead of byte arrays
- Using
zng_memread_8for efficient memory readsAlso applies to: 69-70
132-133: LGTM! Memory comparison operations updated.Memory comparisons correctly updated to work with the new 64-bit variables.
Also applies to: 139-140, 146-147, 173-173
functable.c (2)
64-66: LGTM: Improved implementation selectionThe change from generic to C-specific implementations aligns with the codebase's optimization strategy.
113-117: Excellent enhancement to AVX2 conditionThe addition of BMI2 check alongside AVX2 is well-documented and provides clear performance benefits through:
- Flagless shifts reducing pipeline stalls
- Better register allocation
- Reduced CL register contention
win32/Makefile.arm (1)
207-207: LGTM: Updated header dependenciesThe change from zutil_p.h to zmemory.h aligns with the project's move towards specialized memory functions.
Also applies to: 218-218
win32/Makefile.a64 (1)
186-186: LGTM: Consistent header dependency updatesThe changes mirror those in Makefile.arm, maintaining consistency across different architecture builds.
Also applies to: 197-197
chunkset_tpl.h (2)
230-232: LGTM: Optimized memory alignment handlingThe changes maintain the careful balance between performance and safety in critical memory operations, aligning with the established pattern of avoiding unnecessary bounds checks while ensuring safety.
Also applies to: 238-238
Line range hint
1-1: Verify implementation of PR objectiveWhile the code changes look good, I don't see the implementation of the main PR objective: disabling CRC32-VX for Clang versions 18.0.0 to 19.1.2. Could you point me to the file containing these changes?
✅ Verification successful
Found implementation of Clang version checks for CRC32-VX
The implementation is present in
arch/s390/s390_functions.h. It correctly blocks Clang versions from 18.0.0 to 19.1.1 by raising a compiler error with clear guidance to either disable CRC32-VX or use a different compiler.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Clang version checks or CRC32-VX related code echo "Searching for Clang version checks..." rg -l "clang|CRC32.*VX" --type cpp --type c --type h echo "Searching for S390-specific code..." rg -l "s390|vgfma|CRC32.*VX" --type cpp --type c --type hLength of output: 965
Script:
#!/bin/bash # Check s390-specific files for Clang version checks echo "=== Checking arch/s390/crc32-vx.c ===" rg "clang|__clang__|CLANG" arch/s390/crc32-vx.c -C 5 echo -e "\n=== Checking arch/s390/s390_functions.h ===" rg "clang|__clang__|CLANG" arch/s390/s390_functions.h -C 5 echo -e "\n=== Checking arch/s390/s390_features.h ===" rg "clang|__clang__|CLANG" arch/s390/s390_features.h -C 5 echo -e "\n=== Checking for version checks in header files ===" rg "__clang_major__|__clang_minor__|__clang_patchlevel__" --type h -C 5Length of output: 1966
zbuild.h (2)
206-208: Address the TODO comment about ALIGNED_ macro.The TODO comment indicates missing ALIGNED_ macro definitions for other compilers. This could impact portability.
Would you like me to help implement the ALIGNED_ macro for additional compilers?
249-284: LGTM! Well-structured architecture-specific optimizations.The OPTIMAL_CMP values are well documented and properly configured for different architectures, taking into account unaligned access capabilities.
win32/Makefile.msc (1)
215-217: LGTM! Consistent dependency updates.The object file dependencies have been properly updated to include zmemory.h.
Also applies to: 229-229
test/test_crc32.cc (2)
200-214: LGTM! Well-structured alignment test class.The test class properly handles memory allocation and cleanup for alignment testing. The retrieved learning confirms that the alignment offset parameter is always non-negative.
235-250: LGTM! Comprehensive alignment test coverage.The test suite provides good coverage with various alignment offsets and proper test macro definition.
arch/s390/dfltcc_deflate.c (1)
31-35: LGTM! Enhanced configuration flexibility.The changes properly integrate environment variable configuration and add a runtime disable option.
Also applies to: 65-67
deflate.h (2)
15-15: LGTM: Include added for new memory functions.The addition of zmemory.h is appropriate for the new memory writing functions.
359-360: LGTM: Memory operations refactored to use specialized functions.The replacement of memcpy with zng_memwrite_* functions is consistent across all put_* functions. This change aligns with the broader initiative to improve memory handling throughout the zlib-ng library.
Also applies to: 371-372, 383-384, 395-396, 407-408
configure (6)
103-103: LGTM: New variable initialization.The builddfltccenv variable is properly initialized to 0.
177-177: LGTM: New command line option documentation.The help text for --with-dfltcc-env is clear and consistent with other option descriptions.
211-211: LGTM: Command line option handling.The --with-dfltcc-env option is properly handled, setting builddfltccenv to 1.
773-789: LGTM: secure_getenv availability check.The check for secure_getenv is well-implemented with proper compiler flags being set based on availability.
1894-1903: LGTM: DFLTCC environment support.The DFLTCC environment support is properly conditioned on builddfltccenv flag.
Line range hint
1-2000: Verify the CRC32-VX disabling logic.The core changes mentioned in the PR description about disabling CRC32-VX for Clang versions 18.0.0 to 19.1.1 are not visible in the provided code segments. Let's verify their existence:
✅ Verification successful
CRC32-VX disabling for Clang 18-19.1.1 implemented correctly
The implementation properly detects and prevents using CRC32-VX with affected Clang versions (18.0.0 through 19.1.1) by:
- Compile-time version check using Clang version macros
- Clear error message explaining the issue and providing workarounds
- Option to disable via --without-crc32-vx configure flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for CRC32-VX related changes in the codebase # Search for CRC32-VX related code echo "Searching for CRC32-VX related code..." rg -l "CRC32.VX|crc32.vx|buildcrc32vx" # Search for Clang version checks echo -e "\nSearching for Clang version checks..." rg "clang.*version.*18|clang.*version.*19"Length of output: 518
Script:
#!/bin/bash # Check the implementation of Clang version check and CRC32-VX disabling logic # Show the relevant section from s390_functions.h echo "=== Checking s390_functions.h for version check implementation ===" rg -B 5 -A 10 "clang.*version.*18" arch/s390/s390_functions.h # Check if configure script has any related changes echo -e "\n=== Checking configure for buildcrc32vx related logic ===" rg -B 2 -A 2 "buildcrc32vx" configureLength of output: 1059
Script:
#!/bin/bash # Get complete implementation of CRC32-VX version check # Use ast-grep to find preprocessor directives around version check echo "=== Checking complete preprocessor context in s390_functions.h ===" ast-grep --pattern '#if$$$clang$$$version$$$' arch/s390/s390_functions.h # Get more context around the version check using grep echo -e "\n=== Getting extended context with grep ===" grep -A 20 -B 20 "clang.*version.*18" arch/s390/s390_functions.hLength of output: 385
Script:
#!/bin/bash # Try different approaches to find the version check implementation # Show the complete s390_functions.h file echo "=== Content of s390_functions.h ===" cat arch/s390/s390_functions.h # Look for any clang version related checks echo -e "\n=== Searching for clang version checks with different patterns ===" rg -B 5 -A 5 "__clang|clang_version|__clang_major__|CLANG" arch/s390/s390_functions.hLength of output: 1789
zlib.h.in (1)
52-56: Version bump looks good.The version numbers have been consistently updated from 2.2.2 to 2.2.3 across all version-related macros. This patch version increment is appropriate for a bug fix release that addresses the Clang CRC32-VX issue.
test/add-subdirectory-project/CMakeLists.txt (1)
1-1: CMake version range update looks good.The change to specify a version range (3.5.1...3.29.0) follows modern CMake best practices while maintaining backward compatibility.
.gitignore (1)
30-32: New .gitignore entries look good.The added entries appropriately ignore generated files and build artifacts, following the existing patterns in the file.
Also applies to: 67-67, 79-82
arch/s390/dfltcc_inflate.c (2)
16-16: LGTM!The inclusion of
dfltcc_common.his necessary to access theenv_dfltcc_disabledvariable.
34-36: LGTM!The early check for
env_dfltcc_disabledis thread-safe as the variable is initialized once during library construction and remains constant afterward..github/workflows/release.yml (1)
41-41: LGTM!The addition of explicit Windows SDK version (10.0.22621.0) for ARM builds improves build reproducibility.
Also applies to: 47-47
arch/s390/self-hosted-builder/actions-runner.Dockerfile (4)
6-6: LGTM!Added
sedpackage which is required for the new patch application process.
11-11: LGTM!Updated to .NET SDK 8.0 to support the latest actions-runner.
18-20: LGTM!Improved patch application process:
- Downloads patch from a remote repository
- Uses
sedfor version string modification
41-42: LGTM!Added
chmod=555to ensure correct executable permissions for the runner scripts.arch/s390/README.md (2)
229-232: LGTM!Clear documentation of GitHub PAT requirements with a link to the official documentation.
🧰 Tools
🪛 Markdownlint (0.37.0)
229-229: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
232-232: null
Bare URL used(MD034, no-bare-urls)
256-259: LGTM!Simplified container rebuild process with automated weekly updates.
Also applies to: 262-264
🧰 Tools
🪛 Markdownlint (0.37.0)
257-257: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
257-257: Expected: indented; Actual: fenced
Code block style(MD046, code-block-style)
README.md (1)
28-28: LGTM!Added "Safe" qualifier to better describe the unaligned memory operations feature.
.github/workflows/cmake.yml (1)
84-88: Verify if this change is related to the PR objectives.The removal of
-DWITH_UNALIGNED=OFFfrom the UBSAN job configuration appears unrelated to disabling CRC32-VX for Clang versions. Please clarify if this is an intentional cleanup or if it should be part of a separate PR.CMakeLists.txt (5)
118-120: LGTM: DFLTCC environment variable support.The changes add proper support for DFLTCC environment variable parsing, including:
- New cmake option WITH_DFLTCC_ENV
- Dependency on WITH_DFLTCC_DEFLATE or WITH_DFLTCC_INFLATE
- Integration with the build system
Also applies to: 142-142, 883-885, 1420-1420
Line range hint
308-327: LGTM: Improved MSVC AVX detection.Added proper AVX detection for MSVC, which helps ensure correct SSE/AVX feature detection and definition of corresponding macros.
438-444: LGTM: Added secure_getenv detection.Added proper detection of secure_getenv function, which is important for securely accessing environment variables.
Line range hint
505-518: LGTM: Added ZNOLTOFLAG to compiler checks.Added ZNOLTOFLAG to compiler feature detection checks to ensure consistent behavior when LTO is disabled.
Also applies to: 523-536, 541-554
Line range hint
1-1443: Implementation for disabling CRC32-VX in specific Clang versions is missing.The PR's main objective is to disable the CRC32-VX implementation for Clang versions 18.0.0 to 19.1.2 due to incorrect code generation with IBM S390 VGFMA intrinsics. However, I don't see any changes in the CMake files to implement this functionality. This should be implemented by:
- Adding version detection for Clang
- Disabling CRC32-VX when the problematic versions are detected
- Optionally, adding a warning message during configuration
Run this script to check if the implementation exists in other files:
✅ Verification successful
Implementation for disabling CRC32-VX in specific Clang versions is correctly implemented.
The implementation is found in
arch/s390/s390_functions.hand correctly disables CRC32-VX for Clang versions 18.0.0 to 19.1.2 by generating a compile-time error with a descriptive message. The implementation is complete and properly integrated with the CRC32-VX functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for files containing CRC32-VX or Clang version checks # Look for CRC32-VX related code echo "=== Files containing CRC32-VX related code ===" rg -l "CRC32.?VX|crc32.?vx" # Look for Clang version checks echo -e "\n=== Files containing Clang version checks ===" rg -l "clang.*version|__clang_major__|__clang_minor__|__clang_patchlevel__"Length of output: 532
Script:
#!/bin/bash # Description: Check the implementation in s390_functions.h echo "=== Content of arch/s390/s390_functions.h ===" cat arch/s390/s390_functions.h echo -e "\n=== Relevant sections with Clang and CRC32-VX ===" rg -A 5 -B 5 "(__clang__|CRC32.?VX)" arch/s390/s390_functions.hLength of output: 1992
test/test_compare256.cc (2)
67-73: LGTM! Well-structured test additions.The new test cases are properly organized with appropriate conditional compilation guards and maintain consistency with the existing test pattern.
67-73: LGTM: Well-structured test additions.The new test cases for different word sizes with proper conditional compilation improve test coverage and handle CPU feature dependencies correctly.
arch/x86/chunkset_ssse3.c (2)
6-6: LGTM! Improved memory handling.Good use of specialized memory functions from zmemory.h for safer and more consistent memory access patterns.
Also applies to: 37-37, 41-41, 45-45
6-6: LGTM: Improved memory access safety.The use of
zng_memread_*functions fromzmemory.himproves memory access safety and consistency across the codebase.Also applies to: 37-37, 41-41, 45-45
test/benchmarks/benchmark_compare256.cc (3)
68-74: LGTM! Well-structured benchmark additions.The new benchmarks are properly organized with appropriate conditional compilation guards and maintain consistency with the existing benchmark pattern.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 68-68: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
Line range hint
1-1: Core changes for PR objective are missing.The provided files don't contain the changes needed to disable the CRC32-VX implementation for Clang versions 18 to 19.1.2. Please provide the changes that implement this functionality, which should likely be in the s390-related source files.
Let me help verify which files might need to be modified:
68-74: LGTM: Consistent benchmark additions.The benchmark additions align well with the test changes, maintaining proper conditional compilation based on CPU features.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 68-68: There is an unknown macro here somewhere. Configuration is required. If BENCHMARK_DEFINE_F is a macro then please configure it.
(unknownMacro)
zmemory.h (1)
1-99: Well-structured implementation of optimized memory operations!The implementation provides a robust set of memory operations with:
- Optimized path using
__may_alias__attribute for unaligned access- Safe fallback to
memcpywhen optimizations aren't available- Consistent naming and thorough documentation
insert_string_tpl.h (1)
34-39: Good use of optimized memory operations!The replacement of
memcpywithzng_memread_4is appropriate and maintains correct byte order handling.compare256_rle.h (1)
Line range hint
46-132: Good optimization of memory comparisons!The implementation effectively uses the new memory operations and properly handles byte order differences. The function names now better reflect their purpose.
arch/generic/compare256_p.h (1)
1-9: Well-organized implementation with progressive optimizations!The file provides a clear progression of optimizations from basic 8-bit comparisons to advanced 64-bit operations, with proper conditional compilation.
arch/arm/crc32_acle.c (1)
14-16: Review variable type changes.The change from pointer types to value types makes the memory access pattern more explicit. However, ensure proper alignment requirements are met for direct memory access.
Let's verify the alignment requirements in the ARM architecture documentation:
✅ Verification successful
The variable type changes are safe and well-implemented
The implementation includes proper alignment checks before memory access and follows ARM's memory access patterns with progressive processing from 64-bit down to 8-bit chunks. The change from pointer types to value types is an optimization that maintains correctness through explicit alignment handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for alignment requirements in ARM documentation rg "alignment|unaligned" --type c --type h arch/arm/Length of output: 176
Script:
#!/bin/bash # Search for CRC32 implementation details and usage of these variables rg -A 5 "buf[248]" arch/arm/crc32_acle.c # Also check for any ARM CRC32 instruction related comments or defines rg "crc32|CRC32" arch/arm/Length of output: 3722
|
This tested the code nicely. Seems like quotation is not needed for the preprocessor error directive, my suggestion for a perhaps clearer error: What do you think? |
|
yes that looks indeed better! thx! |
We have to disable the CRC32-VX implementation for some Clang versions (18 <= version < 19.1.2) that generate bad code for the IBM S390 VGFMA intrinsics.
We have to disable the CRC32-VX implementation for some Clang versions (18 <= version < 19.1.2) that generate bad code for the IBM S390 VGFMA intrinsics.
This fixes #1845
Question: should we print a warning during configure, if the intrinsics are not used. Or is it OK to just silently disable them?
Summary by CodeRabbit