From patchwork Thu Feb 16 14:39:08 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1743731 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=upJgUcX/; dkim-atps=neutral Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4PHd0h5WhQz23yD for ; Fri, 17 Feb 2023 01:40:51 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 11A963896C1B for ; Thu, 16 Feb 2023 14:40:47 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 11A963896C1B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1676558447; bh=yTTb0DHPZczYNtqUJMaN9Q3DejnpBzzUX+Pnk/QkGA0=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=upJgUcX/iMvGEkqiLaYsoVCbNL0FG1ZEr2MWRRsYV+Bu7rl0DxLP7YPCMwvB263Oo aE4PV4WQE0kw4lYKW65le+SYbhdUaZRvzgIOqCsdvAmyTJCuKCEA38GxnLlYTVqUcj Y1//KtFRfZbmW3IzpEj2pqdYNpzXTaqxitU542mI= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 93C8B383FBA8 for ; Thu, 16 Feb 2023 14:40:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 93C8B383FBA8 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-63-iMSRQ4nNPzqIszeqcjNlng-1; Thu, 16 Feb 2023 09:39:18 -0500 X-MC-Unique: iMSRQ4nNPzqIszeqcjNlng-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 490C93C0F1A9; Thu, 16 Feb 2023 14:39:09 +0000 (UTC) Received: from localhost (unknown [10.33.37.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id E86B0C15BA0; Thu, 16 Feb 2023 14:39:08 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Implement P2255R2 dangling checks for std::pair Date: Thu, 16 Feb 2023 14:39:08 +0000 Message-Id: <20230216143908.138045-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" Tested powerpc64le-linux. Pushed to trunk. -- >8 -- This uses the new __reference_constructs_from_temporary built-in to identify when a std::pair constructor will bind a reference to a temporary that goes out of scope at the end of the constructor. For example, std::pair p(1, 2); will call the pair::pair(U1&&, U2&&) constructor with U1=int and U2=int. In the constructor body a temporary long will be created and the p.first member will bind to that temporary. When the constructor returns, the reference is immediately dangling. P2255 requires the constructor to be deleted to prevent this bug. Although P2255 was approved for C++23, it fixes a longstanding LWG issue in older standards, and it turns silent runtime undefined behaviour into a compilation error. Because of that, the dangling checks are applied all the way back to C++98. However, if these changes cause too much code to be rejected (e.g. in cases where the dangling reference is never used after the constructor returns) then we can consider removing them for C++20 and older standards. The affected constructors are deleted for C++20 and later, when concepts are available to simplify the constructor constraints. For C++17 and earlier the overload sets are complicated and awkward to maintain, so the dangling checks are done in static assertions in the constructor bodies, instead of being SFINAE-friendly constraints. The pre-C++17 assertions are only enabled for Debug Mode, to avoid introducing a breaking change in Stage 4. We should consider enabling them by default in Stage 1 for GCC 14. libstdc++-v3/ChangeLog: * include/bits/stl_pair.h (pair) [C++20]: Add non-dangling constraints to constructors and add deleted overloads for the dangling cases, as per P2255R2. (pair) [!C++20 && _GLIBCXX_DEBUG]: Add static assertions to make dangling cases ill-formed. * testsuite/20_util/pair/dangling_ref.cc: New test. --- libstdc++-v3/include/bits/stl_pair.h | 112 +++++++++++++++--- .../testsuite/20_util/pair/dangling_ref.cc | 67 +++++++++++ 2 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 libstdc++-v3/testsuite/20_util/pair/dangling_ref.cc diff --git a/libstdc++-v3/include/bits/stl_pair.h b/libstdc++-v3/include/bits/stl_pair.h index d0c73410526..3f1624f40b4 100644 --- a/libstdc++-v3/include/bits/stl_pair.h +++ b/libstdc++-v3/include/bits/stl_pair.h @@ -281,6 +281,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return is_convertible_v<_U2, _T2>; return false; } + + // True if construction from _U1 and _U2 would create a dangling ref. + template + static constexpr bool + _S_dangles() + { +#if __has_builtin(__reference_constructs_from_temporary) + if constexpr (__reference_constructs_from_temporary(_T1, _U1&&)) + return true; + else + return __reference_constructs_from_temporary(_T2, _U2&&); +#else + return false; +#endif + } /// @endcond public: @@ -295,25 +310,37 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION /// Constructor accepting two values of arbitrary types template - requires (_S_constructible<_U1, _U2>()) + requires (_S_constructible<_U1, _U2>()) && (!_S_dangles<_U1, _U2>()) constexpr explicit(!_S_convertible<_U1, _U2>()) pair(_U1&& __x, _U2&& __y) noexcept(_S_nothrow_constructible<_U1, _U2>()) : first(std::forward<_U1>(__x)), second(std::forward<_U2>(__y)) { } + template + requires (_S_constructible<_U1, _U2>()) && (_S_dangles<_U1, _U2>()) + constexpr explicit(!_S_convertible<_U1, _U2>()) + pair(_U1&&, _U2&&) = delete; + /// Converting constructor from a const `pair` lvalue template requires (_S_constructible()) + && (!_S_dangles<_U1, _U2>()) constexpr explicit(!_S_convertible()) pair(const pair<_U1, _U2>& __p) noexcept(_S_nothrow_constructible()) : first(__p.first), second(__p.second) { } + template + requires (_S_constructible()) + && (_S_dangles()) + constexpr explicit(!_S_convertible()) + pair(const pair<_U1, _U2>&) = delete; + /// Converting constructor from a non-const `pair` rvalue template - requires (_S_constructible<_U1, _U2>()) + requires (_S_constructible<_U1, _U2>()) && (!_S_dangles<_U1, _U2>()) constexpr explicit(!_S_convertible<_U1, _U2>()) pair(pair<_U1, _U2>&& __p) noexcept(_S_nothrow_constructible<_U1, _U2>()) @@ -321,25 +348,42 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION second(std::forward<_U2>(__p.second)) { } + template + requires (_S_constructible<_U1, _U2>()) && (_S_dangles<_U1, _U2>()) + constexpr explicit(!_S_convertible<_U1, _U2>()) + pair(pair<_U1, _U2>&&) = delete; + #if __cplusplus > 202002L /// Converting constructor from a non-const `pair` lvalue template - requires (_S_constructible<_U1&, _U2&>()) + requires (_S_constructible<_U1&, _U2&>()) && (!_S_dangles<_U1&, _U2&>()) constexpr explicit(!_S_convertible<_U1&, _U2&>()) pair(pair<_U1, _U2>& __p) noexcept(_S_nothrow_constructible<_U1&, _U2&>()) : first(__p.first), second(__p.second) { } + template + requires (_S_constructible<_U1&, _U2&>()) && (_S_dangles<_U1&, _U2&>()) + constexpr explicit(!_S_convertible<_U1&, _U2&>()) + pair(pair<_U1, _U2>&) = delete; + /// Converting constructor from a const `pair` rvalue template requires (_S_constructible()) + && (!_S_dangles()) constexpr explicit(!_S_convertible()) pair(const pair<_U1, _U2>&& __p) noexcept(_S_nothrow_constructible()) : first(std::forward(__p.first)), second(std::forward(__p.second)) { } + + template + requires (_S_constructible()) + && (_S_dangles()) + constexpr explicit(!_S_convertible()) + pair(const pair<_U1, _U2>&&) = delete; #endif // C++23 private: @@ -463,6 +507,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #else // !__cpp_lib_concepts // C++11/14/17 implementation using enable_if, partially constexpr. + /// @cond undocumented + // Error if construction from _U1 and _U2 would create a dangling ref. +#if __has_builtin(__reference_constructs_from_temporary) \ + && defined _GLIBCXX_DEBUG +# define __glibcxx_no_dangling_refs(_U1, _U2) \ + static_assert(!__reference_constructs_from_temporary(_T1, _U1) \ + && !__reference_constructs_from_temporary(_T2, _U2), \ + "std::pair constructor creates a dangling reference") +#else +# define __glibcxx_no_dangling_refs(_U1, _U2) +#endif + /// @endcond + /** The default constructor creates @c first and @c second using their * respective default constructors. */ template ::template _ImplicitlyConvertiblePair<_U1, _U2>(), bool>::type=true> - constexpr pair(const pair<_U1, _U2>& __p) - : first(__p.first), second(__p.second) { } + constexpr pair(const pair<_U1, _U2>& __p) + : first(__p.first), second(__p.second) + { __glibcxx_no_dangling_refs(const _U1&, const _U2&); } template::template @@ -535,7 +593,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _ImplicitlyConvertiblePair<_U1, _U2>(), bool>::type=false> explicit constexpr pair(const pair<_U1, _U2>& __p) - : first(__p.first), second(__p.second) { } + : first(__p.first), second(__p.second) + { __glibcxx_no_dangling_refs(const _U1&, const _U2&); } #if _GLIBCXX_USE_DEPRECATED #if defined(__DEPRECATED) @@ -575,7 +634,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_DEPRECATED_PAIR_CTOR constexpr pair(_U1&& __x, __zero_as_null_pointer_constant, ...) - : first(std::forward<_U1>(__x)), second(nullptr) { } + : first(std::forward<_U1>(__x)), second(nullptr) + { __glibcxx_no_dangling_refs(_U1&&, std::nullptr_t); } template>, @@ -587,7 +647,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_DEPRECATED_PAIR_CTOR explicit constexpr pair(_U1&& __x, __zero_as_null_pointer_constant, ...) - : first(std::forward<_U1>(__x)), second(nullptr) { } + : first(std::forward<_U1>(__x)), second(nullptr) + { __glibcxx_no_dangling_refs(_U1&&, std::nullptr_t); } template, @@ -599,7 +660,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_DEPRECATED_PAIR_CTOR constexpr pair(__zero_as_null_pointer_constant, _U2&& __y, ...) - : first(nullptr), second(std::forward<_U2>(__y)) { } + : first(nullptr), second(std::forward<_U2>(__y)) + { __glibcxx_no_dangling_refs(std::nullptr_t, _U2&&); } template, @@ -611,7 +673,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _GLIBCXX_DEPRECATED_PAIR_CTOR explicit constexpr pair(__zero_as_null_pointer_constant, _U2&& __y, ...) - : first(nullptr), second(std::forward<_U2>(__y)) { } + : first(nullptr), second(std::forward<_U2>(__y)) + { __glibcxx_no_dangling_refs(std::nullptr_t, _U2&&); } #undef _GLIBCXX_DEPRECATED_PAIR_CTOR #endif @@ -622,7 +685,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _ImplicitlyMoveConvertiblePair<_U1, _U2>(), bool>::type=true> constexpr pair(_U1&& __x, _U2&& __y) - : first(std::forward<_U1>(__x)), second(std::forward<_U2>(__y)) { } + : first(std::forward<_U1>(__x)), second(std::forward<_U2>(__y)) + { __glibcxx_no_dangling_refs(_U1&&, _U2&&); } template(), bool>::type=false> explicit constexpr pair(_U1&& __x, _U2&& __y) - : first(std::forward<_U1>(__x)), second(std::forward<_U2>(__y)) { } + : first(std::forward<_U1>(__x)), second(std::forward<_U2>(__y)) + { __glibcxx_no_dangling_refs(_U1&&, _U2&&); } template::type=true> constexpr pair(pair<_U1, _U2>&& __p) : first(std::forward<_U1>(__p.first)), - second(std::forward<_U2>(__p.second)) { } + second(std::forward<_U2>(__p.second)) + { __glibcxx_no_dangling_refs(_U1&&, _U2&&); } template::template @@ -652,7 +718,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION bool>::type=false> explicit constexpr pair(pair<_U1, _U2>&& __p) : first(std::forward<_U1>(__p.first)), - second(std::forward<_U2>(__p.second)) { } + second(std::forward<_U2>(__p.second)) + { __glibcxx_no_dangling_refs(_U1&&, _U2&&); } + +#undef __glibcxx_no_dangling_refs pair& operator=(__conditional_t<__and_, @@ -714,7 +783,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION /// Templated constructor to convert from other pairs. template pair(const pair<_U1, _U2>& __p) - : first(__p.first), second(__p.second) { } + : first(__p.first), second(__p.second) + { +#if __has_builtin(__reference_constructs_from_temporary) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wunused-local-typedefs" + typedef int _DanglingCheck1[ + __reference_constructs_from_temporary(_T1, const _U1&) ? -1 : 1 + ]; + typedef int _DanglingCheck2[ + __reference_constructs_from_temporary(_T2, const _U2&) ? -1 : 1 + ]; +#pragma GCC diagnostic pop +#endif + } #endif // C++11 }; diff --git a/libstdc++-v3/testsuite/20_util/pair/dangling_ref.cc b/libstdc++-v3/testsuite/20_util/pair/dangling_ref.cc new file mode 100644 index 00000000000..8e0c34816dd --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/pair/dangling_ref.cc @@ -0,0 +1,67 @@ +// { dg-do compile { target c++11 } } +// { dg-options "-Wno-unused-variable" } +// { dg-additional-options "-D_GLIBCXX_DEBUG" { target c++17_down } } + +#include + +#if __cplusplus >= 202002L +// For C++20 and later, constructors are constrained to disallow dangling. +static_assert(!std::is_constructible_v, long, int>); +static_assert(!std::is_constructible_v, int, long>); +static_assert(!std::is_constructible_v, + std::pair>); +static_assert(!std::is_constructible_v, + std::pair>); +static_assert(!std::is_constructible_v, + const std::pair&>); +static_assert(!std::is_constructible_v, + const std::pair&>); +#endif + +void +test_binary_ctors() +{ + std::pair p1(1L, 2); + // { dg-error "here" "" { target c++17_down } 24 } + // { dg-error "use of deleted function" "" { target c++20 } 24 } + + std::pair p2(1, 2L); + // { dg-error "here" "" { target c++17_down } 28 } + // { dg-error "use of deleted function" "" { target c++20 } 28 } + + std::pair p3(1L, 2L); + // { dg-error "here" "" { target c++17_down } 32 } + // { dg-error "use of deleted function" "" { target c++20 } 32 } +} + +void +test_converting_ctors() +{ + std::pair p0; + + std::pair p1(p0); + // { dg-error "here" "" { target c++17_down } 42 } + // { dg-error "use of deleted function" "" { target c++20 } 42 } + + std::pair p2(p0); + // { dg-error "here" "" { target c++17_down } 46 } + // { dg-error "use of deleted function" "" { target c++20 } 46 } + + std::pair p3(p0); + // { dg-error "here" "" { target c++17_down } 50 } + // { dg-error "use of deleted function" "" { target c++20 } 50 } + + std::pair p4(std::move(p0)); + // { dg-error "here" "" { target c++17_down } 54 } + // { dg-error "use of deleted function" "" { target c++20 } 54 } + + std::pair p5(std::move(p0)); + // { dg-error "here" "" { target c++17_down } 58 } + // { dg-error "use of deleted function" "" { target c++20 } 58 } + + std::pair p6(std::move(p0)); + // { dg-error "here" "" { target c++17_down } 62 } + // { dg-error "use of deleted function" "" { target c++20 } 62 } +} + +// { dg-error "static assert.* dangling reference" "" { target { c++17_down } } 0 }