feat(build): support Linux aarch64 builds - #181
Conversation
38606a3 to
2b2e6be
Compare
zjw1111
left a comment
There was a problem hiding this comment.
Thanks for taking on the aarch64 support work — the architecture handling is much cleaner after this. One question about the ppc handling that this refactor carries over.
1471c31 to
c4deec5
Compare
The architecture-specific build logic keyed on PAIMON_CPU_FLAG and PAIMON_ARMV8_ARCH, neither of which was ever defined, so aarch64 got no -march= at all. TargetArchitecture.cmake now resolves the target architecture once, and PAIMON_AARCH64_MARCH selects the -march= value (default armv8-a; an empty value passes no -march flag). Fixing the target-dependent behavior this exposed changes one result on x86-64 as well: out of range float to integer conversion now follows Java. - Remove dead SIMD build logic: the SSE4.2 CRC32C kernel (Castagnoli, never selected, not the persisted zlib CRC-32, so no on-disk value changes), the ppc AltiVec probe, and the unread PAIMON_HAVE_NEON / ARMV8_CRC / ARMV8_CRYPTO definitions. crc32c_test.cpp now pins the checksum to zlib.crc32 reference values. - Name the package after the host platform, e.g. paimon-cpp-linux-aarch64.tar.gz; add --platform and --print-name to build_and_package.sh. Lumina is prebuilt for linux-x86_64 only and now fails configuration elsewhere with an explicit error. - Run the same CI matrix on x86_64 and aarch64 (clang-tidy excepted), plus a script-tests job for the new CMake-module, packaging-argument and asan_symbolize tests; all jobs are required checks. - Fix char-signedness defects the unsigned-char AArch64 Linux ABI exposed: BinaryString UTF-8 leading-byte classification (NumChars, Substring and IndexOf counted bytes, not characters), tolower and toupper on bytes at or above 0x80, TINYINT min/max aggregation (min(-20, 10) returned 10) and VariantValueToString (-20 printed as 236). - Replace the undefined float/double to integer static_casts, in the cast executor and in the Arrow kernel it called, with JavaFloatingToIntegerCast: NaN to 0, out of range saturating at the int32/int64 bounds, then narrowing to width. Both the literal and the array path use it so stats and data stay consistent. - Link compiler-rt's builtins into the Clang UBSan build: the 128-bit multiplication overflow check calls __muloti4, which libgcc does not provide and Clang does not inline on aarch64, so the sanitizer build did not link there. - Pin asan_symbolize.py stdin/stdout to UTF-8 with surrogateescape so a byte that is not valid UTF-8 no longer truncates the test log. - docs: a supported platform matrix, rules on char signedness and on float to integer conversion, and Java-consistent results in the type change support matrix.
|
@u70b3 Sorry for not pinging you during the review stage of this PR — that was an oversight on my side. This PR has already been merged. Whenever you have some time, could you take a look and check whether the current fix meets your use case? If anything is still missing or does not work as expected on your side, please let us know and we will follow up. |
|
@zjw1111 No worries, and thanks for driving the aarch64 port! I took it for a proper spin on my local aarch64 machine (a 128-core Kunpeng-920): replicated the full CI aarch64 matrix locally — gcc debug/release, clang debug/release, ASan+UBSan, and TSan. Everything builds cleanly and the entire test suite passes (40/40). The port works well for my use case. 🎉 While validating, I also ran a deeper ARM portability sweep and found a few suspected/confirmed issues that #181 didn't cover, so I went ahead and fixed them locally:
Fixes plus deterministic TSan regression tests are in #203 — details and the red→green evidence are in the PR description. Happy to split the PR or adjust anything if you'd prefer a different shape! |
…portability An aarch64 portability audit (7-category static sweep + on-hardware validation on a 128-core Kunpeng-920) found one real concurrency bug hidden by x86 TSO and three latent UB/divergence risks: - Singleton<T>::GetInstance() published the instance with a plain store guarded only by a compiler-only MEMORY_BARRIER, and the fast path read it with a plain non-atomic load. On aarch64 this allows readers to observe a non-null pointer to a not-yet-constructed object. Use std::atomic<T*> with acquire/release ordering. - IOHook::Impl::mode_ was a plain enum raced by Reset() and Try(). Make it std::atomic<Mode>, stored before the seq_cst pos_/io_count_ stores so it is published together with them. - SerializationUtils::DeserializeBinaryRow read arity from a byte-filled buffer through reinterpret_cast<int32_t*> (strict-aliasing UB). Use memcpy like the serialize side; identical codegen. - CacheManager and SstFileWriter relied on undefined double->int conversions for extreme configs (x86-64 cvttsd2si yields the integer indefinite value, aarch64 fcvtzs saturates). Add common-layer SaturatingDoubleToInteger with the Java saturation policy and use it at both sites. FieldSumAgg INT8 sum/neg was audited and left unchanged: mod-256 addition and negation are invariant under plain-char signedness, so the stored bytes already match Java bit-for-bit on both ABIs. Unlike the min/max comparisons fixed in PR apache#181, signedness cannot change the result here. Tests, run on the Kunpeng-920 with PAIMON_USE_TSAN=ON for the races: - SingletonTest.TestConcurrentIOHookGetInstance storms the first publication of Singleton<IOHook>: TSan-red pre-fix (race in LazyInstantiation::Create), clean after. A FactoryCreator storm cannot gate this race because the REGISTER_PAIMON_FACTORY constructors in paimon_shared already initialize it before main(). - IOHookTest.TestConcurrentResetAndTry: TSan-red pre-fix (race on Impl::mode_), clean after. - SerializationUtilsTest gains a DataInputStream round-trip that also pins the big-endian wire format. - CacheManagerTest locks the saturated capacity semantics. Pre-fix, x86-64 cvttsd2si yields INT64_MIN (red there), while aarch64 fcvtzs saturates natively (green either way, verified on the Kunpeng-920). - SaturatingCastTest covers the helper's boundaries directly, including the int32_t path used by SstFileWriter. Generated-by: Claude Code (claude-opus-4-8)
Purpose
Linked issue: close #171
Paimon C++ only built and was validated on x86_64: the architecture-specific build logic keyed on
PAIMON_CPU_FLAGandPAIMON_ARMV8_ARCH, neither of which was ever defined, so the armv8 tuning branch was dead code and Arm64 got no-march=flag at all. This change resolves the target architecture once and keys every architecture-specific decision on it. Running the suite onubuntu-24.04-armthen exposed two classes of target-dependent behavior — plain-charsignedness, which the AArch64 Linux ABI decides differently, and undefined float-to-integer conversion — plus a gap in how a failure is reported.Build system and CI:
cmake_modules/TargetArchitecture.cmakederivesPAIMON_TARGET_PROCESSORandPAIMON_TARGET_CPU_FAMILYfromCMAKE_SYSTEM_PROCESSOR, with no compiler probe socmake -Pcan test it. An unrecognized processor gets no tuning flags rather than a configure error.PAIMON_AARCH64_MARCHselects the Arm64-march=value, defaulting toarmv8-a; an explicitly empty value passes no-marchflag at all. A fallback coversadd_subdirectory()consumers, for which the option does not exist.PAIMON_SIMD_LEVEL, so no build ever selected it — no on-disk value changes. The ppc AltiVec probe and the never-readPAIMON_HAVE_NEON/PAIMON_HAVE_ARMV8_CRC/PAIMON_HAVE_ARMV8_CRYPTOdefinitions go with it.linux-x86_64only: configuring it elsewhere now fails with an explicit error, and CI disables it off x86_64.build_and_package.shnames the artifact after the platform, e.g.output/paimon-cpp-linux-x86_64.tar.gz.--platformoverrides the label alone (cross compilation is not wired up end to end);--print-nameprints the resolved name and exits.gcc-debug-x86_64,gcc-debug-aarch64, …), ascript-testsjob is added, and.asf.yamlis renamed and extended in lockstep so all jobs stay required checks. Release verification gains an aarch64 entry in its own workflow.Defects the aarch64 run exposed:
BinaryString::NumBytesForFirstByteclassified a UTF-8 leading byte by its sign. Withcharunsigned on aarch64, every byte was taken for single-byte ASCII, soNumChars,SubstringandIndexOfcounted bytes instead of characters —NumChars("Paimon中文社区")returned 18 rather than 10. The byte is now classified asuint8_t, branch for branch equivalent to the signed form on x86-64, so x86-64 results are unchanged.ToUpperCaseandToLowerCasepassed bytes totoupper/toloweras plainchar— undefined behavior for bytes at or above 0x80 wherecharis signed, x86-64 included. They now go throughunsigned char, on the fast path and in theCppTo*Casefallbacks, and a stray pre-looptolowerwrite inToUpperCaseis removed. Intended results do not change.char, so on aarch64min(-20, 10)returned 10. They compare the signed value now, as doesVariantValueToString, which printed 236 for -20.__muloti4, which libgcc does not provide and Clang inlines on x86-64 but not on aarch64. The Clang UBSan build now links compiler-rt's builtins archive.NumericPrimitiveCastExecutorand in the Arrow kernel it delegated to.JavaFloatingToIntegerCastnow defines it as Java does —NaNbecomes 0, out-of-range saturates at the int32 bounds (int64 for BIGINT), then narrows to the target width by keeping the low bits — on both the literal and the array path, so stats and data stay consistent. This changes results on x86-64, where those conversions previously produced whatever the hardware did.asan_symbolize.pytruncated the failure report: it decoded stdin withsurrogateescapebut re-opened stdout strictly, so a byte that is not valid UTF-8 broke the pipe — which is why the first aarch64 run reported only a single failure. Both streams now usesurrogateescape.Tests
New script-mode tests, run by the
script-testsCI job without a toolchain or a configured build tree:ci/scripts/test_cmake_modules.sh— processor-string mapping inTargetArchitecture.cmake, and thePAIMON_AARCH64_MARCHdefault, override andadd_subdirectory()fallback.ci/scripts/test_packaging_args.sh—build_and_package.shargument parsing, the default platform label,--platformvalidation,--print-nameand the resulting archive name.ci/scripts/test_asan_symbolize.sh— invalid UTF-8 bytes round-trip unchanged and symbolization continues past them; each case was verified to fail against the unfixed script.Updated unit tests (target
unittest):crc32c_test.cpppins the checksum to zlib CRC-32 reference values rather than to whichever kernel was compiled in.cast_executor_test.cppcovers all eight float/double to integer pairs with the same twelve inputs and expected Java results on both the literal and the array path, includingMAX/LOWEST/±INFINITY/NaN, plus the empty-array, sliced-offset and null edges of the conversion that replaced Arrow's kernel.field_min_max_agg_test.cppanddata_define_test.cppcover the negative TINYINT; two utils tests now read it back throughint8_t.binary_string_test.cppenables the four-byte UTF-8 case asu8"\U0001F919"(it was disabled as an ill-formed surrogate pair), adds the same code point toTestSubstringandTestIndexOf, and adds invalid UTF-8 bytes passing through case conversion unchanged. The existing multi-byte tests were the ones failing on aarch64; they are unchanged and pass there now.The
gcc-debug-aarch64andgcc-release-aarch64jobs run the full unit and integration suite onubuntu-24.04-arm, the end-to-end verification that the library builds and passes on aarch64 in both configurations.API and Format
No change to the public API under
include/, and no change to the storage format: the removed CRC32C kernel was never selected by any build, andBinaryString::NumBytesForFirstBytekeeps itscharparameter and its x86-64 results, returning those same values on aarch64 now.One behavior change: casting a float or double whose truncated value does not fit an integer type now follows Java Paimon on every architecture. It was undefined behavior before, so x86-64 results for those inputs change —
CAST(MAX_FLOAT AS TINYINT)yields -1 instead of 0,CAST(300.9 AS TINYINT)yields 44, andCAST(NaN AS INT)yields 0 instead ofINT_MIN. Values that already fit are unaffected.Build-facing changes for downstream users: a new
PAIMON_AARCH64_MARCHoption, the released archive is now namedpaimon-cpp-<platform>.tar.gzinstead ofpaimon-cpp.tar.gz(with--platform/--print-nameadded tobuild_and_package.sh), and configuring with lumina enabled offlinux-x86_64now fails explicitly instead of failing later at link time.Documentation
Yes.
docs/source/building.rstgains a supported platform matrix and documentsPAIMON_AARCH64_MARCHand the platform-labelled packaging.docs/source/user_guide/read.rstupdates the type change support matrix: float/double to integer casts are now well defined and Java-consistent on every supported architecture.docs/code-style.mdgains rules against relying on the signedness of plaincharand on undefined float-to-integer conversions, pointing atJavaFloatingToIntegerCastas the policy Paimon applies.Generative AI tooling
Generated-by: Claude Opus 5 (1M context)