Repository navigation
ci: cover i386 and ClangCL for the AVX2-dispatch guard; drop dead _M_X64 - #144
Conversation
The AVX2 runtime-dispatch guard in src/hdr_histogram.c is the subject of two recently merged fixes (#142 clang-cl, #143 i386), yet CI exercised neither configuration: the matrix excludes 32-bit Linux and never selects the ClangCL toolset. Add two regression jobs that build exactly those configurations, in both of which HDR_HAS_AVX2_DISPATCH must be compiled out: - build-linux-i386: 32-bit GCC (-m32, gcc-multilib), HDR_LOG_REQUIRED=DISABLED so no 32-bit zlib is needed; the core suites still run hdr_value_at_percentile. - build-windows-clangcl: the ClangCL VS toolset (-T ClangCL) on x64. Also simplify the guard: _M_X64 is dead once _MSC_VER is excluded (only the MSVC family defines _M_X64, and clang-cl uses _M_X64 rather than __x86_64__), so gate on __x86_64__ alone and document why 32-bit / MSVC-ABI / ICC are out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…index type) The ClangCL CI job added in this PR surfaced a pre-existing clang-cl build break that PR #142 did not cover: _BitScanReverse{,64} take an 'unsigned long *' out-parameter, but the index was declared uint32_t. MSVC cl.exe tolerates this (same width), but clang-cl treats the pointer-type mismatch as a hard error (-Wincompatible-pointer-types is a default error in Clang 16+). Declare the index as 'unsigned long' to match the intrinsic; on Windows it is 32-bit, so behavior is unchanged. Fixes both the _WIN64 and 32-bit branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second clang-cl-only break surfaced by the new ClangCL CI job: on the MSVC
atomic path, hdr_atomic_{load,store}_pointer are typed 'void**' functions, so
passing 'struct hdr_histogram**' is a hard error under clang-cl
(-Wincompatible-pointer-types, default error in Clang 16+). GCC/Clang use a
type-generic __atomic_* macro, so this never showed on Linux/macOS. Cast the
call-site arguments to (void**), matching the existing idiom in
test/hdr_atomic_test.c. void* and object pointers share representation
(C11 6.2.5p28), so behavior is unchanged on all platforms.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-bit Adversarial review of the guard comment found a factual error: clang-cl defines __x86_64__ AND __clang__ (alongside _MSC_VER), so it is the !defined(_MSC_VER) clause -- not the arch/compiler check -- that keeps clang-cl out of the AVX2 dispatch. Correct the comment so a future editor does not drop _MSC_VER and reintroduce the #142 break. Verified with clang --target=x86_64-pc-windows-msvc -dM -E. Also harden the i386 job: assert the built test binary is 'ELF 32-bit' so the job fails loudly if -m32 is ever silently ignored (would otherwise be a false-positive regression test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
149bddd to
e2d1506
Compare
|
🤖 Automated first-pass review. A human maintainer still needs to review this before merge. Looks good overall. One thing CI can't check: the |
The negative assert used 'cc -m32 ... | grep -q ... && { exit 1; } || true'.
The trailing '|| true' greens the step even if 'cc -m32' itself breaks (e.g.
missing multilib): grep sees empty input, doesn't match, and the step passes
— the same false-positive class the 'ELF 32-bit' check was added to close.
Write the preprocessor macros to a file first so a broken 'cc -m32' fails the
step under 'set -e', then assert with an explicit if-guard. (A '! grep'
one-liner would not work: a pipeline preceded by '!' is exempted from 'set -e',
so a match would not fail the step.) Now fails on both a broken command and a
regressed guard.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # .github/workflows/ci.yml
|
Review round: 2026-09-29. Reviewed commit: Verdict: MERGE-READY for this portability/CI change. The AVX2 guard still limits dispatch to 64-bit x86 GCC/Clang, excludes clang-cl/MSVC and ICC classic, and retains the scalar fallback. The MSVC bit-scan output now has the API-required Fresh local arm64 release CTest 5/5, ASan+UBSan+float-cast-overflow 5/5 and logging-disabled 4/4 pass. The reviewed head's i386 and Windows ClangCL jobs, existing platform matrix, sanitizers and PR fuzz checks are green. Those platform jobs, rather than the arm64 build, support the i386/ClangCL conclusion. This PR adds coverage; it does not claim to fix main's separate timestamp UBSan batch failure (#154), nor does it establish a performance gain. Keep the new jobs and existing sanitizer job when resolving later merges. |
|
Review round: 2026-09-29 (adversarial re-verification, no code change). Reviewed commit: 7-lens adversarial pass on the current head — 7/7 GREEN.
Verdict: MERGE-READY confirmed. |
| /* _BitScanReverse* out-param is 'unsigned long*' (32-bit on Windows); | ||
| uint32_t* is a hard error under clang-cl */ |
There was a problem hiding this comment.
Maybe we should remove the comment (won't make sense after merging)
| /* _BitScanReverse* out-param is 'unsigned long*' (32-bit on Windows); | |
| uint32_t* is a hard error under clang-cl */ |
There was a problem hiding this comment.
Trimmed in 48ef6b6 to a single durable line — /* _BitScanReverse writes an unsigned long */ — dropping the clang-cl/PR-context framing. Kept a one-liner rather than removing it entirely so the unsigned long (not uint32_t) choice doesn't get "cleaned up" back into a clang-cl hard error later; matches @mikeb01's terse-why style. Shout if you'd rather it go completely.
|
|
||
| /* volatile read */ | ||
| old_active = hdr_atomic_load_pointer(&r->active); | ||
| old_active = hdr_atomic_load_pointer((void**) &r->active); |
There was a problem hiding this comment.
WDYT about changing the atomic helpers' types instead of casting here (and line 74)?
void** isn't interchangeable with struct hdr_histogram**; the cast seems to hide the mismatch rather than resolve it
Both hdr_atomic_store_pointer and hdr_atomic_load_pointer have 2~3 callers each, so updating them should be a small change (??)
There was a problem hiding this comment.
Good call — done in 48ef6b6. You're right that the cast hides a real mismatch: void** and struct hdr_histogram** aren't compatible, and on the __atomic branch the helper is already a type-generic macro, so the cast there actually introduced a void** aliasing access that wasn't there before.
Rather than casting at the call sites, I made the two remaining branches (MSVC and the x86_64 asm fallback) type-generic macros to match the __atomic branch — so hdr_atomic_{load,store}_pointer now accept a typed T** everywhere and all three call sites in hdr_interval_recorder.c (plus the two in hdr_atomic_test.c) drop the cast. MSVC: (_ReadBarrier(), *(x)) / (_WriteBarrier(), (void)(*(f)=(v))); asm: __typeof__-based statement expressions. gcc ctest 5/5 + ASan/UBSan 5/5 (incl. the atomic concurrency test that drives these via the recorder). The clang-cl + i386 legs this PR adds will validate the MSVC-branch macros. Happy to switch to typed helper functions instead if you'd prefer that shape.
…**) casts Addresses @paulorsousa review: rather than casting typed T** to void** at each call site (which papers over an incompatible-pointer mismatch and, on the __atomic branch, would introduce a void** aliasing access where none existed), make the MSVC and x86_64-asm branches type-generic macros like the __atomic branch already is. Callers in hdr_interval_recorder.c and the atomic test now pass &r->active with no cast. Also trim the count_leading_zeros comment to the durable why. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review round: 2026-09-29 (follow-up). Reviewed commit: Correction to my earlier note: this PR is not CI-only — besides the i386/ClangCL matrix legs it also hardens the AVX2 runtime-dispatch guard in @paulorsousa's two points are addressed in
gcc ctest 5/5 + ASan/UBSan 5/5 (incl. the atomic concurrency test that drives the recorder's pointer atomics). The MSVC-branch macros are exactly what the new clang-cl leg this PR adds will validate. Glad to switch to typed helper functions instead if that's the preferred shape. |
3 participants
Follow-up to #142 (ClangCL) and #143 (i386), which both edited the AVX2 runtime-dispatch guard in
src/hdr_histogram.cbut were validated manually rather than in CI.Why
The CI matrix excludes 32-bit Linux and never selects the ClangCL toolset, so neither configuration the guard specifically handles was exercised. A future refactor of the guard could silently reintroduce either break.
What
Two regression jobs, in both of which
HDR_HAS_AVX2_DISPATCHmust be compiled out:build-linux-i386— 32-bit GCC (-m32,gcc-multilib),HDR_LOG_REQUIRED=DISABLEDso no 32-bit zlib is needed. The core suites still runhdr_value_at_percentile(the dispatched path). Guards against the Don't attempt AVX2 on i386 #143 break (_mm_extract_epi64is unavailable in 32-bit codegen).build-windows-clangcl— the ClangCL VS toolset (-T ClangCL) on x64. Guards against the Fix ClangCL build on Windows #142 break (clang-cl defines__clang__+_MSC_VERand previously failed to link__cpu_model).Also simplifies the guard:
_M_X64is a dead term once_MSC_VERis excluded (only the MSVC family defines_M_X64, and clang-cl uses_M_X64rather than__x86_64__), so it now gates on__x86_64__alone, with a comment documenting why 32-bit / MSVC-ABI / ICC are excluded.No functional change on the platforms that ship the AVX2 path (64-bit GCC/Clang on Linux/macOS): the fast path stays enabled there.
🤖 Generated with Claude Code