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 81EBB385DC2F for ; Tue, 30 Jan 2024 16:34:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 81EBB385DC2F 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 81EBB385DC2F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706632491; cv=none; b=ix52ImaNQmnOiOcYaeFVjBKdFfZek+3MSp0NVOaUbCEX+LfAaWmC2Z4jF0s+B+AZzpm5TaALMOlZFZ1CgrT1D6X9Tol4j+SUxFzAOoq2V+zqLZ1Ewz8TZxfV+iL+iek2B1NXWtXKsvRiO/ZMyLt0NpzDxTBLrm2k96EJcyVBaBY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706632491; c=relaxed/simple; bh=NbuyDViGSlXWxKWNZ/54QL2ujmwncek+5VaPEZQWo0w=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=fLcoWByHEFHpT3y7XtfRqjjAVJuqlbhJ7V06tam5Tv6SZfNwKQ4lRyTRQHka4ZwuKk+eC2D8bj3A6Qm5npXXYBfC5v5icwMd0ravqdjvOS5AnOnONlSqgajerSRDKDPqWD+gdYVQG8W9W7yudFiRzfN+pX/z5H5bH1m1W/uWvm8= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706632489; 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: in-reply-to:in-reply-to:references:references; bh=MNLtX7v31batnXHfvagf9dkTsJ4FBhIO1odLggqFscw=; b=YLafTFW4WJMEDRUkdxQaTpB/x+OIiJe6RUZMDbC7LijUux6B7+ILCaXmDeJKvfdCRH68/n W5FXASJxxqcH+1jkjEC+DuRW5mEkg8Q0k43bZocJSjWqJM3cs6rI/UWMEGfgHToWSelgTi cl88FgpUKBStmcg6+Cy6zff+e7W9t84= Received: from mail-oi1-f198.google.com (mail-oi1-f198.google.com [209.85.167.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-45-2g1SRHu4MBGcu4Tq2pnpJQ-1; Tue, 30 Jan 2024 11:34:47 -0500 X-MC-Unique: 2g1SRHu4MBGcu4Tq2pnpJQ-1 Received: by mail-oi1-f198.google.com with SMTP id 5614622812f47-3be755d9b3bso1469380b6e.2 for ; Tue, 30 Jan 2024 08:34:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706632487; x=1707237287; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MNLtX7v31batnXHfvagf9dkTsJ4FBhIO1odLggqFscw=; b=ZdNKXmnkHEBtOV3aZz3LYjqAufnNlDW3SgyeuNsYv/urKPDsvT/1PakG/5LIl1AcLM 2lN7tfyh4QH1egtL1gwOdlicINBKk0fS+Q8uvxgx9XJh2O69K0aibefcDzFoB0RZDuiU IPWStwOOHkoNbZduC0oS1wqghUOVDSF5asPN3A2vTQGJXTFovOTdtIpNeXdmm+Wo3K4H pWQKVCuMcur+DzNDXs8xDPf3ylDeHCiu0k5lfGl316JY20OLopidMp8kKRoANZqq1Ltx 0KtojX4QcvedIYWF1baHL1UlZT5bp7CuP9efB2RLBUnbmYcF7AzKDLNfJ1ircWUNlza9 oAYg== X-Gm-Message-State: AOJu0YxE4Tbk92plqVGQXTMA1mTGMg5ulMzH9iha6YuRjs/wpCp7dPTu MxTQ+VOGkNvO+rx3D6pL/nT0YWUB25tmf7vvNYlDpSXKATRoaoXN+19D0+HAd4KL+E6Pj5RKHgy ePvWbL8C+vsPzQSzph7b8HKzXCWuMvlEzIC+QaK4GNaRNhOL8SxNiYVFvdwj3VW8xbRxKAXOH1S whaUJRU6uSK9Vzij17WpU9SWTViwM= X-Received: by 2002:a05:6808:159f:b0:3be:3cb3:8231 with SMTP id t31-20020a056808159f00b003be3cb38231mr7647085oiw.26.1706632486915; Tue, 30 Jan 2024 08:34:46 -0800 (PST) X-Google-Smtp-Source: AGHT+IG7BpTZywsJgSTL3GHY3E/QjrdFUpv03yWOZ6eJlkQh6bVK+0bsAjXfTrShl9z8WB9qb4wgC1LSt9d05S6yhAQ= X-Received: by 2002:a05:6808:159f:b0:3be:3cb3:8231 with SMTP id t31-20020a056808159f00b003be3cb38231mr7647067oiw.26.1706632486645; Tue, 30 Jan 2024 08:34:46 -0800 (PST) MIME-Version: 1.0 References: <9f2bdd3a973d98199bb5b322baa575ab2fba8a58.camel@xry111.site> In-Reply-To: From: Jonathan Wakely Date: Tue, 30 Jan 2024 16:34:30 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor To: xndcn Cc: Xi Ruoyao , "H.J. Lu" , GCC Patches , Jakub Jelinek , libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 16 Jan 2024 at 16:17, xndcn wrote: > > Sorry about the mangled content... > So I add a new add-options for libatomic_16b: > > --- > libstdc++-v3/ChangeLog: > > * include/bits/atomic_base.h: add __builtin_clear_padding in > __atomic_float constructor. > * testsuite/lib/dg-options.exp: add new add-options for libatomic_16b. > * testsuite/29_atomics/atomic_float/compare_exchange_padding.cc: New test= . > --- > libstdc++-v3/include/bits/atomic_base.h | 7 ++- > .../atomic_float/compare_exchange_padding.cc | 50 +++++++++++++++++++ > libstdc++-v3/testsuite/lib/dg-options.exp | 22 ++++++++ > 3 files changed, 78 insertions(+), 1 deletion(-) > create mode 100644 > libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.c= c > > diff --git a/libstdc++-v3/include/bits/atomic_base.h > b/libstdc++-v3/include/bits/atomic_base.h > index f4ce0fa53..104ddfdbe 100644 > --- a/libstdc++-v3/include/bits/atomic_base.h > +++ b/libstdc++-v3/include/bits/atomic_base.h > @@ -1283,7 +1283,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > constexpr > __atomic_float(_Fp __t) : _M_fp(__t) > - { } > + { > +#if __cplusplus >=3D 201402L && __has_builtin(__builtin_clear_padding) > + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Fp>()) > + __builtin_clear_padding(std::__addressof(_M_fp)); > +#endif > + } > > __atomic_float(const __atomic_float&) =3D delete; > __atomic_float& operator=3D(const __atomic_float&) =3D delete; > diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_excha= nge_padding.cc > b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding= .cc > new file mode 100644 > index 000000000..d538b3d55 > --- /dev/null > +++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_pad= ding.cc > @@ -0,0 +1,50 @@ > +// { dg-do run { target c++20 } } > +// { dg-options "-O0" } > +// { dg-timeout 10 } > +// { dg-additional-options "-mlong-double-80" { target x86_64-*-* } } > +// { dg-do run { target { ia32 || x86_64-*-* } } } > +// { dg-add-options libatomic_16b } Why not just use -latomic unconditionally here? You're adding a new libatomic_16b option that expands to -latomic for x86 and nothing otherwise, and then using it in a test that only runs for x86. So it's just adding -latomic to all targets that run the test, right? Also, this patch needs either a copyright assignment or a DCO sign-off: https://gcc.gnu.org/contribute.html#legal > + > +#include > +#include > + > +template > +void __attribute__((noinline,noipa)) > +fill_padding(T& f) > +{ > + T mask; > + __builtin_memset(&mask, 0xff, sizeof(T)); > + __builtin_clear_padding(&mask); > + unsigned char* ptr_f =3D (unsigned char*)&f; > + unsigned char* ptr_mask =3D (unsigned char*)&mask; > + for (int i =3D 0; i < sizeof(T); i++) > + { > + if (ptr_mask[i] =3D=3D 0x00) > + { > + ptr_f[i] =3D 0xff; > + } > + } > +} > + > +void > +test01() > +{ > + long double f =3D 0.5f; // long double may contains padding on X86 > + fill_padding(f); > + std::atomic as{ f }; // padding cleared on constructor > + long double t =3D 1.5; > + > + as.fetch_add(t); > + long double s =3D f + t; > + t =3D as.load(); > + VERIFY(s =3D=3D t); // padding ignored on float comparing > + fill_padding(s); > + VERIFY(as.compare_exchange_weak(s, f)); // padding cleared on cmpexchg > + fill_padding(f); > + VERIFY(as.compare_exchange_strong(f, t)); // padding cleared on cmpexc= hg > +} > + > +int main() > +{ > + test01(); > +} > diff --git a/libstdc++-v3/testsuite/lib/dg-options.exp > b/libstdc++-v3/testsuite/lib/dg-options.exp > index 850442b6b..25da20d58 100644 > --- a/libstdc++-v3/testsuite/lib/dg-options.exp > +++ b/libstdc++-v3/testsuite/lib/dg-options.exp > @@ -356,6 +356,28 @@ proc add_options_for_libatomic { flags } { > return $flags > } > > +# Add option to link to libatomic, if required for atomics on 16-byte (1= 28-bit) > +proc add_options_for_libatomic_16b { flags } { > + if { ([istarget i?86-*-*] || [istarget x86_64-*-*]) > + } { > + global TOOL_OPTIONS > + > + set link_flags "" > + if ![is_remote host] { > + if [info exists TOOL_OPTIONS] { > + set link_flags "[atomic_link_flags [get_multilibs ${TOOL_OPTIONS}]]" > + } else { > + set link_flags "[atomic_link_flags [get_multilibs]]" > + } > + } > + > + append link_flags " -latomic " > + > + return "$flags $link_flags" > + } > + return $flags > +} > + > # Add options to enable use of deprecated features. > proc add_options_for_using-deprecated { flags } { > return "$flags -U_GLIBCXX_USE_DEPRECATED -D_GLIBCXX_USE_DEPRECATED= =3D1" > -- > 2.25.1 > > > Xi Ruoyao =E4=BA=8E2024=E5=B9=B41=E6=9C=8816=E6=97= =A5=E5=91=A8=E4=BA=8C 18:12=E5=86=99=E9=81=93=EF=BC=9A > > > > On Tue, 2024-01-16 at 17:53 +0800, xndcn wrote: > > > Thanks, so I add a test: atomic_float/compare_exchange_padding.cc, > > > which will fail due to timeout without the patch. > > > > Please resend in plain text instead of HTML. Sending in HTML causes th= e > > patch mangled. > > > > And libstdc++ patches should CC libstdc++@gcc.gnu.org, in addition to > > gcc-patches@gcc.gnu.org. > > > > > --- > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/atomic_base.h: add __builtin_clear_padding in __atomi= c_float constructor. > > > * testsuite/lib/dg-options.exp: enable libatomic for IA32 and X86-64= . > > > * testsuite/29_atomics/atomic_float/compare_exchange_padding.cc: New= test. > > > --- > > > libstdc++-v3/include/bits/atomic_base.h | 7 ++- > > > .../atomic_float/compare_exchange_padding.cc | 50 +++++++++++++++++= ++ > > > libstdc++-v3/testsuite/lib/dg-options.exp | 1 + > > > 3 files changed, 57 insertions(+), 1 deletion(-) > > > create mode 100644 libstdc++-v3/testsuite/29_atomics/atomic_float/co= mpare_exchange_padding.cc > > > > > > diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/i= nclude/bits/atomic_base.h > > > index d3a2c4f3805..cbe3749e17f 100644 > > > --- a/libstdc++-v3/include/bits/atomic_base.h > > > +++ b/libstdc++-v3/include/bits/atomic_base.h > > > @@ -1283,7 +1283,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > > > > constexpr > > > __atomic_float(_Fp __t) : _M_fp(__t) > > > - { } > > > + { > > > +#if __cplusplus >=3D 201402L && __has_builtin(__builtin_clear_paddin= g) > > > + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Fp>()) > > > + __builtin_clear_padding(std::__addressof(_M_fp)); > > > +#endif > > > + } > > > > > > __atomic_float(const __atomic_float&) =3D delete; > > > __atomic_float& operator=3D(const __atomic_float&) =3D delete; > > > diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_e= xchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare= _exchange_padding.cc > > > new file mode 100644 > > > index 00000000000..9376ab22850 > > > --- /dev/null > > > +++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange= _padding.cc > > > @@ -0,0 +1,50 @@ > > > +// { dg-do run { target c++20 } } > > > +// { dg-options "-O0" } > > > +// { dg-timeout 10 } > > > +// { dg-additional-options "-mlong-double-80" { target x86_64-*-* } = } > > > +// { dg-do run { target { ia32 || x86_64-*-* } } } > > > +// { dg-add-options libatomic } > > > + > > > +#include > > > +#include > > > + > > > +template > > > +void __attribute__((noinline,noipa)) > > > +fill_padding(T& f) > > > +{ > > > + T mask; > > > + __builtin_memset(&mask, 0xff, sizeof(T)); > > > + __builtin_clear_padding(&mask); > > > + unsigned char* ptr_f =3D (unsigned char*)&f; > > > + unsigned char* ptr_mask =3D (unsigned char*)&mask; > > > + for (int i =3D 0; i < sizeof(T); i++) > > > + { > > > + if (ptr_mask[i] =3D=3D 0x00) > > > + { > > > + ptr_f[i] =3D 0xff; > > > + } > > > + } > > > +} > > > + > > > +void > > > +test01() > > > +{ > > > + long double f =3D 0.5f; // long double may contains padding on X86 > > > + fill_padding(f); > > > + std::atomic as{ f }; // padding cleared on constructo= r > > > + long double t =3D 1.5; > > > + > > > + as.fetch_add(t); > > > + long double s =3D f + t; > > > + t =3D as.load(); > > > + VERIFY(s =3D=3D t); // padding ignored on float comparing > > > + fill_padding(s); > > > + VERIFY(as.compare_exchange_weak(s, f)); // padding cleared on cmpe= xchg > > > + fill_padding(f); > > > + VERIFY(as.compare_exchange_strong(f, t)); // padding cleared on cm= pexchg > > > +} > > > + > > > +int main() > > > +{ > > > + test01(); > > > +} > > > diff --git a/libstdc++-v3/testsuite/lib/dg-options.exp b/libstdc++-v3= /testsuite/lib/dg-options.exp > > > index bc387d17ed7..d9a19dadd7f 100644 > > > --- a/libstdc++-v3/testsuite/lib/dg-options.exp > > > +++ b/libstdc++-v3/testsuite/lib/dg-options.exp > > > @@ -337,6 +337,7 @@ proc add_options_for_libatomic { flags } { > > > || ([istarget powerpc*-*-*] && [check_effective_target_ilp32]) > > > || [istarget riscv*-*-*] > > > || ([istarget sparc*-*-linux-gnu] && [check_effective_target_ilp32= ]) > > > + || ([istarget i?86-*-*] || [istarget x86_64-*-*]) > > > > This seems too overkill as "dg-add-options libatomic" is not intended t= o > > handle 16-byte atomics. Maybe we can fork this to a new dg-add-options > > like "add_options_for_libatomic_16b"? > > > > > } { > > > global TOOL_OPTIONS > > > > > > -- > > > 2.25.1 > > > > -- > > Xi Ruoyao > > School of Aerospace Science and Technology, Xidian University >