Fix the behavior of vector<bool, HugeAlloc> whose size overflows uint32_t

[frederick-vs-ja]

On 32-bit platforms, the following program currently behaves incorrectly with MSVC STL - assertions fail (but pass on 64-bit platforms). Godbolt link.

#include <algorithm>
#include <cassert>
#include <cstddef>
#include <cstdint>
#include <memory>
#include <vector>

template <class T>
struct huge_allocator {
    huge_allocator() = default;
    template <class U>
    constexpr huge_allocator(const huge_allocator<U>) noexcept {}

    using value_type      = T;
    using size_type       = std::uint64_t;
    using difference_type = std::int64_t;

    T* allocate(std::uint64_t n) {
        return std::allocator<T>{}.allocate(static_cast<std::size_t>(n));
    }

    void deallocate(T* p, std::uint64_t n) {
        std::allocator<T>{}.deallocate(p, static_cast<std::size_t>(n));
    }
};

int main() {
    constexpr std::uint64_t small_bit_length = 0x7000'4321ULL;
    constexpr std::uint64_t large_bit_length = 0x1'2000'4321ULL; // overflows uint32_t
    constexpr auto large_bit_diff            = static_cast<std::int64_t>(large_bit_length);

    std::vector<bool, huge_allocator<bool>> v(small_bit_length);
    v.resize(large_bit_length);
    assert(v.end() - v.begin() == large_bit_diff);

    v.back() = true;
    assert(std::find(v.begin(), v.end(), true) - v.begin() == large_bit_diff - 1);

    v[small_bit_length]                    = true;
    v[large_bit_length - small_bit_length] = true;
    assert(std::count(v.begin(), v.end(), false) == large_bit_diff - 3);
}

The cause of this issue is that multiplication of _VBITS (of type int after lvalue-to-rvalue conversion) and differences of pointers (of type ptrdiff_t) may overflow int32_t. IMO it's still reasonable for a 32-bit program to allocate ~600MiB storage for a dynamic bitset, so this issue probably needs fix.

This PR casts _VBITS to difference_type/_Difference_type (which is int64_t when using huge_allocator) before multiplication in order to fix the issue.

[strega-nil-ms]

I certainly like the bugfix here; however, I'm not sure I love that we have this new _VBITS_DIFF; it doesn't seem that unreasonable to just cast _VBITS to difference_type in the specific places it's required. I'm still an approve, though.

[StephanTLavavej]

I am somewhat concerned about the test reliability implications of consuming 604 MB at runtime. That said, this is less than the compiler memory consumption of certain ranges tests, so I have no concrete objection to this.

[StephanTLavavej]

No change requested: I searched for all mentions of _VBITS and it appears that all affected locations are indeed being updated. 😻

[StephanTLavavej]

Thanks! I fixed a comment typo and rewrapped it, and changed the test allocator's converting constructor to take a const reference as is traditional.

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[StephanTLavavej]

5 billion thanks for fixing this bug! 😹 🐞 🛠️