microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
10.04k stars 1.48k forks source link

<type_traits>: aligned_storage has incorrect alignment defaults #784

Open CaseyCarter opened 4 years ago

CaseyCarter commented 4 years ago

[tab:meta.trans.other] specifies that default-alignment in

template<size_t Len, size_t Align = default-alignment>
struct aligned_storage;

"shall be the most stringent alignment requirement for any C++ object type whose size is no greater than Len". Since all valid alignments are powers of two, and the size of an object is always a multiple of its alignment, the default should be the largest power of two which divides Len evenly and is not greater than some upper bound. That upper bound is either alignof(max_align_t) (the largest fundamental alignment requirement) or some compiler-specific implementation limit on the largest possible alignment for an object type. I suspect the Standard intends the former, but I've asked LWG for clarification.

In any case, we should not be using alignof(max_align_t) as the default alignment for all sizes, which results in silliness like sizeof(aligned_storage<1>) == 8.

Command-line test case (See https://godbolt.org/z/J7naDp)

C:\Users\Casey\Desktop>type repro.cpp
#include <cstddef>
#include <type_traits>
#include <utility>

using std::aligned_storage_t, std::index_sequence, std::max_align_t, std::size_t;

constexpr size_t alignment_for(size_t const Size) {
    for (size_t alignment = 1; alignment < alignof(max_align_t); alignment *= 2) {
        if (((alignment * 2 - 1) & Size) != 0) {
            return alignment;
        }
    }
    return alignof(max_align_t);
}

template <size_t Size>
constexpr void test_one() {
    static_assert(alignof(aligned_storage_t<Size>) == alignment_for(Size));
    static_assert(alignof(aligned_storage_t<Size>) != alignof(max_align_t));
    static_assert(sizeof(aligned_storage_t<Size>) == Size);
}

template <size_t... Sizes>
constexpr void test(index_sequence<Sizes...>) {
    (test_one<Sizes + 1>(), ...);
}

static_assert((test(std::make_index_sequence<alignof(max_align_t) - 1>{}), true));

static_assert(alignof(aligned_storage_t<2 * alignof(max_align_t)>) == alignof(max_align_t));

C:\Users\Casey\Desktop>cl /nologo /std:c++17 /permissive- repro.cpp
repro.cpp
repro.cpp(18): error C2607: static assertion failed
repro.cpp(24): note: see reference to function template instantiation 'void test_one<1>(void)' being compiled
repro.cpp(28): note: see reference to function template instantiation 'void test<0,1,2,3,4,5,6>(std::integer_sequence<size_t,0,1,2,3,4,5,6>)' being compiled
repro.cpp(28): note: while evaluating constexpr function 'test'
repro.cpp(19): error C2607: static assertion failed
repro.cpp(28): note: while evaluating constexpr function 'test'
repro.cpp(20): error C2607: static assertion failed
repro.cpp(28): note: while evaluating constexpr function 'test'

vNext note: Resolving this issue will require breaking binary compatibility. We won't be able to accept pull requests for this issue until the vNext branch is available. See #169 for more information.

Additional Context Skipped libcxx test https://github.com/microsoft/STL/blob/06827feb4cdc4d2328dfbfab9fd5302de6058dd9/tests/libcxx/expected_results.txt#L644-L645

SuperWig commented 4 years ago

Is it worth creating a vNext/ABI breaking issue template? (I notice a lot fail to mention that note)

StephanTLavavej commented 4 years ago

Hmm - I'd worry that such an issue template could create confusion, and it wouldn't help if we realize that an ordinary issue should be labeled vNext.

SuperWig commented 4 years ago

Alright, cool. Nagging it is.


You could also turn "Checklist For Merging A Pull Request" into "Maintainer Guidelines" with a point to remember to add the note if/when marking an issue as vNext?

StephanTLavavej commented 4 years ago

In our weekly meeting, the other maintainers felt that having issue guidelines/checklists would be too much process. IIRC, Casey mentioned that he added the note to all existing vNext issues, so hopefully it'll be easier to remember for new issues. We certainly won't mind if you (or anyone else) replies with this note to a vNext issue either. :cat: