github-actions[bot] commented on code in PR #67727:
URL: https://github.com/apache/doris/pull/67727#discussion_r3968279010
##########
thirdparty/build-thirdparty.sh:
##########
@@ -616,12 +616,28 @@ build_snappy() {
sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
fi
+ local snappy_cxx_flags="-O3"
+ case "$(uname -m)" in
Review Comment:
[P1] Tie this ISA-specific archive to the BE target
The installed `libsnappy.a` is now target-specific, but its lifetime is not
tied to the BE target. The published x86/ARM build images run this script with
the defaults, while `build.sh` reuses any complete third-party tree solely by
checking the last unrelated library. Consequently `build-for-release.sh
--noavx2` can compile BE without AVX2 but link this default AVX2 archive;
similarly, `ARM_MARCH=armv8-a` can reuse the default `+crc` archive. Upstream
Snappy 1.2.1 has reachable unconditional `_mm256_*` operations under `__AVX__`
and emits ARM CRC instructions when built with `+crc`. The BE startup check
cannot catch the mismatch because its AVX probes are compiled from the BE's own
target macros, and the ARM path does not probe CRC. These nominal lower-target
packages can therefore `SIGILL` on the CPUs they are intended to support.
Please keep the reusable archive at the baseline, add target-specific
variants/runtime dispatch, or fingerprint it and force selection/rebuild wheneve
r the BE target changes.
##########
thirdparty/build-thirdparty.sh:
##########
@@ -616,12 +616,28 @@ build_snappy() {
sed -i 's/-fno-rtti/-frtti/g' CMakeLists.txt
fi
+ local snappy_cxx_flags="-O3"
+ case "$(uname -m)" in
+ x86_64)
+ # Match the BE's SSE4.2 baseline and optional AVX2 target.
+ snappy_cxx_flags+=" -msse4.2"
+ case "${USE_AVX2:-ON}" in
Review Comment:
[P2] Normalize `USE_AVX2` before branching
This case does not use the same truth table as the BE's later `if(USE_AVX2)`
in CMake. CMake treats named boolean constants case-insensitively and also
treats `N`, `IGNORE`, `NOTFOUND`, and `*-NOTFOUND` as false, but values such as
`USE_AVX2=Off` fall through here and add `-mavx2` while BE omits it. That
creates an AVX2 Snappy archive inside an otherwise non-AVX2 fresh build and can
fault at runtime. Please normalize or reject the setting once, or implement the
exact CMake false set, so both build stages select the same target.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
