Avoid UB in MSB computation in feistel permutation.

[niklas-uhl]

While this has no impact on the actual implementation, the missing
check resulted in a failed assertion

KaGen/kagen/tools/random_permutation.h

Line 297 in 1118666

assert(most_significant_bit_set(left_half) <= _bits_left_half);

When the left half is all zeros, most_significant_bit_set is
undefined and may return arbitrary large values, making this assertion
fail in debug builds.

[mschimek]

AI suggests something like

template <typename Data>
std::uint8_t most_significant_bit_set(const Data arg) {
    static_assert(std::is_unsigned<Data>::value, "Data must be an unsigned integral type.");

    // __builtin_clz* are undefined for 0
    if (arg == 0) {
        return 0;
    }

    constexpr std::size_t width = std::numeric_limits<Data>::digits; // number of value bits
    std::size_t leading_zeros = 0;

    if constexpr (width == std::numeric_limits<unsigned int>::digits) {
        leading_zeros = static_cast<std::size_t>(__builtin_clz(static_cast<unsigned int>(arg)));
    } else if constexpr (width == std::numeric_limits<unsigned long>::digits) {
        leading_zeros = static_cast<std::size_t>(__builtin_clzl(static_cast<unsigned long>(arg)));
    } else if constexpr (width == std::numeric_limits<unsigned long long>::digits) {
        leading_zeros = static_cast<std::size_t>(__builtin_clzll(static_cast<unsigned long long>(arg)));
    } else {
        static_assert(width == std::numeric_limits<unsigned long long>::digits,
                      "Unsupported Data width for builtin clz");
    }

    // 0-based index of the most significant set bit
    const std::size_t msb_index = width - 1 - leading_zeros;
    return static_cast<std::uint8_t>(msb_index);
}

what do you think?
Do we ever call it with signed integers? If not, we could just a std::is_same_v<unsigned ..., Data>?

[niklas-uhl]

It's only ever called with uint64_t, and only once, when computing the needed number of bits based on the bit-width of the maximum value of the permutation. All remaining calls are in assertions which just check that the computed number of bits is big enough to fit the value.

[niklas-uhl]

Since the code uses uint64_t we need the if constexpr but now at least everything else is SFINAEd out and easier to understand.

[niklas-uhl]

There might still be happening weird things when the number of vertices to permute is zero, we should rename most_significant_bit_set to most_significant_bit, and it should probably return an optional. But this is something for another PR.