Możliwe niezdefiniowane zachowanie w pierwotnej implementacji static_vector

12

tl; dr: Myślę, że mój static_vector ma niezdefiniowane zachowanie, ale nie mogę go znaleźć.

Ten problem dotyczy Microsoft Visual C ++ 17. Mam tę prostą i niedokończoną implementację static_vector, tj. Wektor o stałej pojemności, który można przypisać do stosu. Jest to program w C ++ 17, wykorzystujący std :: aligned_storage i std :: launder. Próbowałem sprowadzić go poniżej do części, które moim zdaniem są istotne dla problemu:

template <typename T, size_t NCapacity>
class static_vector
{
public:
    typedef typename std::remove_cv<T>::type value_type;
    typedef size_t size_type;
    typedef T* pointer;
    typedef const T* const_pointer;
    typedef T& reference;
    typedef const T& const_reference;

    static_vector() noexcept
        : count()
    {
    }

    ~static_vector()
    {
        clear();
    }

    template <typename TIterator, typename = std::enable_if_t<
        is_iterator<TIterator>::value
    >>
    static_vector(TIterator in_begin, const TIterator in_end)
        : count()
    {
        for (; in_begin != in_end; ++in_begin)
        {
            push_back(*in_begin);
        }
    }

    static_vector(const static_vector& in_copy)
        : count(in_copy.count)
    {
        for (size_type i = 0; i < count; ++i)
        {
            new(std::addressof(storage[i])) value_type(in_copy[i]);
        }
    }

    static_vector& operator=(const static_vector& in_copy)
    {
        // destruct existing contents
        clear();

        count = in_copy.count;
        for (size_type i = 0; i < count; ++i)
        {
            new(std::addressof(storage[i])) value_type(in_copy[i]);
        }

        return *this;
    }

    static_vector(static_vector&& in_move)
        : count(in_move.count)
    {
        for (size_type i = 0; i < count; ++i)
        {
            new(std::addressof(storage[i])) value_type(move(in_move[i]));
        }
        in_move.clear();
    }

    static_vector& operator=(static_vector&& in_move)
    {
        // destruct existing contents
        clear();

        count = in_move.count;
        for (size_type i = 0; i < count; ++i)
        {
            new(std::addressof(storage[i])) value_type(move(in_move[i]));
        }

        in_move.clear();

        return *this;
    }

    constexpr pointer data() noexcept { return std::launder(reinterpret_cast<T*>(std::addressof(storage[0]))); }
    constexpr const_pointer data() const noexcept { return std::launder(reinterpret_cast<const T*>(std::addressof(storage[0]))); }
    constexpr size_type size() const noexcept { return count; }
    static constexpr size_type capacity() { return NCapacity; }
    constexpr bool empty() const noexcept { return count == 0; }

    constexpr reference operator[](size_type n) { return *std::launder(reinterpret_cast<T*>(std::addressof(storage[n]))); }
    constexpr const_reference operator[](size_type n) const { return *std::launder(reinterpret_cast<const T*>(std::addressof(storage[n]))); }

    void push_back(const value_type& in_value)
    {
        if (count >= capacity()) throw std::out_of_range("exceeded capacity of static_vector");
        new(std::addressof(storage[count])) value_type(in_value);
        count++;
    }

    void push_back(value_type&& in_moveValue)
    {
        if (count >= capacity()) throw std::out_of_range("exceeded capacity of static_vector");
        new(std::addressof(storage[count])) value_type(move(in_moveValue));
        count++;
    }

    template <typename... Arg>
    void emplace_back(Arg&&... in_args)
    {
        if (count >= capacity()) throw std::out_of_range("exceeded capacity of static_vector");
        new(std::addressof(storage[count])) value_type(forward<Arg>(in_args)...);
        count++;
    }

    void pop_back()
    {
        if (count == 0) throw std::out_of_range("popped empty static_vector");
        std::destroy_at(std::addressof((*this)[count - 1]));
        count--;
    }

    void resize(size_type in_newSize)
    {
        if (in_newSize > capacity()) throw std::out_of_range("exceeded capacity of static_vector");

        if (in_newSize < count)
        {
            for (size_type i = in_newSize; i < count; ++i)
            {
                std::destroy_at(std::addressof((*this)[i]));
            }
            count = in_newSize;
        }
        else if (in_newSize > count)
        {
            for (size_type i = count; i < in_newSize; ++i)
            {
                new(std::addressof(storage[i])) value_type();
            }
            count = in_newSize;
        }
    }

    void clear()
    {
        resize(0);
    }

private:
    typename std::aligned_storage<sizeof(T), alignof(T)>::type storage[NCapacity];
    size_type count;
};

Przez jakiś czas działało to dobrze. W pewnym momencie robiłem coś bardzo podobnego do tego - rzeczywisty kod jest dłuższy, ale sedno tego:

struct Foobar
{
    uint32_t Member1;
    uint16_t Member2;
    uint8_t Member3;
    uint8_t Member4;
}

void Bazbar(const std::vector<Foobar>& in_source)
{
    static_vector<Foobar, 8> valuesOnTheStack { in_source.begin(), in_source.end() };

    auto x = std::pair<static_vector<Foobar, 8>, uint64_t> { valuesOnTheStack, 0 };
}

Innymi słowy, najpierw kopiujemy 8-bajtowe struktury Foobar do static_vector na stosie, a następnie tworzymy std :: parę static_vector 8-bajtowych struktur jako pierwszy element, a uint64_t jako drugi. Mogę zweryfikować, czy wartościOnTheStack zawierają prawidłowe wartości bezpośrednio przed zbudowaniem pary. I ... to segfault z włączoną optymalizacją wewnątrz konstruktora kopiowania static_vector (który został wbudowany w funkcję wywołującą) podczas konstruowania pary.

Krótko mówiąc, sprawdziłem demontaż. To gdzie rzeczy stają się trochę dziwne; wygenerowany asm wokół wbudowanego konstruktora kopii pokazano poniżej - zauważ, że pochodzi on z rzeczywistego kodu, a nie z powyższego przykładu, który jest dość blisko, ale ma więcej elementów powyżej konstrukcji pary:

00621E45  mov         eax,dword ptr [ebp-20h]  
00621E48  xor         edx,edx  
00621E4A  mov         dword ptr [ebp-70h],eax  
00621E4D  test        eax,eax  
00621E4F  je          <this function>+29Ah (0621E6Ah)  
00621E51  mov         eax,dword ptr [ecx]  
00621E53  mov         dword ptr [ebp+edx*8-0B0h],eax  
00621E5A  mov         eax,dword ptr [ecx+4]  
00621E5D  mov         dword ptr [ebp+edx*8-0ACh],eax  
00621E64  inc         edx  
00621E65  cmp         edx,dword ptr [ebp-70h]  
00621E68  jb          <this function>+281h (0621E51h)  

Okej, więc najpierw mamy dwie instrukcje mov kopiujące element zliczający ze źródła do miejsca docelowego; na razie w porządku. edx jest zerowany, ponieważ jest to zmienna pętli. Następnie mamy szybkie sprawdzenie, czy liczba wynosi zero; nie jest zero, więc przechodzimy do pętli for, w której kopiujemy 8-bajtową strukturę za pomocą dwóch 32-bitowych operacji mov, najpierw z pamięci do rejestracji, a następnie z rejestru do pamięci. Ale jest coś podejrzanego - w przypadku, gdy spodziewalibyśmy się czegoś z [ebp + edx * 8 +] do odczytu z obiektu źródłowego, zamiast tego jest po prostu ... [ecx]. To nie brzmi dobrze. Jaka jest wartość ecx?

Okazuje się, że ecx zawiera tylko adres śmieci, ten sam, na którym segfagujemy. Skąd ta wartość? Oto asm bezpośrednio powyżej:

00621E1C  mov         eax,dword ptr [this]  
00621E22  push        ecx  
00621E23  push        0  
00621E25  lea         ecx,[<unrelated local variable on the stack, not the static_vector>]  
00621E2B  mov         eax,dword ptr [eax]  
00621E2D  push        ecx  
00621E2E  push        dword ptr [eax+4]  
00621E31  call        dword ptr [<external function>@16 (06AD6A0h)]  

To wygląda jak zwykłe stare wywołanie funkcji cdecl. Rzeczywiście, funkcja ma wywołanie zewnętrznej funkcji C tuż powyżej. Ale zwróć uwagę na to, co się dzieje: ecx jest używany jako tymczasowy rejestr do wypychania argumentów na stos, funkcja jest wywoływana, a ... ecx nigdy nie jest dotykany, dopóki nie zostanie błędnie użyty poniżej do odczytu ze źródła static_vector.

W praktyce zawartość ecx zostaje nadpisana przez wywoływaną tutaj funkcję, co oczywiście jest dozwolone. Ale nawet jeśli tak się nie stanie, w żadnym wypadku ecx nigdy nie będzie zawierał adresu poprawnej rzeczy - w najlepszym razie wskazywałby na element lokalnego stosu, który nie jest statycznym wektorem. Wygląda na to, że kompilator wyemitował fałszywy zestaw. Ta funkcja nigdy nie może wygenerować prawidłowego wyniku.

Więc tam jestem teraz. Dziwny montaż, gdy włączone są optymalizacje podczas zabawy w std :: launder land, pachnie jak niezdefiniowane zachowanie. Ale nie widzę, skąd to może pochodzić. Jako dodatkowa, ale marginalnie użyteczna informacja, brzęk z odpowiednimi flagami tworzy podobny zestaw do tego, z tym wyjątkiem, że poprawnie używa ebp + edx zamiast ecx do odczytu wartości.

pjohansson
źródło
Tylko pobieżny wygląd, ale dlaczego clear()korzystasz z zasobów, do których dzwoniłeś std::move?
Batszeba
Nie rozumiem, jak to jest istotne. Jasne, byłoby również dopuszczalne pozostawienie static_vector tego samego rozmiaru, ale kilku przeniesionych obiektów. Zawartość zostanie zniszczona, gdy i tak uruchomi się destruktor static_vector. Ale wolę pozostawić przeniesiony wektor o rozmiarze zero.
pjohansson
Szum. Zatem poza moją kategorią płac. Poproś o opinię, ponieważ jest ona dobrze zadawana i może przyciągnąć uwagę.
Batszeba
Nie można odtworzyć awarii z twoim kodem (nie pomaga to, że nie kompiluje się z powodu braku is_iterator), podaj minimalny odtwarzalny przykład
Alan Birtles
1
przy okazji, myślę, że wiele kodu nie ma tu znaczenia. Chodzi mi o to, że nigdzie nie dzwonisz do operatora przypisań, aby można go było usunąć z przykładu
bartop

Odpowiedzi:

6

Myślę, że masz błąd kompilatora. Dodanie __declspec( noinline )do operator[]wydaje się naprawić awarię:

__declspec( noinline ) constexpr const_reference operator[]( size_type n ) const { return *std::launder( reinterpret_cast<const T*>( std::addressof( storage[ n ] ) ) ); }

Możesz spróbować zgłosić błąd firmie Microsoft, ale wydaje się, że błąd został już naprawiony w Visual Studio 2019.

std::launderWydaje się, że usunięcie również naprawia awarię:

constexpr const_reference operator[]( size_type n ) const { return *reinterpret_cast<const T*>( std::addressof( storage[ n ] ) ); }
Alan Birtles
źródło
Brakuje mi też innych wyjaśnień. Mimo że jest to do bani, biorąc pod uwagę naszą obecną sytuację, wydaje się prawdopodobne, że o to chodzi, więc zaznaczę to jako przyjętą odpowiedź.
pjohansson
Usunięcie prania to naprawia? Usunięcie prania byłoby jawnie niezdefiniowanym zachowaniem! Dziwne.
pjohansson
@pjohansson std::launderjest / był znany z niepoprawnej implementacji niektórych implementacji. Być może twoja wersja MSVS opiera się na tej nieprawidłowej implementacji. Niestety nie mam źródeł.
Fureeish