From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 9B4373858D39 for ; Tue, 18 Jan 2022 21:48:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9B4373858D39 Received: from mail-yb1-f200.google.com (mail-yb1-f200.google.com [209.85.219.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-163-pbdQkrBpPm2D8Balhs3b1Q-1; Tue, 18 Jan 2022 16:48:31 -0500 X-MC-Unique: pbdQkrBpPm2D8Balhs3b1Q-1 Received: by mail-yb1-f200.google.com with SMTP id 127-20020a250f85000000b00611ab6484abso641571ybp.23 for ; Tue, 18 Jan 2022 13:48:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6VxHS9dyNMbnA/66Z3socOJH3LSmXSzaDxxKS1X88kk=; b=qTQZdLKXEY0zr1mjbENApiEGXVITse22KJlXHhvpN75//1noXiWsTYiXHM+pYDZBx3 ANWd1N7XanWnCDEWEpmu5hV3VvXV3PC+xVF4SFCN1Wuwzfg3DpoFSH+blKPZKOxx4QZB 8zj+/7bNdSAtRTsMGxPmp/q9/eYsJRB3ojvVAvoi+wGSWxA5va572djIOwl65V+hdqpb Fr074fVnBQ6JRR6zQSRC+CZdeq/jx4sYJxc8UbIDlE4SuZwUFPt7PR9RIyxWCMp3EeEx pQPyv9jnfKHcG/lmbX1VeLLfTa71G0XHN3Ijd+T03GEjNSyOLOdWOz8//pfJlE8jATdt LtlA== X-Gm-Message-State: AOAM533vFbUu8rjJAcEenft8QB9K4mIU4i/sfL4jiWaWjxQWzzAEv8Rw C0d8FD5nYiqZ5HSsmUJNCdV+Digg1wT96RP/DnjdimApJhB3/4W8Te8hiV/4WpK2CFgbM8guyEA lS+8/xyWp6CZ/JthXV76WTCNJMUbhIEk= X-Received: by 2002:a25:86cd:: with SMTP id y13mr36093381ybm.120.1642542510997; Tue, 18 Jan 2022 13:48:30 -0800 (PST) X-Google-Smtp-Source: ABdhPJyTKoju6hubN+DwNF9TiW/1sqhh/UZZEHv6VGhmDrSpUE6lVzacmqGtuC7Zd5E0HVcpkyYwBnzMnyAQYfVKACI= X-Received: by 2002:a25:86cd:: with SMTP id y13mr36093351ybm.120.1642542510591; Tue, 18 Jan 2022 13:48:30 -0800 (PST) MIME-Version: 1.0 References: <20210923180837.633173-1-rodgert@appliantology.com> <20210927141031.651313-1-rodgert@appliantology.com> In-Reply-To: From: Jonathan Wakely Date: Tue, 18 Jan 2022 21:48:19 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange To: Thomas Rodgers Cc: Thomas Rodgers , gcc Patches , "libstdc++" , Thomas Rodgers X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, HTML_MESSAGE, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2022 21:48:35 -0000 On Tue, 2 Nov 2021 at 01:26, Thomas Rodgers wrote: > This should address Jonathan's feedback and adds support for atomic_ref > >This change implements P0528 which requires that padding bits not >participate in atomic compare exchange operations. All arguments to the >generic template are 'sanitized' by the __builtin_clearpadding intrisic The name of the intrinsic and the word "instrinsic" have typos. >before they are used in comparisons. This alrequires that any stores >also sanitize the incoming value. > >Signed-off-by: Thomas Rodgers > >libstdc++=v3/ChangeLog: Typo > > * include/std/atomic (atomic::atomic(_Tp): clear padding for Unclosed paren. >+#if __has_builtin(__builtin_clear_padding) Instead of checking this built-in at every call site, can't we just make __maybe_has_padding return false if the built-in isn't supported? __clear_padding already handles the case where the built-in isn't supported. >+ template >+ constexpr bool >+ __maybe_has_padding() >+ { >+#if __has_builtin(__has_unique_object_representations) >+ return !__has_unique_object_representations(_Tp) >+ && !is_floating_point<_Tp>::value; >+#else >+ return true; >+#endif >+ } So make that: template constexpr bool __maybe_has_padding() { #if ! __has_builtin(__builtin_clear_padding) return false; #elif __has_builtin(__has_unique_object_representations) return !__has_unique_object_representations(_Tp) && !is_floating_point<_Tp>::value; #else return true; #endif } >+ if _GLIBCXX14_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) >+ { This needs to be _GLIBCXX17_CONSTEXPR (everywhere that `if constexpr` is used). >+ { >+ alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; >+ __builtin_memcpy(__buf, std::__addressof(__e), sizeof(_Tp)); alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; _Tp* __exp = ::new((void*)__buf) _Tp(__e); >+ auto* __exp = __atomic_impl::__clear_padding(*reinterpret_cast<_Tp*>(__buf)); And then you don't need the reinterpret_cast: __exp = __atomic_impl::__clear_padding(__exp); >+ auto* __des = __atomic_impl::__clear_padding(__i); >+ if (__atomic_compare_exchange(std::__addressof(__val), __exp, __des, __weak, >+ int(__s), int(__f))) >+ return true; > template > _GLIBCXX_ALWAYS_INLINE void > store(_Tp* __ptr, _Val<_Tp> __t, memory_order __m) noexcept >- { __atomic_store(__ptr, std::__addressof(__t), int(__m)); } >+ { >+#if __has_builtin(__builtin_clear_padding) >+ if _GLIBCXX14_CONSTEXPR (__maybe_has_padding<_Tp>()) >+ __clear_padding(__t); >+#endif >+ __atomic_store(__ptr, std::__addressof(__t), int(__m)); >+ } > All calls to __clear_padding need to be qualified. >+ return __compare_exchange(*__ptr, __expected, __desired, true, >+ __success, __failure); So do calls to __compare_exchange. > > explicit > __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t)) >- { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); } >+ { >+ __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding) >+ __builtin_clear_padding(_M_ptr); >+#endif >+ } Is this safe to do? What if multiple threads all create a std::atomic_ref round the same object at once, they'll all try to clear padding, and so race, won't they? I don't think we can clear padding on atomic_ref construction, only on store and RMW operations. >--- a/libstdc++-v3/include/std/atomic >+++ b/libstdc++-v3/include/std/atomic >@@ -228,13 +228,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > atomic& operator=(const atomic&) = delete; > atomic& operator=(const atomic&) volatile = delete; > >-#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > constexpr atomic(_Tp __i) noexcept : _M_i(__i) >- { __builtin_clear_padding(std::__addressof(_M_i)); } >-#else >- constexpr atomic(_Tp __i) noexcept : _M_i(__i) >- { } >+ { >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding) >+ __builtin_clear_padding(std::__addressof(_M_i)); > #endif >+ } > Is this an incremental patch relative to the first one? The changes to this file look correct. >--- /dev/null >+++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc >@@ -0,0 +1,43 @@ >+// { dg-options "-std=gnu++2a" } >+// { dg-do run { target c++2a } } This new test is using "2a" not "20".