From 7d7acfd66e56e1b6279de7c5595934c958dc075c Mon Sep 17 00:00:00 2001 From: Jamie Pond Date: Sun, 19 Apr 2026 22:38:38 -0700 Subject: [PATCH] Fix UB in PopcountLogic<6> CombiningMask on standards-conformant compilers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PopcountLogic::CombiningMask computed `T(1 << HalvedGroupSize) - 1`, which performs the shift at `int` precision because `1` is an `int`. For `LogarithmOfGroupSize = 6`, `HalvedGroupSize` is 32 — so the expression is `1 << 32`, which is UB per [expr.shift] (shift count not less than width of the promoted type). GCC happens to accept this in a constant-expression context; Clang and MSVC /std:c++20 /permissive- correctly reject it ("non-type template argument is not a constant expression", "shift count 32 >= width of type 'int' (32 bits)"). The result is that any translation unit that includes under MSVC with conformance mode fails to compile, because SWAR.h's unconditional static_asserts on line 589-596 force instantiation of PopcountLogic<6, uint64_t> via logarithmFloor. Fix: do the shift at `T`'s precision — `T(T(1) << HalvedGroupSize) - 1`. Add a regression test that directly instantiates PopcountLogic<6, u64>. popcount() would have caught this, but on non-MSVC platforms it routes through PopcountIntrinsic (which uses __builtin_popcountll) and never touches PopcountLogic<6>, which is why the existing popcount tests at <1>, <2>, <4>, <5> all passed. --- inc/zoo/meta/popcount.h | 2 +- test/swar/BasicOperations.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/inc/zoo/meta/popcount.h b/inc/zoo/meta/popcount.h index 3b00056b..f952484b 100644 --- a/inc/zoo/meta/popcount.h +++ b/inc/zoo/meta/popcount.h @@ -40,7 +40,7 @@ struct PopcountLogic { constexpr static int GroupSize = 1 << LogarithmOfGroupSize; constexpr static int HalvedGroupSize = GroupSize / 2; constexpr static auto CombiningMask = - BitmaskMaker::value; + BitmaskMaker::value; using Recursion = PopcountLogic; constexpr static T execute(T input); }; diff --git a/test/swar/BasicOperations.cpp b/test/swar/BasicOperations.cpp index 602384ae..591236ea 100644 --- a/test/swar/BasicOperations.cpp +++ b/test/swar/BasicOperations.cpp @@ -358,6 +358,33 @@ static_assert(0x210 == popcount<1>(0x320)); static_assert(0x4321 == popcount<2>(0xF754)); static_assert(0x50004 == popcount<4>(0x3E001122)); +// Regression: PopcountLogic<6, u64>::CombiningMask previously computed +// `T(1 << 32) - 1`. The shift runs at `int` precision because `1` is an +// `int`, so `1 << 32` is UB per [expr.shift] and fails as a constant +// expression under any standards-conformant constexpr evaluator (clang, +// MSVC /std:c++20 /permissive-). GCC happens to accept it, which is why +// the bug was dormant. The fix is `T(T(1) << 32) - 1`. Without it, +// `CombiningMask` fails to be a constant expression and these asserts +// fail to compile. +// +// We reach PopcountLogic<6> directly rather than via popcount<6>(x), +// because on non-MSVC platforms popcount<6> routes through the +// PopcountIntrinsic struct (which uses __builtin_popcountll) and never +// instantiates PopcountLogic<6, u64> — the original code path that +// carried the bug. +static_assert(meta::PopcountLogic<6, u64>::CombiningMask == 0x0000'0000'FFFF'FFFFull); +static_assert(meta::PopcountLogic<6, u64>::HalvedGroupSize == 32); +static_assert(meta::PopcountLogic<6, u64>::GroupSize == 64); +static_assert(0 == meta::PopcountLogic<6, u64>::execute(0ull)); +static_assert(1 == meta::PopcountLogic<6, u64>::execute(1ull)); +static_assert(1 == meta::PopcountLogic<6, u64>::execute(1ull << 32)); +static_assert(1 == meta::PopcountLogic<6, u64>::execute(1ull << 63)); +static_assert(32 == meta::PopcountLogic<6, u64>::execute(0xFFFFFFFFull)); +static_assert(32 == meta::PopcountLogic<6, u64>::execute(0xFFFFFFFF00000000ull)); +static_assert(32 == meta::PopcountLogic<6, u64>::execute(0xAAAAAAAAAAAAAAAAull)); +static_assert(32 == meta::PopcountLogic<6, u64>::execute(0x0123456789ABCDEFull)); +static_assert(64 == meta::PopcountLogic<6, u64>::execute(0xFFFFFFFFFFFFFFFFull)); + static_assert(1 == msbIndex(1ull<<1)); static_assert(3 == msbIndex(1ull<<3)); static_assert(5 == msbIndex(1ull<<5));