u70b3 opened a new pull request, #203: URL: https://github.com/apache/paimon-cpp/pull/203
### Purpose Following up on the aarch64 port (#181), an ARM portability audit (7-category static sweep + on-hardware validation on a 128-core Kunpeng-920, ARMv8.2) found **one real concurrency bug currently hidden by x86 TSO** and four latent UB/divergence risks: 1. **`Singleton<T>` double-checked locking is broken on weak memory models** (`singleton.cpp:33`, `singleton.h:34`). The instance pointer was published with a plain store guarded only by a compiler-only `MEMORY_BARRIER` (empty asm, no fence instruction), and the fast path read it with a plain non-atomic load. On aarch64 a reader can observe a non-null pointer to a not-yet-constructed object. Evidence: TSan reports the race deterministically on the real library; a 128-thread litmus of the exact pattern observed 978 torn reads in 2M rounds on Kunpeng-920; disassembly shows zero `dmb/ldar/stlr` around publication. A torn `Singleton<FactoryCreator>` read exposes a half-built `std::map`; a torn `Singleton<IOHook>` read exposes a garbage `impl_` pointer. Fix: `std::atomic<T*>` with acquire fast-path load and release store after creation. `LazyInstantiation::Create(T*&)` keeps its public signature. 2. **`IOHook::Impl::mode_` data race** (`io_hook.cpp`) — plain enum raced by `Reset()` (test thread) and `Try()` (IO threads), TSan-confirmed. Impact limited to fault-injection tests (possible missed/spurious injection → flaky tests). Fix: `std::atomic<Mode>`, stored first in `Reset()` so the existing seq_cst stores publish it. 3. **`FieldSumAgg` INT8 on plain `char`** (`field_sum_agg.cpp`) — the same pattern #181 fixed in `field_min/max_agg.h`. Bit-identical to Java today (mod-256 wrap is signedness-independent), but any future widening/comparison of the accumulator diverges (byte 0xEC → 236 unsigned vs −20 signed). Fix: compute through `int8_t` like the siblings. 4. **Strict-aliasing UB** (`serialization_utils.h:81`) — arity read from a byte-filled buffer via `reinterpret_cast<int32_t*>`. Latent today (all `Bytes` are 64-aligned, UBSan-alignment clean). Fix: `memcpy` like the serialize side; identical `ldr w` codegen. 5. **Undefined double→int conversions** (`cache_manager.h:62`, `sst_file_writer.cpp:31`) — results diverge across architectures (x86 `cvttsd2si` yields INT_MIN, aarch64 `fcvtzs` saturates, constant folding yields poison) for pathological configs (~8 EiB cache / ~2 GB block size). Fix: new common-layer helper `SaturatingDoubleToInteger<T>` implementing the Java saturation policy that `docs/code-style.md` mandates and #181's `JavaFloatingToIntegerCast` established (common-layer twin to keep the common↛core layering rule). Out of scope (documented follow-ups): tightening option validation for `btree-index.block-size`/`cache-page-size`, and the unchecked int64→int32 narrowing in `CoreOptions::GetCachePageSize` (core_options.cpp:1775). ### Tests TDD-style; red states verified against the pre-fix code on the repo's TSan build: | Test | Pre-fix | Post-fix | |---|---|---| | `SingletonTest.TestConcurrentIOHookGetInstance` / `TestConcurrentFactoryCreatorGetInstance` (new, 32-thread barrier storms) | Deterministic TSan data-race report | TSan-clean, pass | | `IOHookTest.TestConcurrentResetAndTry` (new) | Deterministic TSan data-race report | TSan-clean, pass | | `FieldSumAggTest.TestInt8SignedSemantics` (new: 8 Java-semantics boundary cases) | Pass (characterization lock) | Pass | | `SerializationUtilsTest.TestDeserializeBinaryRowFromStream` (new: stream-path round-trip + BE wire-format pin) | Pass | Pass | | `CacheManagerTest` (new ×3: INT64_MAX saturation, 512/512 split, LRU eviction) | Fails on x86 pre-fix (`INT64_MIN` capacity); passes on aarch64 pre-fix | Pass everywhere | Validation on aarch64 (gcc 11 Debug + clang 14 TSan builds): `paimon-common-factories-test` 10/10, `paimon-common-test` 1432/1432, `paimon-core-test` 1653/1653, full `ctest` 40/40 (100%). `pre-commit run --files <changed>` all green (clang-format, cmake-format, cpplint, codespell); `git diff --check` clean. ### API and Format No public API or storage format change. `LazyInstantiation::Create(T*&)` signature is unchanged; `MEMORY_BARRIER` remains defined in `macros.h` (no other users remain in repo code). Wire formats (incl. the big-endian arity prefix) are untouched and pinned by tests. ### Documentation No documentation changes needed (code comments added at each fix site explain the ordering/saturation rationale). ### Generative AI tooling Generated-by: Claude Code (claude-opus-4-8) -- 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]
