Repository navigation
Fix ClangCL build on Windows - #142
Conversation
filipecosta90
left a comment
filipecosta90
left a comment
There was a problem hiding this comment.
Adversarial review — verdict: correct & safe ✅
I ran a multi-angle adversarial review of this one-line change (seven independent passes: compiler-macro correctness, root-cause, build/CI coverage, performance, dead-code/interaction, portability, and UB/correctness). Six of seven concluded the change is good; the seventh raised only a maintainability point about the overlap with #143 (see below). Nothing blocks merge.
Root cause (confirmed). clang-cl defines both __clang__ and _MSC_VER (and _M_X64 on x64), so before this change the guard was satisfied via _M_X64 && __clang__ and HDR_HAS_AVX2_DISPATCH was enabled. The failure is a link error, not a compile error: __builtin_cpu_supports("avx2") emits a reference to compiler-rt's __cpu_model / __cpu_indicator_init, which the MSVC toolchain does not auto-link → unresolved symbol. !defined(_MSC_VER) is exactly the right discriminator — a __has_builtin(__builtin_cpu_supports) probe would not help, since the front end supports the builtin and the breakage is purely at link time.
No collateral damage. The exclusion only affects toolchains that define _MSC_VER and __GNUC__||__clang__, i.e. clang-cl / icx-cl. It correctly keeps AVX2 dispatch for MinGW and GNU-ABI clang (which define __clang__/__GNUC__ but not _MSC_VER), is a no-op for plain MSVC cl.exe (already excluded by the __GNUC__||__clang__ requirement), and is a no-op on Linux/macOS/BSD.
Correctness & performance. The only target that loses the AVX2 path is clang-cl x64 — which currently does not build at all — so "scalar that compiles" strictly dominates. The scalar fallback (get_value_from_idx_up_to_count_scalar) returns byte-identical percentile results; only throughput differs. The 64-bit GNU/Clang fast path on Linux/macOS is untouched.
Non-blocking notes:
-
After this change
_M_X64/_M_IX86become dead terms (they imply_MSC_VER), so the guard could be simplified. -
CI has no clang-cl (
-T ClangCL) toolset job, so this fix is validated manually (the Node.js build) rather than in CI. Awindows-latest+-T ClangCLmatrix entry would close that gap. -
Overlap with #143: that PR edits the same physical line (it removes
__i386__/_M_IX86). I confirmed the two conflict textually —git applyof one on top of the other fails — so whichever merges first, the other needs a rebase. The clean end state that satisfies both goals with no dead terms is a single combined guard:#if defined(__x86_64__) \ && (defined(__GNUC__) || defined(__clang__)) && !defined(__INTEL_COMPILER) && !defined(_MSC_VER)
Preprocessor-verified: dispatch stays ON for Linux/mac GCC+Clang x86_64, and OFF for clang-cl x64 (your fix) and i386 (#143's fix).
Thanks for the fix and the clear root-cause writeup from the Node.js side.
…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>
…-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>
…-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>
…X64 (#144) * ci: cover i386 and ClangCL for the AVX2-dispatch guard; drop dead _M_X64 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> * fix: clang-cl build error in count_leading_zeros_64 (_BitScanReverse 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> * fix: clang-cl build error in hdr_interval_recorder (void** atomic args) 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> * docs+ci: correct clang-cl guard comment; assert i386 job is really 32-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> * ci: assert AVX2 dispatch is enabled on 64-bit x86 * ci: assert AVX2 dispatch is compiled out on i386 * ci: make i386 AVX2-dispatch negative assert fail loudly 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> * atomic: make hdr_atomic_{load,store}_pointer type-generic; drop (void**) 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> --------- Co-authored-by: fcostaoliveira <filipe@redis.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: fcostaoliveira <oly@codeperf.io>
2 participants
This problem was detected when updating the Histogram dependency in Node.js in this PR. The patch worked locally when building Node, so I am opening this PR to push it upstream.