From patchwork Fri Oct 18 15:13:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1999268 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=OJ1E21HM; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.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 (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4XVSwc1KF1z1xth for ; Sat, 19 Oct 2024 02:16:48 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 606E13858429 for ; Fri, 18 Oct 2024 15:16:46 +0000 (GMT) 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.133.124]) by sourceware.org (Postfix) with ESMTP id 28E1E3858C42 for ; Fri, 18 Oct 2024 15:16:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28E1E3858C42 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 28E1E3858C42 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1729264577; cv=none; b=XMvL9b5f/r0yNUHJhA63PjIJPUmrxgAmreKKdbWxq+i+F521Zf2FF+pXh3SQJ2bsb8Io+PI9a5OYJj/PzMX/anFYw3RqCz9/aKOZy79DKN/zeOq/6q5F1owj6FxWRMmr/iyg7E2QqqqZ1KRIOUyFofkGXPSa0e5gmdqHPAdgy9o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1729264577; c=relaxed/simple; bh=uDLCGYMjn7rGzobOrVWhGziyVTXEZAPJzXVJcZBDP3c=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=UBfb5NaExh5QKHxIuuibOhfQjB5+A8HAwYZOwuAabjPADV6PsCjCO66gxODsvbYCQPPJldztMxW+aNrfRZWCscF5fvK6KIHXNpTW//iwuuTMgWSXm7QeW3uz5IeaKG6ciTJ0wpcUni01zIRmpidnckVMxThBnZy2glOX9XXOoXA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1729264574; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=7ca8gWhnn9LAA8LMruWoJDLyHa/rwDaK816auFjWTSU=; b=OJ1E21HMBAxI9Gt1Q9xqDf9zUKYg2h3NmyF+hNpdr+RysTDwNwoYd3+iRd85VCOcq6mcYl Y1DHmTwuFTvvc296QjscKA8eo1HbQV1BIZMPdAvp9fzefG3lCPYzOc203oCzM/mkIn6tjB HGqpB+bcFBXVdmNXKsJjAZk6t5DvMBY= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-218-prKtJtW3M-mP507adPXpBw-1; Fri, 18 Oct 2024 11:16:11 -0400 X-MC-Unique: prKtJtW3M-mP507adPXpBw-1 Received: from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.17]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 362301955F3C; Fri, 18 Oct 2024 15:16:10 +0000 (UTC) Received: from localhost (unknown [10.42.28.5]) by mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 78DC519560A3; Fri, 18 Oct 2024 15:16:08 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Cc: =?utf-8?q?Fran=C3=A7ois_Dumont?= Subject: [PATCH] libstdc++: Avoid using std::__to_address with iterators Date: Fri, 18 Oct 2024 16:13:03 +0100 Message-ID: <20241018151607.387875-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.17 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.0 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_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org Do others agree with my reasoning below? The changes to implement the rule "use std::__niter_base before C++20 and use std::to_address after C++20" were easier than I expected. There weren't many places that were doing it "wrong" and needed to change. Tested x86_64-linux. -- >8 -- In r12-3935-g82626be2d633a9 I added the partial specialization std::pointer_traits<__normal_iterator> so that __to_address would work with __normal_iterator objects. Soon after that, François replaced it in r12-6004-g807ad4bc854cae with an overload of __to_address that served the same purpose, but was less complicated and less wrong. I now think that both commits were mistakes, and that instead of adding hacks to make __normal_iterator work with __to_address, we should not be using __to_address with iterators at all before C++20. The pre-C++20 std::__to_address function should only be used with pointer-like types, specifically allocator_traits::pointer types. Those pointer-like types are guaranteed to be contiguous iterators, so that getting a raw memory address from them is OK. For arbitrary iterators, even random access iterators, we don't know that it's safe to lower the iterator to a pointer e.g. for std::deque iterators it's not, because (it + n) == (std::to_address(it) + n) only holds within the same block of the deque's storage. For C++20, std::to_address does work correctly for contiguous iterators, including __normal_iterator, and __to_address just calls std::to_address so also works. But we have to be sure we have an iterator that satisfies the std::contiguous_iterator concept for it to be safe, and we can't check that before C++20. So for pre-C++20 code the correct way to handle iterators that might be pointers or might be __normal_iterator is to call __niter_base, and if necessary use is_pointer to check whether __niter_base returned a real pointer. We currently have some uses of std::__to_address with iterators where we've checked that they're either pointers, or __normal_iterator wrappers around pointers, or satisfy std::contiguous_iterator. But this seems a little fragile, and it would be better to just use std::__niter_base for the pointers and __normal_iterator cases, and use C++20 std::to_address when the C++20 std::contiguous_iterator concept is satisfied. This patch does that. libstdc++-v3/ChangeLog: * include/bits/basic_string.h (basic_string::assign): Replace use of __to_address with __niter_base or std::to_address as appropriate. * include/bits/ptr_traits.h (__to_address): Add comment. * include/bits/shared_ptr_base.h (__shared_ptr): Qualify calls to __to_address. * include/bits/stl_algo.h (find): Replace use of __to_address with __niter_base or std::to_address as appropriate. Only use either of them when the range is not empty. * include/bits/stl_iterator.h (__to_address): Remove overload for __normal_iterator. * include/debug/safe_iterator.h (__to_address): Remove overload for _Safe_iterator. * include/std/ranges (views::counted): Replace use of __to_address with std::to_address. * testsuite/24_iterators/normal_iterator/to_address.cc: Removed. --- libstdc++-v3/include/bits/basic_string.h | 19 +++++++++++++------ libstdc++-v3/include/bits/ptr_traits.h | 4 ++++ libstdc++-v3/include/bits/shared_ptr_base.h | 4 ++-- libstdc++-v3/include/bits/stl_algo.h | 16 +++++++++++----- libstdc++-v3/include/bits/stl_iterator.h | 12 ------------ libstdc++-v3/include/debug/safe_iterator.h | 17 ----------------- libstdc++-v3/include/std/ranges | 2 +- .../normal_iterator/to_address.cc | 19 ------------------- 8 files changed, 31 insertions(+), 62 deletions(-) delete mode 100644 libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index e9b17ea48b5..16e356e0678 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -1732,18 +1732,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string& assign(_InputIterator __first, _InputIterator __last) { + using _IterTraits = iterator_traits<_InputIterator>; + if constexpr (is_pointer::value + && is_same::value) + { + __glibcxx_requires_valid_range(__first, __last); + return _M_replace(size_type(0), size(), + std::__niter_base(__first), __last - __first); + } #if __cplusplus >= 202002L - if constexpr (contiguous_iterator<_InputIterator> - && is_same_v, _CharT>) -#else - if constexpr (__is_one_of<_InputIterator, const_iterator, iterator, - const _CharT*, _CharT*>::value) -#endif + else if constexpr (contiguous_iterator<_InputIterator> + && is_same_v, + _CharT>) { __glibcxx_requires_valid_range(__first, __last); return _M_replace(size_type(0), size(), std::__to_address(__first), __last - __first); } +#endif else return *this = basic_string(__first, __last, get_allocator()); } diff --git a/libstdc++-v3/include/bits/ptr_traits.h b/libstdc++-v3/include/bits/ptr_traits.h index ca67feecca3..cef88f61f8c 100644 --- a/libstdc++-v3/include/bits/ptr_traits.h +++ b/libstdc++-v3/include/bits/ptr_traits.h @@ -211,6 +211,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __ptr; } + // This should only be used for pointer-like types (e.g. allocator pointers) + // and (in C++20 and later) for types satisfying std::contiguous_iterator. + // It should not be used for arbitrary random access iterators, because + // they might not be contiguous iterators (e.g. deque::iterator isn't). template constexpr typename std::pointer_traits<_Ptr>::element_type* __to_address(const _Ptr& __ptr) diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h b/libstdc++-v3/include/bits/shared_ptr_base.h index ef0658f6182..9a7617e7014 100644 --- a/libstdc++-v3/include/bits/shared_ptr_base.h +++ b/libstdc++-v3/include/bits/shared_ptr_base.h @@ -1560,7 +1560,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __shared_ptr(unique_ptr<_Yp, _Del>&& __r) : _M_ptr(__r.get()), _M_refcount() { - auto __raw = __to_address(__r.get()); + auto __raw = std::__to_address(__r.get()); _M_refcount = __shared_count<_Lp>(std::move(__r)); _M_enable_shared_from_this_with(__raw); } @@ -1576,7 +1576,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __shared_ptr(unique_ptr<_Tp1, _Del>&& __r, __sp_array_delete) : _M_ptr(__r.get()), _M_refcount() { - auto __raw = __to_address(__r.get()); + auto __raw = std::__to_address(__r.get()); _M_refcount = __shared_count<_Lp>(std::move(__r)); _M_enable_shared_from_this_with(__raw); } diff --git a/libstdc++-v3/include/bits/stl_algo.h b/libstdc++-v3/include/bits/stl_algo.h index 780bd8e5e82..c10f8aa6395 100644 --- a/libstdc++-v3/include/bits/stl_algo.h +++ b/libstdc++-v3/include/bits/stl_algo.h @@ -3834,7 +3834,7 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO using _ValT = typename iterator_traits<_InputIterator>::value_type; if constexpr (__can_use_memchr_for_find<_ValT, _Tp>) if constexpr (is_pointer_v -#if __cpp_lib_concepts +#if __glibcxx_concepts && __glibcxx_to_address || contiguous_iterator<_InputIterator> #endif ) @@ -3847,11 +3847,17 @@ _GLIBCXX_BEGIN_NAMESPACE_ALGO return __last; else if (!__is_constant_evaluated()) { - const void* __p0 = std::__to_address(__first); const int __ival = static_cast(__val); - if (auto __n = std::distance(__first, __last); __n > 0) - if (auto __p1 = __builtin_memchr(__p0, __ival, __n)) - return __first + ((const char*)__p1 - (const char*)__p0); + if (auto __n = __last - __first; __n > 0) + { +#if __glibcxx_concepts && __glibcxx_to_address + const void* __p0 = std::to_address(__first); +#else + const void* __p0 = std::__niter_base(__first); +#endif + if (auto __p1 = __builtin_memchr(__p0, __ival, __n)) + return __first + ((const char*)__p1 - (const char*)__p0); + } return __last; } } diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h index 26c5eab4b4e..05fb488f119 100644 --- a/libstdc++-v3/include/bits/stl_iterator.h +++ b/libstdc++-v3/include/bits/stl_iterator.h @@ -1348,18 +1348,6 @@ namespace std _GLIBCXX_VISIBILITY(default) { _GLIBCXX_BEGIN_NAMESPACE_VERSION -#if __cplusplus >= 201103L && __cplusplus <= 201703L - // Need to overload __to_address because the pointer_traits primary template - // will deduce element_type of __normal_iterator as T* rather than T. - template - [[__gnu__::__always_inline__]] _GLIBCXX_NODISCARD - constexpr auto - __to_address(const __gnu_cxx::__normal_iterator<_Iterator, - _Container>& __it) noexcept - -> decltype(std::__to_address(__it.base())) - { return std::__to_address(__it.base()); } -#endif - #if __cplusplus >= 201103L /** * @addtogroup iterators diff --git a/libstdc++-v3/include/debug/safe_iterator.h b/libstdc++-v3/include/debug/safe_iterator.h index d3e959b8fa7..983cbae37da 100644 --- a/libstdc++-v3/include/debug/safe_iterator.h +++ b/libstdc++-v3/include/debug/safe_iterator.h @@ -1161,23 +1161,6 @@ namespace __gnu_debug } // namespace __gnu_debug -#if __cplusplus >= 201103L && __cplusplus <= 201703L -namespace std _GLIBCXX_VISIBILITY(default) -{ -_GLIBCXX_BEGIN_NAMESPACE_VERSION - - template - constexpr auto - __to_address(const __gnu_debug::_Safe_iterator< - __gnu_cxx::__normal_iterator<_Iterator, _Container>, - _Sequence>& __it) noexcept - -> decltype(std::__to_address(__it.base().base())) - { return std::__to_address(__it.base().base()); } - -_GLIBCXX_END_NAMESPACE_VERSION -} -#endif - #undef _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_END #undef _GLIBCXX20_CONSTEXPR_NON_LITERAL_SCOPE_BEGIN #undef _GLIBCXX_DEBUG_VERIFY_DIST_OPERANDS diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges index 5b455888536..9f233729d9c 100644 --- a/libstdc++-v3/include/std/ranges +++ b/libstdc++-v3/include/std/ranges @@ -3935,7 +3935,7 @@ namespace views::__adaptor operator() [[nodiscard]] (_Iter __i, iter_difference_t<_Iter> __n) const { if constexpr (contiguous_iterator<_Iter>) - return span(std::__to_address(__i), __n); + return span(std::to_address(__i), __n); else if constexpr (random_access_iterator<_Iter>) return subrange(__i, __i + __n); else diff --git a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc b/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc deleted file mode 100644 index 6afc6540609..00000000000 --- a/libstdc++-v3/testsuite/24_iterators/normal_iterator/to_address.cc +++ /dev/null @@ -1,19 +0,0 @@ -// { dg-do compile { target { c++11 } } } -#include -#include -#include - -#include - -char* p __attribute__((unused)) - = std::__to_address(std::string("1").begin()); -const char* q __attribute__((unused)) - = std::__to_address(std::string("2").cbegin()); -int* r __attribute__((unused)) - = std::__to_address(std::vector(1, 1).begin()); -const int* s __attribute__((unused)) - = std::__to_address(std::vector(1, 1).cbegin()); -int* t __attribute__((unused)) - = std::__to_address(std::vector>(1, 1).begin()); -const int* u __attribute__((unused)) - = std::__to_address(std::vector>(1, 1).cbegin());