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.133.124]) by sourceware.org (Postfix) with ESMTPS id 24E6B3857C4B for ; Thu, 1 Feb 2024 13:54:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 24E6B3857C4B 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 24E6B3857C4B 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=1706795682; cv=none; b=GtrEP1kgccRNhDEimlDUnYDqczVU6OnmX5hd0aCi1JhWUYE+h4TWmlZ5Fonx7WZt/YudmSqajbXuahTh+HnwqyxYHqfrgUUH0YmUaYoXNP8g0L4hrgLSwOeneNy0M/Og7XbeRU/3wTxVEBnVTLJrCK9SbdyanSYmgk93Y47o9PI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706795682; c=relaxed/simple; bh=Xz5S+QSYvJgCcT2/Ee5/kpBLXtxY7BAPlIiuTTzumeo=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=iDGm0uovbph5nGibE9E+4MmAp4vvD+1UyiszLypj7WfsKmftPxUD/Y4llJ8Esq7WTYf2gjfPddfYiY+uX3Y94JFpTLqAkpn10OnY7GJO7qYz6n+m9eoJL9YjT/lMUhVjkQUY/PGFZhWQAsZEbNAmtUDxwfl13aATkWdqAA17uCE= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706795680; 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: in-reply-to:in-reply-to:references:references; bh=mhrrx9+N2erVyjIQCwD2ZjpnkPn97k+uJiqc3hvRrCk=; b=JVaT8WQWoex6EVS7Pb/vlqNTHEeXclTP/ypg3DCMAIUx3WZcflxhDRfrS0wimRJuTF8/+h LxiZa01XP+YKIUYbiIYu/xPKbU3E4cYCaPcZsV1A46qVTalozJ9kek5KTRpfF9nsTlNz0w CNPtMFyE14nyW5DTzeOE9gTa8QIh4DM= Received: from mail-yb1-f199.google.com (mail-yb1-f199.google.com [209.85.219.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-360-OqkWwfXePs2hogcy5vH-3Q-1; Thu, 01 Feb 2024 08:54:37 -0500 X-MC-Unique: OqkWwfXePs2hogcy5vH-3Q-1 Received: by mail-yb1-f199.google.com with SMTP id 3f1490d57ef6-dc693399655so1498791276.1 for ; Thu, 01 Feb 2024 05:54:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706795677; x=1707400477; h=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=mhrrx9+N2erVyjIQCwD2ZjpnkPn97k+uJiqc3hvRrCk=; b=lNV6lVlMIX1PXuHAUqjbpZKtD3qYGM6OSZKKQZD/ekEqJJWGMruduryVBIa4GrnLnt nBRXJY7ShV7Pupa0/Cy0D8mkp448DykMzgw0VMhayEQjvrU7QWj1WzHckS/Pec1c272v /xHzqVbyM15XFTnZvWphJDkvwj6AhOn39KumLUdYZGQqan44j0VP1LknB/b/WjpaIMAC k/j2gNi89cb9PKgY5J+ILxy7jBHVtYik+gpMcX2gwLVuQHNFB4IlrEg51EGz4h69Q1a+ W7CdbdiXnJB33DY7jEoyxnY9Sqy905ZDs2610M2eiFpIqvhgfvbRnKCXGUNotGPzizeZ kfXg== X-Gm-Message-State: AOJu0Yw/9xCmKjNdibS2F8a5VywN4cctbAB64H1IDGuihiFgJEEpeXHk cuUY/0eAjEQLoETIQWOJE+1f/R6Ep53JwfSjk4lVMZWby11k2/qa04oudwVP/f80sGeKIPdE6cF aIzY4MKnkKXAqIYaBNRtiSfKdwiiLAx33SYBNBZtl6MNdev84B3bNbAjMqJQdsC8oFriavnEDGI jp8I3MQIR12Rp6Pyl+PgxyU/xJyIc= X-Received: by 2002:a25:b9c3:0:b0:dc2:399b:644d with SMTP id y3-20020a25b9c3000000b00dc2399b644dmr5214997ybj.38.1706795677168; Thu, 01 Feb 2024 05:54:37 -0800 (PST) X-Google-Smtp-Source: AGHT+IEpHvUZAZ53trAgVhEOYF3Mk6WmP7lvy4TUuGfX71M1vwW1ncTmAiu4h6dYwNNKSTNt/YUyMIYrO3PQmtWrNwY= X-Received: by 2002:a25:b9c3:0:b0:dc2:399b:644d with SMTP id y3-20020a25b9c3000000b00dc2399b644dmr5214984ybj.38.1706795676830; Thu, 01 Feb 2024 05:54:36 -0800 (PST) MIME-Version: 1.0 References: <9f2bdd3a973d98199bb5b322baa575ab2fba8a58.camel@xry111.site> In-Reply-To: From: Jonathan Wakely Date: Thu, 1 Feb 2024 13:54:20 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: atomic: Add missing clear_padding in __atomic_float constructor To: xndcn Cc: Jakub Jelinek , Xi Ruoyao , "H.J. Lu" , GCC Patches , libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-12.8 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_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 Wed, 31 Jan 2024 at 17:20, xndcn wrote: > > Thanks! > > > Why not just use -latomic unconditionally here? > testsuites of libstdc++ seems to have some tricks to find and link > libatomic.a by using "dg-add-options libatomic", which do nothing for > x86 target. In previous email, we decide not to pollute the current > option, so we add a new libatomic_16b option here. Right, we don't want to change the existing libatomic option. But we don't need a new one if it's going to be used in exactly one test and if the new option does the same thing for all targets that run the test. However, on that subject, does the test really need to be restricted to x86? It shouldn't loop on any target, right? > > > Why dg-timeout? > without the patch, "as.fetch_add(t);" will result in an infinite loop, > so I add timeout to help it escape. Should I keep it or not? No, because the patch is supposed to prevent the infinite loop, and so there's no need to stop it looping after 10s. It won't loop at all. > > > Also, the target selectors above look wrong. > Thanks, fixed with checking "std::numeric_limits::digits == 64". > > > This code is not compiled for C++11 and C++14, so this should just use > > constexpr not _GLIBCXX17_CONSTEXPR. > Thanks, fixed with "constexpr". Actually, why are you using the built-in directly in the first place? See below. > > So here is the fixed patch, please review it again, thanks: > --- > 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. > > Signed-off-by: xndcn > --- > libstdc++-v3/include/bits/atomic_base.h | 7 ++- > .../atomic_float/compare_exchange_padding.cc | 54 +++++++++++++++++++ > libstdc++-v3/testsuite/lib/dg-options.exp | 22 ++++++++ > 3 files changed, 82 insertions(+), 1 deletion(-) > create mode 100644 > libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc > > diff --git a/libstdc++-v3/include/bits/atomic_base.h > b/libstdc++-v3/include/bits/atomic_base.h > index f4ce0fa53..90e3f0e4b 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 __has_builtin(__builtin_clear_padding) > + if constexpr (__atomic_impl::__maybe_has_padding<_Fp>()) > + __builtin_clear_padding(std::__addressof(_M_fp)); > +#endif > + } Why can't this be simply: { __atomic_impl::__clear_padding(_M_fp); } ? We only need to clear padding for long double, not float and double, right? > > __atomic_float(const __atomic_float&) = delete; > __atomic_float& operator=(const __atomic_float&) = delete; > diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc > b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc > new file mode 100644 > index 000000000..e6e17e4b5 > --- /dev/null > +++ b/libstdc++-v3/testsuite/29_atomics/atomic_float/compare_exchange_padding.cc > @@ -0,0 +1,54 @@ > +// { dg-do run { target c++20 } } > +// { dg-options "-O0" } > +// { dg-timeout 10 } > +// { dg-do run { target { i?86-*-* x86_64-*-* } } } Why can't we run this on all targets? > +// { dg-add-options libatomic_16b } Let's just add -latomic to the dg-options line for this one test. We don't want that in general because we want to know that atomics work without it in most cases. It should be fine to use it for one test. > + > +#include > +#include > +#include > + > +template > +void __attribute__((noinline,noipa)) > +fill_padding(T& f) > +{ > + T mask; > + __builtin_memset(&mask, 0xff, sizeof(T)); There's no reason to use __builtin_memset here, just include and use std::memcpy. > + __builtin_clear_padding(&mask); > + unsigned char* ptr_f = (unsigned char*)&f; > + unsigned char* ptr_mask = (unsigned char*)&mask; > + for (int i = 0; i < sizeof(T); i++) > + { > + if (ptr_mask[i] == 0x00) > + { > + ptr_f[i] = 0xff; > + } > + } > +} > + > +void > +test01() > +{ > + // test for long double with padding (float80) > + if constexpr (std::numeric_limits::digits == 64) > + { > + long double f = 0.5f; // long double may contains padding on X86 It definitely does have padding, just say "long double has padding bits on x86" > + fill_padding(f); > + std::atomic as{ f }; // padding cleared on constructor > + long double t = 1.5; > + > + as.fetch_add(t); > + long double s = f + t; > + t = as.load(); > + VERIFY(s == 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 cmpexchg > + } > +} > + > +int main() > +{ > + test01(); > +} > diff --git a/libstdc++-v3/testsuite/lib/dg-options.exp > b/libstdc++-v3/testsuite/lib/dg-options.exp > index 850442b6b..75b27a136 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 (128-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=1" > -- > 2.25.1 >