boostorg / container

STL-like containers from Boost
http://www.boost.org/libs/container/
Boost Software License 1.0
96 stars 116 forks source link

Regression when using recursive_wrapper<small_vector<incomplete, 4>> #173

Closed pdimov closed 3 years ago

pdimov commented 3 years ago

As reported on Slack, the following code

#include <boost/variant.hpp>
#include <boost/container/small_vector.hpp>

struct variable;

using table_value = boost::container::small_vector<variable, 4>;

using value_type = boost::variant<int, boost::recursive_wrapper<table_value>>;

struct __declspec(dllexport) variable
{
    value_type value_;
};

int main()
{
}

doesn't compile under MSVC 2019 (see https://github.com/KazDragon/telnetpp/issues/238), but is claimed to have worked with 1.74. The issue doesn't reproduce if I use recursive_wrapper<variable> directly, without the small_vector, and recursive_wrapper hasn't changed since 1.74, so it must be a change in small_vector that has triggered the failure.

KazDragon commented 3 years ago

https://ci.appveyor.com/project/KazDragon/telnetpp/builds/36920601/job/6ae7oxa6taxsvivt is a build using code of that layout when building a DLL on MSVC with Boost 1.74.

igaztanaga commented 3 years ago

I don't know how this was supposed to work with small_vector, as small_vector is not supposed to work with incomplete types (small_vector needs to store variables internally so it needs to know the size and the alignment, which requires a fully defined type).

In the example, due to the DLL atribute; value_ is fully instantiated, which somehow requires small_vectorto be instantiated. The instantiation chain suggests that boost::is_nothrow_move_assignable is called for boost::recursive_wrapper which triggers the incomplete small_vector instantiation for small_vector.

In the example, removing the dllexport attribute makes the error go away, but I can't figure out why dllexport changes the rules. The issue starts in the BOOST_NOEXCEPT_IF part of variant that triggers the instantiation of small_vector<variable,> while variable is still not fully defined:


#ifndef BOOST_NO_CXX11_RVALUE_REFERENCES
    variant& operator=(variant&& rhs)
#if !defined(__GNUC__) || (__GNUC__ != 4) || (__GNUC_MINOR__ > 6) || defined(__clang__)
        BOOST_NOEXCEPT_IF(variant_move_noexcept_constructible::type::value && variant_move_noexcept_assignable::type::value)
#endif
    {
        variant_assign( detail::variant::move(rhs) );
        return *this;
    }
igaztanaga commented 3 years ago

Further experiments suggest that it is related to the implicit generation of copy/move constructor/assignment operations, as this works fine:


struct __declspec(dllexport) variable
{
   variable (const variable &o)  : value_(o.value_)  {}
   variable (variable &&o)         : value_(boost::move(o.value_))  {}

   variable & operator=(const variable &o)
   {    value_ = o.value_;   return *this;    }

   variable & operator=(variable &&o)
   {   value_ = boost::move(o.value_);   return *this;   }

   value_type value_;
};

I am unsure this is a `small_vector` issue.
igaztanaga commented 3 years ago

Reviewing the issue I think it's not a Boost.Container error.

small_vector is not supposed to support incomplete types and somehow using __declspec(dllexport) provokes early instantiation of small_vector.

I understand that this can be viewed as a regression, but I think code worked accidentally and I don't want to support those corner cases as any change in small_vector could trigger similar errors.

Maybe this could indicate a compiler issue since removing dllexport fixes the compilation error.