Message ID | 20230329234000.1405216-1-jwakely@redhat.com |
---|---|
State | New |
Headers | show |
Series | [committed] libstdc++: Fix constexpr functions in <experimental/internet> | expand |
On Thu, 30 Mar 2023 at 00:40, Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > Tested powerpc64le-linux. Pushed to trunk. > > -- >8 -- > > Change ip::basic_endpoint to work in constant expressions, but only for > C++20 and later (due to the use of a union, which cannot change active > member in constexpr evaluation until C++20). > > During constant evaluation we cannot inspect the common initial sequence > of basic_endpoint's union members to check whether sin_family == AF_INET > or AF_INET6. This means we need to store an additional boolean member > that remembers whether we have a v4 or v6 address. The address type can > change behind our backs if a user copies an address to the data() > pointer and then calls resize(n), so we need to inspect the sa_family_t > member in the union after a resize and update the boolean. POSIX only > guarantees that the sa_family_t member of each protocol-specific address > structure is at the same offset and of the same type, not that there is > a common initial sequence. The check in resize is done using memcmp, so > that we avoid accessing an inactive member of the union if the > sockaddr_in and sockaddr_in6 structures do not have a common initial > sequence that includes the sa_family_t member. If we had https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2641r2.html then we wouldn't need the extra boolean. During constant evaluation we could use the new magic function to check which union member is active, and during runtime we could inspect the sa_family_t member (still using memcmp, because POSIX doesn't guarantee a common initial sequence, even though I think everybody does give it one in practice).
On Thu, 30 Mar 2023 at 15:44, Jonathan Wakely wrote: > > On Thu, 30 Mar 2023 at 00:40, Jonathan Wakely via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: > > > > Tested powerpc64le-linux. Pushed to trunk. > > > > -- >8 -- > > > > Change ip::basic_endpoint to work in constant expressions, but only for > > C++20 and later (due to the use of a union, which cannot change active > > member in constexpr evaluation until C++20). > > > > During constant evaluation we cannot inspect the common initial sequence > > of basic_endpoint's union members to check whether sin_family == AF_INET > > or AF_INET6. This means we need to store an additional boolean member > > that remembers whether we have a v4 or v6 address. The address type can > > change behind our backs if a user copies an address to the data() > > pointer and then calls resize(n), so we need to inspect the sa_family_t > > member in the union after a resize and update the boolean. POSIX only > > guarantees that the sa_family_t member of each protocol-specific address > > structure is at the same offset and of the same type, not that there is > > a common initial sequence. The check in resize is done using memcmp, so > > that we avoid accessing an inactive member of the union if the > > sockaddr_in and sockaddr_in6 structures do not have a common initial > > sequence that includes the sa_family_t member. > > If we had https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2641r2.html > then we wouldn't need the extra boolean. During constant evaluation we > could use the new magic function to check which union member is > active, and during runtime we could inspect the sa_family_t member > (still using memcmp, because POSIX doesn't guarantee a common initial > sequence, even though I think everybody does give it one in practice). In fact, thinking about P2641 some more, I might revert this change. Instead of adding an extra bool member to support constexpr, I think I'll just remove the 'constexpr' keywords from basic_endpoint for now, and implement it in terms of just inspecting the sa_family_t member of the union members. And then later, once we have something like P2641, we can re-add the constexpr keywords and use is_within_lifetime during constant evaluation. That way we don't add a bool then need to take it away again, changing the ABI each time.
Am Do., 30. März 2023 um 18:00 Uhr schrieb Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org>: > [..] > > In fact, thinking about P2641 some more, I might revert this change. > Instead of adding an extra bool member to support constexpr, I think > I'll just remove the 'constexpr' keywords from basic_endpoint for now, > and implement it in terms of just inspecting the sa_family_t member of > the union members. And then later, once we have something like P2641, > we can re-add the constexpr keywords and use is_within_lifetime during > constant evaluation. That way we don't add a bool then need to take it > away again, changing the ABI each time. I was just going to make the same suggestion. - Daniel
On Thu, 30 Mar 2023 at 17:01, Daniel Krügler wrote: > > Am Do., 30. März 2023 um 18:00 Uhr schrieb Jonathan Wakely via > Libstdc++ <libstdc++@gcc.gnu.org>: > > > [..] > > > > In fact, thinking about P2641 some more, I might revert this change. > > Instead of adding an extra bool member to support constexpr, I think > > I'll just remove the 'constexpr' keywords from basic_endpoint for now, > > and implement it in terms of just inspecting the sa_family_t member of > > the union members. And then later, once we have something like P2641, > > we can re-add the constexpr keywords and use is_within_lifetime during > > constant evaluation. That way we don't add a bool then need to take it > > away again, changing the ABI each time. > > I was just going to make the same suggestion. Actually, as pointed out in Barry's P2641R1 paper, we can just use GCC's __builtin_constant_p: + constexpr bool + _M_is_v6() const noexcept + { + // For constexpr eval we can just detect which union member is active. + // i.e. emulate P2641R1's std::is_active_member(&_M_data._M_v6)). + if (std::__is_constant_evaluated()) + return __builtin_constant_p(_M_data._M_v6.sin6_family); + return _M_data._M_v6.sin6_family == AF_INET6; + }
On Fri, 31 Mar 2023 at 12:06, Jonathan Wakely wrote: > > On Thu, 30 Mar 2023 at 17:01, Daniel Krügler wrote: > > > > Am Do., 30. März 2023 um 18:00 Uhr schrieb Jonathan Wakely via > > Libstdc++ <libstdc++@gcc.gnu.org>: > > > > > [..] > > > > > > In fact, thinking about P2641 some more, I might revert this change. > > > Instead of adding an extra bool member to support constexpr, I think > > > I'll just remove the 'constexpr' keywords from basic_endpoint for now, > > > and implement it in terms of just inspecting the sa_family_t member of > > > the union members. And then later, once we have something like P2641, > > > we can re-add the constexpr keywords and use is_within_lifetime during > > > constant evaluation. That way we don't add a bool then need to take it > > > away again, changing the ABI each time. > > > > I was just going to make the same suggestion. > > Actually, as pointed out in Barry's P2641R1 paper, we can just use > GCC's __builtin_constant_p: > > + constexpr bool > + _M_is_v6() const noexcept > + { > + // For constexpr eval we can just detect which union member is active. > + // i.e. emulate P2641R1's std::is_active_member(&_M_data._M_v6)). > + if (std::__is_constant_evaluated()) > + return __builtin_constant_p(_M_data._M_v6.sin6_family); > + return _M_data._M_v6.sin6_family == AF_INET6; > + } Done in commit 10e573e86c6a1ed11df529288ed8fba97f876032
diff --git a/libstdc++-v3/include/experimental/internet b/libstdc++-v3/include/experimental/internet index cae07f466da..dff81b456ab 100644 --- a/libstdc++-v3/include/experimental/internet +++ b/libstdc++-v3/include/experimental/internet @@ -977,7 +977,8 @@ namespace ip { return make_address(__str, __throw_on_error{"make_address"}); } inline address - make_address(const string& __str, error_code& __ec) noexcept; // TODO + make_address(const string& __str, error_code& __ec) noexcept + { return make_address(__str.c_str(), __ec); } inline address make_address(const string& __str) @@ -1275,7 +1276,12 @@ namespace ip constexpr address_v4 broadcast() const noexcept - { return address_v4{_M_addr.to_uint() | (0xFFFFFFFFu >> _M_prefix_len)}; } + { + auto __b = _M_addr.to_uint(); + if (_M_prefix_len < 32) + __b |= 0xFFFFFFFFu >> _M_prefix_len; + return address_v4{__b}; + } address_v4_range hosts() const noexcept @@ -1510,19 +1516,31 @@ namespace ip basic_endpoint() noexcept : _M_data() { _M_data._M_v4.sin_family = protocol_type::v4().family(); } - constexpr + _GLIBCXX20_CONSTEXPR basic_endpoint(const protocol_type& __proto, port_type __port_num) noexcept : _M_data() { - __glibcxx_assert(__proto == protocol_type::v4() - || __proto == protocol_type::v6()); - - _M_data._M_v4.sin_family = __proto.family(); - _M_data._M_v4.sin_port = address_v4::_S_hton_16(__port_num); + if (__proto == protocol_type::v4()) + { + _M_data._M_v4.sin_family = __proto.family(); + _M_data._M_v4.sin_port = address_v4::_S_hton_16(__port_num); + } + else if (__proto == protocol_type::v6()) + { + std::_Construct(&_M_data._M_v6); + _M_data._M_v6.sin6_family = __proto.family(); + _M_data._M_v6.sin6_port = address_v4::_S_hton_16(__port_num); + _M_is_v6 = true; + } + else + { + __glibcxx_assert(__proto == protocol_type::v4() + || __proto == protocol_type::v6()); + } } - constexpr + _GLIBCXX20_CONSTEXPR basic_endpoint(const ip::address& __addr, port_type __port_num) noexcept : _M_data() @@ -1535,37 +1553,42 @@ namespace ip } else { - _M_data._M_v6 = {}; + std::_Construct(&_M_data._M_v6); _M_data._M_v6.sin6_family = protocol_type::v6().family(); _M_data._M_v6.sin6_port = address_v4::_S_hton_16(__port_num); - __builtin_memcpy(_M_data._M_v6.sin6_addr.s6_addr, - __addr._M_v6._M_bytes.data(), 16); + uint8_t* __s6a = _M_data._M_v6.sin6_addr.s6_addr; + for (int __i = 0; __i < 16; ++__i) + __s6a[__i] = __addr._M_v6._M_bytes[__i]; _M_data._M_v6.sin6_scope_id = __addr._M_v6._M_scope_id; + _M_is_v6 = true; } } // members: + constexpr protocol_type protocol() const noexcept { - return _M_is_v6() ? protocol_type::v6() : protocol_type::v4(); + return _M_is_v6 ? protocol_type::v6() : protocol_type::v4(); } constexpr ip::address address() const noexcept { - ip::address __addr; - if (_M_is_v6()) + if (_M_is_v6) { - __builtin_memcpy(&__addr._M_v6._M_bytes, - _M_data._M_v6.sin6_addr.s6_addr, 16); - __addr._M_is_v4 = false; + address_v6 __v6; + const uint8_t* __s6a = _M_data._M_v6.sin6_addr.s6_addr; + for (int __i = 0; __i < 16; ++__i) + __v6._M_bytes[__i] = __s6a[__i]; + __v6._M_scope_id = _M_data._M_v6.sin6_scope_id; + return __v6; } else { - __builtin_memcpy(&__addr._M_v4._M_addr, - &_M_data._M_v4.sin_addr.s_addr, 4); + address_v4 __v4; + __v4._M_addr = _M_data._M_v4.sin_addr.s_addr; + return __v4; } - return __addr; } void @@ -1573,37 +1596,62 @@ namespace ip { if (__addr.is_v6()) { - _M_data._M_v6 = {}; + std::_Construct(&_M_data._M_v6); _M_data._M_v6.sin6_family = protocol_type::v6().family(); __builtin_memcpy(_M_data._M_v6.sin6_addr.s6_addr, __addr._M_v6._M_bytes.data(), 16); _M_data._M_v6.sin6_scope_id = __addr._M_v6._M_scope_id; + _M_is_v6 = true; } else { + std::_Construct(&_M_data._M_v4); _M_data._M_v4.sin_family = protocol_type::v4().family(); _M_data._M_v4.sin_addr.s_addr = __addr._M_v4._M_addr; + _M_is_v6 = false; } } constexpr port_type port() const noexcept - { return address_v4::_S_ntoh_16(_M_data._M_v4.sin_port); } + { + port_type __p = 0; + if (_M_is_v6) + __p = _M_data._M_v6.sin6_port; + else + __p = _M_data._M_v4.sin_port; + return address_v4::_S_ntoh_16(__p); + } void port(port_type __port_num) noexcept - { _M_data._M_v4.sin_port = address_v4::_S_hton_16(__port_num); } + { + __port_num = address_v4::_S_hton_16(__port_num); + if (_M_is_v6) + _M_data._M_v6.sin6_port = __port_num; + else + _M_data._M_v4.sin_port = __port_num; + } void* data() noexcept { return &_M_data; } const void* data() const noexcept { return &_M_data; } - constexpr size_t size() const noexcept - { return _M_is_v6() ? sizeof(sockaddr_in6) : sizeof(sockaddr_in); } + constexpr size_t + size() const noexcept + { return _M_is_v6 ? sizeof(sockaddr_in6) : sizeof(sockaddr_in); } void resize(size_t __s) { + __glibcxx_assert(__s >= 0); + static_assert(offsetof(sockaddr_in6, sin6_family) + == offsetof(sockaddr_in, sin_family), + "sockaddr_in::sin_family and sockaddr_in6::sin6_family " + "must be at the same offset"); + const sa_family_t __in6 = AF_INET6; + const auto* __ptr = (char*)&_M_data + offsetof(sockaddr_in, sin_family); + _M_is_v6 = __builtin_memcmp(&__in6, __ptr, sizeof(__in6)) == 0; if (__s != size()) __throw_length_error("net::ip::basic_endpoint::resize"); } @@ -1617,8 +1665,7 @@ namespace ip sockaddr_in6 _M_v6; } _M_data; - constexpr bool _M_is_v6() const noexcept - { return _M_data._M_v4.sin_family == AF_INET6; } + bool _M_is_v6 = false; }; /** basic_endpoint comparisons diff --git a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/cons.cc b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/cons.cc index af9fef2215e..49bfb63baf6 100644 --- a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/cons.cc +++ b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/cons.cc @@ -68,6 +68,15 @@ test03() VERIFY( a1.to_bytes() == address_v4::bytes_type( 5, 6, 7, 8 ) ); } +constexpr bool +test_constexpr() +{ + test01(); + test02(); + test03(); + return true; +} + int main() { @@ -75,10 +84,5 @@ main() test02(); test03(); - constexpr bool c = []{ - test01(); - test02(); - test03(); - return true; - }; + static_assert( test_constexpr(), "valid in constant expressions" ); } diff --git a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/creation.cc b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/creation.cc index 84aebbb7adc..8469b1267e1 100644 --- a/libstdc++-v3/testsuite/experimental/net/internet/address/v4/creation.cc +++ b/libstdc++-v3/testsuite/experimental/net/internet/address/v4/creation.cc @@ -88,6 +88,14 @@ test03() VERIFY( ec == std::errc::invalid_argument ); } +constexpr bool +test_constexpr() +{ + test01(); + test02(); + return true; +} + int main() { @@ -95,9 +103,5 @@ main() test02(); test03(); - constexpr bool c = []{ - test01(); - test02(); - return true; - }; + static_assert( test_constexpr(), "valid in constant expressions" ); } diff --git a/libstdc++-v3/testsuite/experimental/net/internet/endpoint/cons.cc b/libstdc++-v3/testsuite/experimental/net/internet/endpoint/cons.cc index 1b5c92c0b58..b4bef88b4a3 100644 --- a/libstdc++-v3/testsuite/experimental/net/internet/endpoint/cons.cc +++ b/libstdc++-v3/testsuite/experimental/net/internet/endpoint/cons.cc @@ -21,7 +21,10 @@ test_default() VERIFY( t2.port() == 0 ); } -constexpr void +#if __cplusplus >= 202002 +constexpr +#endif +void test_proto() { ip::tcp::endpoint t1(ip::tcp::v4(), 22); @@ -35,7 +38,10 @@ test_proto() VERIFY( t2.port() == 80 ); } -constexpr void +#if __cplusplus >= 202002 +constexpr +#endif +void test_addr() { ip::address_v4 a1(ip::address_v4::bytes_type(1, 2, 3, 4)); @@ -51,16 +57,23 @@ test_addr() VERIFY( t2.port() == 80 ); } +constexpr bool +test_constexpr() +{ + test_default(); +#if __cplusplus >= 202002 + // Non-default basic_endpoint constructors are only constexpr in C++20. + test_proto(); + test_addr(); +#endif + return true; +} + int main() { test_default(); test_proto(); test_addr(); - constexpr bool c = [] { - test_default(); - test_proto(); - test_addr(); - return true; - }; + static_assert( test_constexpr(), "valid in constant expressions" ); } diff --git a/libstdc++-v3/testsuite/experimental/net/internet/endpoint/extensible.cc b/libstdc++-v3/testsuite/experimental/net/internet/endpoint/extensible.cc new file mode 100644 index 00000000000..d205024c799 --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/net/internet/endpoint/extensible.cc @@ -0,0 +1,47 @@ +// { dg-do run { target c++14 } } +// { dg-require-effective-target net_ts_ip } +// { dg-add-options net_ts } + +#include <experimental/internet> +#include <cstring> +#include <testsuite_hooks.h> + +using namespace std::experimental::net; + +void +test_extensible() +{ + static_assert(ip::tcp::endpoint().capacity() == sizeof(sockaddr_in6), + "ip::tcp::endpoint::capacity() can store a sockaddr_in6"); + + ip::tcp::endpoint t1(ip::tcp::v4(), 22); + VERIFY(t1.size() == sizeof(sockaddr_in)); + t1.resize(sizeof(sockaddr_in)); + try { + t1.resize(2 * sizeof(sockaddr_in)); + VERIFY(false); + } catch (const std::length_error&) { + } + + ip::tcp::endpoint t2(ip::tcp::v6(), 80); + VERIFY(t2.size() == sizeof(sockaddr_in6)); + t2.resize(sizeof(sockaddr_in6)); + try { + t2.resize(2 * sizeof(sockaddr_in6)); + VERIFY(false); + } catch (const std::length_error&) { + } + + ip::tcp::endpoint t3; + std::memcpy(t3.data(), t1.data(), t1.size()); + t3.resize(t1.size()); + VERIFY( t3 == t1 ); + std::memcpy(t3.data(), t2.data(), t2.size()); + t3.resize(t2.size()); + VERIFY( t3 == t2 ); +} + +int main() +{ + test_extensible(); +} diff --git a/libstdc++-v3/testsuite/experimental/net/internet/network/v4/cons.cc b/libstdc++-v3/testsuite/experimental/net/internet/network/v4/cons.cc index 7784b6f6f58..c49f8435584 100644 --- a/libstdc++-v3/testsuite/experimental/net/internet/network/v4/cons.cc +++ b/libstdc++-v3/testsuite/experimental/net/internet/network/v4/cons.cc @@ -111,6 +111,16 @@ test03_errors() } } +constexpr bool +test_constexpr() +{ + test01(); + test02(); + test03(); + return true; +} + + int main() { @@ -120,10 +130,5 @@ main() test03(); test03_errors(); - constexpr bool c = []{ - test01(); - test02(); - test03(); - return true; - }; + static_assert( test_constexpr(), "valid in constant expressions" ); } diff --git a/libstdc++-v3/testsuite/experimental/net/internet/network/v4/members.cc b/libstdc++-v3/testsuite/experimental/net/internet/network/v4/members.cc index 3ea65862649..7587eb1202b 100644 --- a/libstdc++-v3/testsuite/experimental/net/internet/network/v4/members.cc +++ b/libstdc++-v3/testsuite/experimental/net/internet/network/v4/members.cc @@ -166,6 +166,17 @@ test_to_string() VERIFY( str2 == "87.65.43.21/4" ); } +constexpr bool +test_constexpr() +{ + test_netmask(); + test_network(); + test_broadcast(); + test_canonical(); + test_is_host(); + return true; +} + int main() { test_netmask(); @@ -175,12 +186,5 @@ int main() test_is_host(); test_to_string(); - constexpr bool c = []{ - test_netmask(); - test_network(); - test_broadcast(); - test_canonical(); - test_is_host(); - return true; - }; + static_assert( test_constexpr(), "valid in constant expressions" ); }