Skip to content

[fix](be) Fix ARM64 correctness, alignment UB, and build compatibility - #66857

Open
u70b3 wants to merge 4 commits into
apache:masterfrom
u70b3:arm-risk-hardening
Open

[fix](be) Fix ARM64 correctness, alignment UB, and build compatibility#66857
u70b3 wants to merge 4 commits into
apache:masterfrom
u70b3:arm-risk-hardening

Conversation

@u70b3

@u70b3 u70b3 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:

Systematic analysis of the BE codebase for aarch64 (ARM64) hazards — x86-only code paths, misaligned-access UB, and weak-memory-model races — done on a 128-core ARM server (aarch64, Ubuntu 22.04). Several real bugs were found and reproduced, plus a class of latent UB that is fragile on ARM. This PR fixes the real bugs, hardens the latent ones with zero-cost changes, and adds regression tests. A final commit fixes what prevents master from compiling at all on a current aarch64 toolchain.

Real bugs (reproduced on aarch64):

  1. common/signal_handler.h — the crash-handler election CAS is non-atomic on aarch64. The fallback chain requires HAVE___SYNC_VAL_COMPARE_AND_SWAP (never defined by this CMake build) or x86 inline asm, so aarch64 lands in a naive read-check-write that "has a race condition" (per its own comment). FailureSignalHandler() uses it to elect the single thread that dumps crash state. Reproduced with a 128-thread gtest: 2–13 threads won the election per run (should be exactly 1), which means concurrent crashers corrupt each other's dumps or crash again inside the handler. Fixed with __atomic_compare_exchange_n (available on every supported arch).
  2. service/doris_main.cpp — AArch32 asm in the startup NEON check. vadd.i32 q8,q8,q8 does not assemble on AArch64 once __ARM_NEON__ is defined (Clang defines it on some ARM targets); reproduced: unknown mnemonic 'vadd.i32'. Now picks add v8.4s,... on __aarch64__ and keeps the AArch32 spelling on __arm__.
  3. util/bfd_parser.cpp — hardcoded bfd_set_default_target("elf64-x86-64") fails on aarch64 (verified: the x86 backend is not bundled in aarch64 libbfd), logging a spurious error at every BE start. Selects the target per architecture.
  4. core/column/columns_common.cpp — the SIMD path of count_bytes_in_filter was silently compiled out on aarch64. The guard #if defined(__SSE2__) || defined(__aarch64__) && defined(__POPCNT__) already parses as __SSE2__ || (__aarch64__ && __POPCNT__) (&& binds tighter), and ARM toolchains never define __POPCNT__ (sse2neon does not define __SSE2__ either), so aarch64 always took the scalar loop. Dropped the __POPCNT__ gate so the block is enabled unconditionally on __aarch64__ (same SSE2 intrinsics via sse2neon; results verified bit-identical by ColumnsCommonTest).
  5. thirdparty/build-thirdparty.sh[[ "${USE_AVX2}" -eq 0 ]] is true for USE_AVX2=ON (bash arithmetic evaluates "ON" to 0), silently disabling croaring AVX2 for users following the script's own help text. Now normalized case-insensitively: 0/OFF/FALSE/NO disable, 1/ON/TRUE/YES or empty/unset keep enabled (consistent with the CMake boolean semantics BE sees via -DUSE_AVX2=...), unknown values warn and keep the default.

Misaligned-access UB hardening (aarch64 scalar loads tolerate misalignment so nothing crashes today, but these are C++ UB, trip UBSan, and can miscompile under aggressive optimization; unaligned_load/memcpy compile to the same single load instruction — zero cost):
util/bit_packing.inline.h (parquet/RLE decode hot path), util/bitmap_intersect.h (serialize/deserialize), storage/types.h (get_cpp_type_value on packed rows), util/hash_util.hpp (crc_hash/crc_hash64/murmur_hash2_64), ngram_bloom_filter.cpp (init — also fixes a rounded-up tail over-read: it copied words * sizeof(uint64_t) bytes from a size-byte buffer, i.e. up to 7 bytes past the end for bf sizes not divisible by 8 such as 65535; it now copies exactly size bytes and keeps the last word's tail zeroed so contains() still matches query-side filters).

Weak-memory-model hardening (aarch64 is weakly ordered):

  • io/cache/block_file_cache_profile: the statistics counters are now a plain AtomicStatistics value member with the metrics hook registered in the constructor. The previous atomic_shared_ptr load on the hot read path was not lock-free (libstdc++ implements std::atomic<shared_ptr> with an internal lock pool; the project's libc++ fallback uses the legacy lock-table free functions), and the lazy-init publication race it guarded was unreachable — the singleton is fully constructed before instance() publishes it.
  • storage/olap_common.h VersionWithTime: update_ts is atomic and is stored before the release CAS that publishes a new version. A reader that acquire-loads version and then update_ts is therefore guaranteed a timestamp at least as new as the one stored for that version's publication; "new version + stale timestamp" cannot happen (the previous store-after-CAS order left that interleaving window open on every architecture, x86 included). The residual "old version + newer ts" combination only biases compaction toward the conservative max retention count. Covered by a deterministic multi-threaded regression test.
  • bucketed_aggregation_source_operator: relaxed load of the merge-target index (whose value is then dereferenced cross-thread) → acquire, matching the sibling use in the same file.
  • util/histogram.cpp: CAS retry loops get _mm_pause (maps to isb via sse2neon on aarch64) to avoid LL/SC contention storms on many-core ARM.

Build fixes (separate commit — master does not compile on aarch64 Ubuntu 22.04 with a current distro toolchain; needed to build/test any of the above):
clang-19 compat: -Wshadow kept non-fatal (newer clang flags namespace shadowing by protobuf-generated enums), std::powfstd::pow, std::formatfmt::format, [[maybe_unused]] for a function only used on the x86_64 unwind path, UT -Wno-deprecated-declarations (libstdc++ ≥ 12), a __res_nsearchres_nsearch forwarder in glibc-compatibility (glibc ≥ 2.34 exports it only as a non-default compat version, which breaks linking the prebuilt krb5 archive), and #define SNII_CRC32C_X86 0 on non-x86 in snii/encoding/crc32c.cpp (the BE_TEST-only TU used the macro in #if without defining it off-x86, so -Wundef -Werror broke the aarch64 doris_be_test link).

How verified (TDD on a 128-core aarch64 server)

  • The CAS race test fails before the fix (2–13 winners/128 threads) and passes after; same for the asm compile check and the bfd/bash behavior checks.
  • New unaligned-buffer tests pass with the hardening and serve as regression guards (also runnable under -fsanitize=alignment); the ngram exact-size test (bf sizes 65/67/100/511/65535, zero-slack buffers) is the ASan guard for the tail over-read.
  • VersionWithTimeTest (4 readers × 20k version publications) deterministically passes with the fixed publication order and has a real failure window under the old order.
  • SignalHandlerTest, NGramBloomFilterTest, BitPackingUnalignedTest, BitmapIntersectTest (incl. VecDateTimeValue keys), ColumnsCommonTest, HashUtilUnalignedTest (murmur + crc + crc64), VersionWithTimeTest: 16/16 pass on aarch64 (clang 19.1.7, -march=armv8-a+crc), full doris_be_test link included.
  • 251 existing unit tests across the touched areas (bit packing, bitmap, bloom filter, crc, hash util, histogram, columns): all pass.
  • Full FE+BE build with these changes succeeds (-Werror), single-node cluster smoke test (create/insert/filter/aggregate/md5) verified.

Known follow-ups (out of scope for this PR)

  • Narrow the global -Wno-error=shadow exemption: existing warnings (mostly protobuf-generated enum collisions) are numerous and deserve a scoped cleanup PR.
  • Run the new ngram exact-size test under an ASAN_UT build so the over-read guard actively trips on regression (functional semantics already verified in the normal build).
  • The bucketed_aggregation acquire load and histogram _mm_pause have no dedicated tests — memory-ordering and spin-hint behavior are not practically unit-testable; the comments there are the guard.
  • This PR spans several independent fault domains (ARM64 correctness / alignment UB / concurrency / build / croaring config); happy to split it into per-theme PRs if reviewers prefer that for review and backport.

Release note

Fix several aarch64 (ARM64) correctness and build issues in BE, including a non-atomic CAS in the crash signal handler, and harden hot paths against misaligned-access UB and weak-memory-ordering hazards.

Check List (For Author)

  • Test

    • Unit Test
    • Manual test (add detailed scripts or steps below)
      • Built FE+BE on aarch64 Ubuntu 22.04 (clang 19.1.7, -march=armv8-a+crc), started a single-node cluster, ran create/insert/select smoke SQL successfully. Unit tests: run-be-ut.sh --run --filter='SignalHandlerTest.*:NGramBloomFilterTest.*:BitPackingUnalignedTest.*:BitmapIntersectTest.*:ColumnsCommonTest.*:HashUtilUnalignedTest.*:VersionWithTimeTest.*' (16/16 pass).
  • Behavior changed:

    • Yes. No on-disk format, protocol, or hash-output change (the hardening only changes how the same bytes are loaded, and bloom-filter bits are byte-identical). Runtime behavior changes: the crash-handler election is now atomic on aarch64 (exactly one dumper), the aarch64 SIMD popcount path in count_bytes_in_filter is enabled (same results, faster), the bfd default target is selected per architecture (spurious BE-start error log on aarch64 is gone), and croaring now honors the documented USE_AVX2 switch consistently with BE's CMake.
  • Does this need documentation?

    • No.

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@u70b3
u70b3 force-pushed the arm-risk-hardening branch from a7a87ab to f55f0c8 Compare August 18, 2026 01:46
@u70b3
u70b3 marked this pull request as draft August 18, 2026 01:47
@u70b3 u70b3 changed the title [fix](be) Fix and harden aarch64 (ARM64) risk code paths [fix](be) Fix ARM64 correctness, alignment UB, and build compatibility Aug 18, 2026
@u70b3
u70b3 marked this pull request as ready for review August 18, 2026 06:45
@u70b3
u70b3 force-pushed the arm-risk-hardening branch 2 times, most recently from 0cc16b8 to 0dc7c83 Compare August 18, 2026 09:14
u70b3 added 4 commits August 19, 2026 11:42
- SignalHandlerTest: 128-thread race on the CAS used by the crash-handler
  thread election. On aarch64 (where the fallback chain ends in a naive
  non-atomic read-check-write) 2-13 of 128 threads win the election;
  exactly one must win.
- NGramBloomFilter / BitPacking / BitmapIntersect / HashUtil unaligned
  tests: exercise the code paths with deliberately misaligned buffers
  (offset 1..7) so the memcpy-based hardening is regression-guarded and
  UBSan-ready.
- ColumnsCommonTest: pin the signed '>' 0 semantics of
  count_bytes_in_filter (bytes 128..255 must not be counted) so the
  scalar and SIMD paths always agree.
Real bugs:
- common/signal_handler.h: the CAS used to elect the single crash-dumping
  thread fell back to a non-atomic read-check-write on aarch64
  (HAVE___SYNC_VAL_COMPARE_AND_SWAP is never defined by this CMake build
  and the inline-asm branch is x86-only). On a 128-core ARM server,
  2-13 of 128 racing threads won the election, corrupting crash dumps.
  Use __atomic_compare_exchange_n which exists on all supported arches.
- service/doris_main.cpp: the startup NEON self-check used AArch32 asm
  (vadd.i32 q8,...) which no aarch64 assembler accepts once __ARM_NEON__
  is defined (Clang targets); pick the spelling per architecture.
- util/bfd_parser.cpp: bfd_set_default_target hardcoded elf64-x86-64,
  which fails on aarch64 (the x86 backend is not bundled there) and
  logged a spurious error on every BE start; select per-arch target.
- core/column/columns_common.cpp: '|| &&' precedence typo silently
  compiled out the SIMD path of count_bytes_in_filter on aarch64.
- thirdparty/build-thirdparty.sh: [[ USE_AVX2 -eq 0 ]] is true for 'ON'
  (bash arithmetic), silently disabling croaring AVX2 for users who
  export USE_AVX2=ON; use a string compare.

Misaligned-access UB hardening (zero-cost unaligned_load/memcpy; aarch64
scalar loads tolerate misalignment but the casts are UB and break UBSan):
- util/bit_packing.inline.h UnpackValue (parquet/RLE decode hot path)
- util/bitmap_intersect.h serialize/deserialize helpers
- storage/types.h BaseFieldTypeTraits::get_cpp_type_value (packed rows)
- util/hash_util.hpp crc_hash/crc_hash64/murmur_hash2_64
- ngram_bloom_filter.cpp init (also fixes reserve-then-index misuse)

Weak-memory-ordering hardening for aarch64:
- io/cache/block_file_cache_profile: use atomic_shared_ptr for lock-free
  stats access (DCL on a plain shared_ptr can observe a torn or
  half-constructed object)
- storage/olap_common.h VersionWithTime: atomic update_ts and
  release/acquire CAS so readers never see new version + stale ts
- bucketed_aggregation_source_operator: acquire load for the cross-thread
  merge-target index (was relying on data-dependency ordering)
- util/histogram.cpp: _mm_pause backoff in CAS retry loops
- bitmap_value.h: suppress -Wshadow for BitmapDataType enum constants
  (they collide with protobuf-exported doris::BITMAP; clang >= 17 turns
  it into an -Werror failure)
- be/CMakeLists.txt: keep -Wshadow non-fatal on clang, and
  -Wno-deprecated-declarations for BE_TEST (libstdc++ >= 12 marks
  std::get_temporary_buffer deprecated, tripping existing tests)
- parquet_column_convert.h: std::powf -> std::pow (libstdc++ <cmath>
  does not declare powf in namespace std)
- function_string_misc.cpp: std::format -> fmt::format (libstdc++ 12
  has no <format>; fmt is the project convention)
- be_thread_stack_action.cpp: append_frame is only used on the x86_64
  unwind path; mark [[maybe_unused]] so aarch64 -Werror builds pass
- glibc-compatibility: add resolv_shim.c forwarding __res_nsearch to
  res_nsearch — glibc >= 2.34 exports the former only as a non-default
  compat version, breaking the link against the prebuilt krb5 archive
…consistency

Review round for the aarch64 hardening PR:

- olap_common.h: store update_ts BEFORE the release CAS that publishes a
  new version, so a reader can never observe "new version + stale
  timestamp" (the previous order left an interleaving window on any arch);
  document the exact invariant instead of overclaiming; add
  VersionWithTimeTest concurrency regression test.
- ngram_bloom_filter: init() copied words*8 rounded-up bytes from the
  caller buffer, over-reading up to 7 bytes for bf sizes not divisible by
  8 (e.g. 65535). Copy exactly size bytes and keep the tail zeroed so
  contains() matches query-side filters; add exact-size ASan guard test
  (65/67/100/511/65535). The old "reserve()-then-index" root-cause story
  was wrong: the constructor already sized the vector.
- block_file_cache_profile: replace the atomic_shared_ptr (not lock-free:
  libstdc++ lock pool / legacy free-function lock table) with an
  AtomicStatistics value member and register the metrics hook in the
  constructor. The lazy-init publication race it guarded was unreachable
  because the singleton is fully constructed before publication.
- build-thirdparty.sh: accept CMake boolean spellings for USE_AVX2
  (0/OFF/FALSE/NO, case-insensitive) so croaring agrees with BE's CMake
  when users set e.g. USE_AVX2=OFF; unknown values warn and keep default.
- glibc-compatibility: resolv_shim.c was compiled twice (globbed into the
  archive AND listed in the explicit OBJECT lib); REMOVE_ITEM it from the
  globbed sources.
- signal_handler.h / columns_common.cpp: fix stale and misleading comments
  (the naive CAS fallback is gone; the aarch64 SIMD guard was not
  "parenthesized" - the __POPCNT__ gate was dropped).
- tests: unaligned-buffer coverage for crc_hash and crc_hash64 (only
  murmur_hash2_64 had it); VecDateTimeValue misaligned round-trip for
  bitmap_intersect (the int32 test comment overclaimed); correct the int32
  test comment.
- snii/encoding/crc32c.cpp: define SNII_CRC32C_X86 as 0 on non-x86 so the
  BE_TEST-only TU compiles with -Wundef -Werror on aarch64. Pre-existing
  break from apache#66809 that blocked linking doris_be_test, unrelated to the
  review edits.

Verified on aarch64 (clang 19.1.7, -march=armv8-a+crc): full ninja
doris_be_test build clean; 16/16 tests pass across VersionWithTimeTest,
NGramBloomFilterTest, HashUtilUnalignedTest, BitmapIntersectTest,
SignalHandlerTest, ColumnsCommonTest, BitPackingUnalignedTest.
@u70b3
u70b3 force-pushed the arm-risk-hardening branch from 0dc7c83 to 49c43f0 Compare August 19, 2026 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants